[lldb][debugserver] remove g/G packet handling from debugserver

In 2013 we added the QSaveRegisterState and QRestoreRegisterState
packets to checkpoint a thread's register state while executing an
inferior function call, instead of using the g packet to read all
registers into lldb, then the G packet to set them again after the
func call.

Since then, lldb has not sent g/G (except as a bug) - it either
asks for registers individually (p/P) or or asks debugserver to
save and restore the entire register set with these lldb extensions.

Felipe recently had a codepath that fell back to using g/G and found
that it does not work with the modern signed fp/sp/pc/lr registers
that we can get -- it sidesteps around the clearing of the
non-addressable bits that we do when reading/writing them, and
results in a crash. ( #132079 )

Instead of fixing that issue, I am removing g/G from debugserver
because it's not needed by lldb, and it will would be easy for
future bugs to creep in to this packet that lldb should not use,
but it can accidentally fall back to and result in subtle bugs.

This does mean that a debugger using debugserver on darwin which
doesn't use QSaveRegisterState/QRestoreRegisterState will need
to fall back to reading & writing each register individually.
I'm open to re-evaluating this decision if this proves to be
needed, and supporting these lldb extensions is onerous.

Author: jasonmolenda

lldb/test/API/tools/lldb-server/register-reading/TestGdbRemoteGPacket.py

@@ -25,39 +25,6 @@ def _extract_register_value(reg_info, reg_bank, byte_order, bytes_per_entry=8):
 
 
 class TestGdbRemoteGPacket(gdbremote_testcase.GdbRemoteTestCaseBase):
-    @skipUnlessDarwin  # G packet not supported
-    def test_g_packet(self):
-        self.build()
-        self.prep_debug_monitor_and_inferior()
-        self.test_sequence.add_log_lines(
-            [
-                "read packet: $g#67",
-                {
-                    "direction": "send",
-                    "regex": r"^\$(.+)#[0-9a-fA-F]{2}$",
-                    "capture": {1: "register_bank"},
-                },
-            ],
-            True,
-        )
-        context = self.expect_gdbremote_sequence()
-        register_bank = context.get("register_bank")
-        self.assertNotEqual(register_bank[0], "E")
-
-        self.test_sequence.add_log_lines(
-            [
-                "read packet: $G" + register_bank + "#00",
-                {
-                    "direction": "send",
-                    "regex": r"^\$(.+)#[0-9a-fA-F]{2}$",
-                    "capture": {1: "G_reply"},
-                },
-            ],
-            True,
-        )
-        context = self.expect_gdbremote_sequence()
-        self.assertNotEqual(context.get("G_reply")[0], "E")
-
     @skipIf(archs=no_match(["x86_64"]))
     def g_returns_correct_data(self, with_suffix):
         procs = self.prep_debug_monitor_and_inferior()

lldb/tools/debugserver/source/RNBRemote.cpp

@@ -243,14 +243,10 @@ void RNBRemote::CreatePacketTable() {
                      "Read memory"));
   t.push_back(Packet(read_register, &RNBRemote::HandlePacket_p, NULL, "p",
                      "Read one register"));
-  t.push_back(Packet(read_general_regs, &RNBRemote::HandlePacket_g, NULL, "g",
-                     "Read registers"));
   t.push_back(Packet(write_memory, &RNBRemote::HandlePacket_M, NULL, "M",
                      "Write memory"));
   t.push_back(Packet(write_register, &RNBRemote::HandlePacket_P, NULL, "P",
                      "Write one register"));
