[lldb][debugserver] remove g/G packet handling from debugserver #132127
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. ( llvm#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.
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) ChangesIn 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. Full diff: https://github.com/llvm/llvm-project/pull/132127.diff 3 Files Affected:
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);
|
JDevlieghere
approved these changes
Mar 20, 2025
kastiglione
approved these changes
Mar 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.