[lldb][debugserver] remove g/G packet handling from debugserver (#132127)

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);