-  t.push_back(Packet(write_general_regs, &RNBRemote::HandlePacket_G, NULL, "G",
-                     "Write registers"));
   t.push_back(Packet(insert_mem_bp, &RNBRemote::HandlePacket_z, NULL, "Z0",
                      "Insert memory breakpoint"));
   t.push_back(Packet(remove_mem_bp, &RNBRemote::HandlePacket_z, NULL, "z0",
@@ -3288,97 +3284,6 @@ rnb_err_t RNBRemote::HandlePacket_X(const char *p) {
   return SendPacket("OK");
 }
 
-/* 'g' -- read registers
- Get the contents of the registers for the current thread,
- send them to gdb.
- Should the setting of the Hg packet determine which thread's registers
- are returned?  */
-
-rnb_err_t RNBRemote::HandlePacket_g(const char *p) {
-  std::ostringstream ostrm;
-  if (!m_ctx.HasValidProcessID()) {
-    return SendErrorPacket("E11");
-  }
-
-  if (g_num_reg_entries == 0)
-    InitializeRegisters();
-
-  nub_process_t pid = m_ctx.ProcessID();
-  nub_thread_t tid = ExtractThreadIDFromThreadSuffix(p + 1);
-  if (tid == INVALID_NUB_THREAD)
-    return HandlePacket_ILLFORMED(__FILE__, __LINE__, p,
-                                  "No thread specified in p packet");
-
-  // Get the register context size first by calling with NULL buffer
-  nub_size_t reg_ctx_size = DNBThreadGetRegisterContext(pid, tid, NULL, 0);
-  if (reg_ctx_size) {
-    // Now allocate enough space for the entire register context
-    std::vector<uint8_t> reg_ctx;
-    reg_ctx.resize(reg_ctx_size);
-    // Now read the register context
-    reg_ctx_size =
-        DNBThreadGetRegisterContext(pid, tid, &reg_ctx[0], reg_ctx.size());
-    if (reg_ctx_size) {
-      append_hex_value(ostrm, reg_ctx.data(), reg_ctx.size(), false);
-      return SendPacket(ostrm.str());
-    }
-  }
-  return SendErrorPacket("E74");
-}
-
-/* 'G XXX...' -- write registers
- How is the thread for these specified, beyond "the current thread"?
- Does gdb actually use the Hg packet to set this?  */
-
-rnb_err_t RNBRemote::HandlePacket_G(const char *p) {
-  if (!m_ctx.HasValidProcessID()) {
-    return SendErrorPacket("E11");
-  }
-
-  if (g_num_reg_entries == 0)
-    InitializeRegisters();
-
-  p += 1; // Skip the 'G'
-
-  nub_process_t pid = m_ctx.ProcessID();
-  nub_thread_t tid = ExtractThreadIDFromThreadSuffix(p);
-  if (tid == INVALID_NUB_THREAD)
-    return HandlePacket_ILLFORMED(__FILE__, __LINE__, p,
-                                  "No thread specified in p packet");
-  // Skip the thread specification in `G;thread:3488ea;[..data...]`
-  const char *last_semi = strrchr(p, ';');
-  if (last_semi)
-    p = last_semi + 1;
-
-  StdStringExtractor packet(p);
-
-  // Get the register context size first by calling with NULL buffer
-  nub_size_t reg_ctx_size = DNBThreadGetRegisterContext(pid, tid, NULL, 0);
-  if (reg_ctx_size) {
-    // Now allocate enough space for the entire register context
-    std::vector<uint8_t> reg_ctx;
-    reg_ctx.resize(reg_ctx_size);
-
-    const nub_size_t bytes_extracted =
-        packet.GetHexBytes(&reg_ctx[0], reg_ctx.size(), 0xcc);
-    if (bytes_extracted == reg_ctx.size()) {
-      // Now write the register context
-      reg_ctx_size =
-          DNBThreadSetRegisterContext(pid, tid, reg_ctx.data(), reg_ctx.size());
-      if (reg_ctx_size == reg_ctx.size())
-        return SendPacket("OK");
-      else
-        return SendErrorPacket("E55");
-    } else {
-      DNBLogError("RNBRemote::HandlePacket_G(%s): extracted %llu of %llu "
-                  "bytes, size mismatch\n",
-                  p, (uint64_t)bytes_extracted, (uint64_t)reg_ctx_size);
-      return SendErrorPacket("E64");
-    }
-  }
-  return SendErrorPacket("E65");
-}
-
 static bool RNBRemoteShouldCancelCallback(void *not_used) {
   RNBRemoteSP remoteSP(g_remoteSP);
   if (remoteSP.get() != NULL) {

lldb/tools/debugserver/source/RNBRemote.h

@@ -46,8 +46,6 @@ class RNBRemote {
     cont,                          // 'c'
     continue_with_sig,             // 'C'
     detach,                        // 'D'
-    read_general_regs,             // 'g'
-    write_general_regs,            // 'G'
     set_thread,                    // 'H'
     step_inferior_one_cycle,       // 'i'
     signal_and_step_inf_one_cycle, // 'I'
@@ -221,8 +219,6 @@ class RNBRemote {
   rnb_err_t HandlePacket_M(const char *p);
   rnb_err_t HandlePacket_x(const char *p);
   rnb_err_t HandlePacket_X(const char *p);
-  rnb_err_t HandlePacket_g(const char *p);
-  rnb_err_t HandlePacket_G(const char *p);
   rnb_err_t HandlePacket_z(const char *p);
   rnb_err_t HandlePacket_T(const char *p);
   rnb_err_t HandlePacket_p(const char *p);