Author: Jason Molenda Date: 2025-03-20T13:32:52-07:00 New Revision: ad5cac3b06c3cb41397acc1fc96beae9b460f20c
URL: https://github.com/llvm/llvm-project/commit/ad5cac3b06c3cb41397acc1fc96beae9b460f20c DIFF: https://github.com/llvm/llvm-project/commit/ad5cac3b06c3cb41397acc1fc96beae9b460f20c.diff LOG: [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. ( https://github.com/llvm/llvm-project/pull/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. Added: Modified: lldb/test/API/tools/lldb-server/register-reading/TestGdbRemoteGPacket.py lldb/tools/debugserver/source/RNBRemote.cpp lldb/tools/debugserver/source/RNBRemote.h Removed: ################################################################################ diff --git a/lldb/test/API/tools/lldb-server/register-reading/TestGdbRemoteGPacket.py b/lldb/test/API/tools/lldb-server/register-reading/TestGdbRemoteGPacket.py index 5bae98d326762..4c2d23f02ba3c 100644 --- a/lldb/test/API/tools/lldb-server/register-reading/TestGdbRemoteGPacket.py +++ b/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() diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp b/lldb/tools/debugserver/source/RNBRemote.cpp index c225ac1667b54..eb7c5ca32c02a 100644 --- a/lldb/tools/debugserver/source/RNBRemote.cpp +++ b/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, ®_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(®_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) { diff --git a/lldb/tools/debugserver/source/RNBRemote.h b/lldb/tools/debugserver/source/RNBRemote.h index a95bece79b46c..c552713551013 100644 --- a/lldb/tools/debugserver/source/RNBRemote.h +++ b/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); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
