release/20.x: [lldb] Add support for gdb-style 'x' packet (#124733) #125653
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
|
@DavidSpickett What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-lldb Author: None (llvmbot) ChangesBackport 13d0318 Requested by: @labath Full diff: https://github.com/llvm/llvm-project/pull/125653.diff 5 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
index 1784487323ad6b..4b782b3b470fe2 100644
--- a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
+++ b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
@@ -126,6 +126,9 @@ def respond(self, packet):
if packet[0] == "m":
addr, length = [int(x, 16) for x in packet[1:].split(",")]
return self.readMemory(addr, length)
+ if packet[0] == "x":
+ addr, length = [int(x, 16) for x in packet[1:].split(",")]
+ return self.x(addr, length)
if packet[0] == "M":
location, encoded_data = packet[1:].split(":")
addr, length = [int(x, 16) for x in location.split(",")]
@@ -267,6 +270,9 @@ def writeRegister(self, register, value_hex):
def readMemory(self, addr, length):
return "00" * length
+ def x(self, addr, length):
+ return ""
+
def writeMemory(self, addr, data_hex):
return "OK"
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index b3f1c6f052955b..581dd8f8e0b6b6 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -275,7 +275,6 @@ void GDBRemoteCommunicationClient::ResetDiscoverableSettings(bool did_exec) {
m_supports_vCont_s = eLazyBoolCalculate;
m_supports_vCont_S = eLazyBoolCalculate;
m_supports_p = eLazyBoolCalculate;
- m_supports_x = eLazyBoolCalculate;
m_supports_QSaveRegisterState = eLazyBoolCalculate;
m_qHostInfo_is_valid = eLazyBoolCalculate;
m_curr_pid_is_valid = eLazyBoolCalculate;
@@ -295,6 +294,7 @@ void GDBRemoteCommunicationClient::ResetDiscoverableSettings(bool did_exec) {
m_supports_qXfer_siginfo_read = eLazyBoolCalculate;
m_supports_augmented_libraries_svr4_read = eLazyBoolCalculate;
m_uses_native_signals = eLazyBoolCalculate;
+ m_x_packet_state.reset();
m_supports_qProcessInfoPID = true;
m_supports_qfProcessInfo = true;
m_supports_qUserName = true;
@@ -348,6 +348,7 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
m_supports_memory_tagging = eLazyBoolNo;
m_supports_qSaveCore = eLazyBoolNo;
m_uses_native_signals = eLazyBoolNo;
+ m_x_packet_state.reset();
m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
// not, we assume no limit
@@ -401,6 +402,8 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
m_supports_qSaveCore = eLazyBoolYes;
else if (x == "native-signals+")
m_uses_native_signals = eLazyBoolYes;
+ else if (x == "binary-upload+")
+ m_x_packet_state = xPacketState::Prefixed;
// Look for a list of compressions in the features list e.g.
// qXfer:features:read+;PacketSize=20000;qEcho+;SupportedCompressions=zlib-
// deflate,lzma
@@ -715,19 +718,20 @@ Status GDBRemoteCommunicationClient::WriteMemoryTags(
return status;
}
-bool GDBRemoteCommunicationClient::GetxPacketSupported() {
- if (m_supports_x == eLazyBoolCalculate) {
+GDBRemoteCommunicationClient::xPacketState
+GDBRemoteCommunicationClient::GetxPacketState() {
+ if (!m_x_packet_state)
+ GetRemoteQSupported();
+ if (!m_x_packet_state) {
StringExtractorGDBRemote response;
- m_supports_x = eLazyBoolNo;
- char packet[256];
- snprintf(packet, sizeof(packet), "x0,0");
- if (SendPacketAndWaitForResponse(packet, response) ==
+ m_x_packet_state = xPacketState::Unimplemented;
+ if (SendPacketAndWaitForResponse("x0,0", response) ==
PacketResult::Success) {
if (response.IsOKResponse())
- m_supports_x = eLazyBoolYes;
+ m_x_packet_state = xPacketState::Bare;
}
}
- return m_supports_x;
+ return *m_x_packet_state;
}
lldb::pid_t GDBRemoteCommunicationClient::GetCurrentProcessID(bool allow_lazy) {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index 898d176abc3465..1118a76d7211b5 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -218,7 +218,14 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
bool GetpPacketSupported(lldb::tid_t tid);
- bool GetxPacketSupported();
+ enum class xPacketState {
+ Unimplemented,
+ Prefixed, // Successful responses start with a 'b' character. This is the
+ // style used by GDB.
+ Bare, // No prefix, packets starts with the memory being read. This is
+ // LLDB's original style.
+ };
+ xPacketState GetxPacketState();
bool GetVAttachOrWaitSupported();
@@ -541,7 +548,6 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
LazyBool m_attach_or_wait_reply = eLazyBoolCalculate;
LazyBool m_prepare_for_reg_writing_reply = eLazyBoolCalculate;
LazyBool m_supports_p = eLazyBoolCalculate;
- LazyBool m_supports_x = eLazyBoolCalculate;
LazyBool m_avoid_g_packets = eLazyBoolCalculate;
LazyBool m_supports_QSaveRegisterState = eLazyBoolCalculate;
LazyBool m_supports_qXfer_auxv_read = eLazyBoolCalculate;
@@ -561,6 +567,7 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
LazyBool m_supports_memory_tagging = eLazyBoolCalculate;
LazyBool m_supports_qSaveCore = eLazyBoolCalculate;
LazyBool m_uses_native_signals = eLazyBoolCalculate;
+ std::optional<xPacketState> m_x_packet_state;
bool m_supports_qProcessInfoPID : 1, m_supports_qfProcessInfo : 1,
m_supports_qUserName : 1, m_supports_qGroupName : 1,
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 538c8680140091..21a0fa283644d6 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2609,11 +2609,15 @@ void ProcessGDBRemote::WillPublicStop() {
// Process Memory
size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t size,
Status &error) {
+ using xPacketState = GDBRemoteCommunicationClient::xPacketState;
+
GetMaxMemorySize();
- bool binary_memory_read = m_gdb_comm.GetxPacketSupported();
+ xPacketState x_state = m_gdb_comm.GetxPacketState();
+
// M and m packets take 2 bytes for 1 byte of memory
- size_t max_memory_size =
- binary_memory_read ? m_max_memory_size : m_max_memory_size / 2;
+ size_t max_memory_size = x_state != xPacketState::Unimplemented
+ ? m_max_memory_size
+ : m_max_memory_size / 2;
if (size > max_memory_size) {
// Keep memory read sizes down to a sane limit. This function will be
// called multiple times in order to complete the task by
@@ -2624,8 +2628,8 @@ size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t size,
char packet[64];
int packet_len;
packet_len = ::snprintf(packet, sizeof(packet), "%c%" PRIx64 ",%" PRIx64,
- binary_memory_read ? 'x' : 'm', (uint64_t)addr,
- (uint64_t)size);
+ x_state != xPacketState::Unimplemented ? 'x' : 'm',
+ (uint64_t)addr, (uint64_t)size);
assert(packet_len + 1 < (int)sizeof(packet));
UNUSED_IF_ASSERT_DISABLED(packet_len);
StringExtractorGDBRemote response;
@@ -2634,19 +2638,25 @@ size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t size,
GDBRemoteCommunication::PacketResult::Success) {
if (response.IsNormalResponse()) {
error.Clear();
- if (binary_memory_read) {
+ if (x_state != xPacketState::Unimplemented) {
// The lower level GDBRemoteCommunication packet receive layer has
// already de-quoted any 0x7d character escaping that was present in
// the packet
- size_t data_received_size = response.GetBytesLeft();
- if (data_received_size > size) {
- // Don't write past the end of BUF if the remote debug server gave us
- // too much data for some reason.
- data_received_size = size;
+ llvm::StringRef data_received = response.GetStringRef();
+ if (x_state == xPacketState::Prefixed &&
+ !data_received.consume_front("b")) {
+ error = Status::FromErrorStringWithFormatv(
+ "unexpected response to GDB server memory read packet '{0}': "
+ "'{1}'",
+ packet, data_received);
+ return 0;
}
- memcpy(buf, response.GetStringRef().data(), data_received_size);
- return data_received_size;
+ // Don't write past the end of BUF if the remote debug server gave us
+ // too much data for some reason.
+ size_t memcpy_size = std::min(size, data_received.size());
+ memcpy(buf, data_received.data(), memcpy_size);
+ return memcpy_size;
} else {
return response.GetHexBytes(
llvm::MutableArrayRef<uint8_t>((uint8_t *)buf, size), '\xdd');
diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestReadMemory.py b/lldb/test/API/functionalities/gdb_remote_client/TestReadMemory.py
new file mode 100644
index 00000000000000..81dcb54aef5d8e
--- /dev/null
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestReadMemory.py
@@ -0,0 +1,55 @@
+import lldb
+from lldbsuite.support import seven
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+
+
+class TestReadMemory(GDBRemoteTestBase):
+ def test_x_with_prefix(self):
+ class MyResponder(MockGDBServerResponder):
+ def qSupported(self, client_features):
+ # binary-upload+ indicates we use the gdb style of x packets
+ return super().qSupported(client_features) + ";binary-upload+"
+
+ def x(self, addr, length):
+ return "bfoobar" if addr == 0x1000 else "E01"
+
+ self.server.responder = MyResponder()
+ target = self.dbg.CreateTargetWithFileAndTargetTriple("", "x86_64-pc-linux")
+ process = self.connect(target)
+
+ error = lldb.SBError()
+ self.assertEqual(b"foobar", process.ReadMemory(0x1000, 10, error))
+
+ def test_x_bare(self):
+ class MyResponder(MockGDBServerResponder):
+ def x(self, addr, length):
+ # The OK response indicates we use the old lldb style.
+ if addr == 0 and length == 0:
+ return "OK"
+ return "foobar" if addr == 0x1000 else "E01"
+
+ self.server.responder = MyResponder()
+ target = self.dbg.CreateTargetWithFileAndTargetTriple("", "x86_64-pc-linux")
+ process = self.connect(target)
+
+ error = lldb.SBError()
+ self.assertEqual(b"foobar", process.ReadMemory(0x1000, 10, error))
+
+ def test_m_fallback(self):
+ class MyResponder(MockGDBServerResponder):
+ def x(self, addr, length):
+ # If `x` is unsupported, we should fall back to `m`.
+ return ""
+
+ def readMemory(self, addr, length):
+ return seven.hexlify("foobar") if addr == 0x1000 else "E01"
+
+ self.server.responder = MyResponder()
+ target = self.dbg.CreateTargetWithFileAndTargetTriple("", "x86_64-pc-linux")
+ process = self.connect(target)
+
+ error = lldb.SBError()
+ self.assertEqual(b"foobar", process.ReadMemory(0x1000, 10, error))
|
|
In the future (see https://discourse.llvm.org/c/subprojects/lldb/8) we will be changing the packet format used by debug-/lldb-server. This ensures forwards-compatibility with those servers. |
|
I agree with backporting this. There should be a release note for it. I know we could get away without it because the relevant GDB has not been out (maybe not at all) at this point, but it'll be something easy to point to for us in a few years time, and GDB folks who want to confirm what we did without reading our source code. I suggest: I think this can be done as a PR to the release branch? No need to complicate this one. And it would be good to add a note to https://lldb.llvm.org/resources/lldbgdbremote.html#x-binary-memory-read (just on |
labath
added a commit
that referenced
this pull request
Feb 4, 2025
SG ~> #125680
Yes, I've been meaning to do that. Thanks for reminding me. ~> #125682 |
|
@labath (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
tstellar
pushed a commit
that referenced
this pull request
Feb 7, 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.
Backport 13d0318
Requested by: @labath