[lldb] Add support for gdb-style 'x' packet #124733

labath · 2025-01-28T11:40:19Z

See also https://discourse.llvm.org/t/rfc-fixing-incompatibilties-of-the-x-packet-w-r-t-gdb/84288 and https://sourceware.org/pipermail/gdb/2025-January/051705.html

labath · 2025-01-28T11:40:34Z

@rocallahan

github-actions · 2025-01-28T11:43:44Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2025-01-28T11:43:44Z

✅ With the latest revision this PR passed the C/C++ code formatter.

DO NOT SUBMIT until the mechanism for detection of the packet format is confirmed.

llvmbot · 2025-01-28T16:17:26Z

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

See also https://discourse.llvm.org/t/rfc-fixing-incompatibilties-of-the-x-packet-w-r-t-gdb/84288 and https://sourceware.org/pipermail/gdb/2025-January/051705.html

Full diff: https://github.com/llvm/llvm-project/pull/124733.diff

5 Files Affected:

(modified) lldb/packages/Python/lldbsuite/test/gdbclientutils.py (+6)
(modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (+13-9)
(modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h (+7-2)
(modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+23-13)
(added) lldb/test/API/functionalities/gdb_remote_client/TestReadMemory.py (+36)

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..50f11cb009b8ca 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -218,7 +218,12 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
 
   bool GetpPacketSupported(lldb::tid_t tid);
 
-  bool GetxPacketSupported();
+  enum class xPacketState {
+    Unimplemented,
+    Prefixed, // Successful responses start with a 'b' character.
+    Bare,     // No prefix, packets starts with the memory being read.
+  };
+  xPacketState GetxPacketState();
 
   bool GetVAttachOrWaitSupported();
 
@@ -541,7 +546,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 +565,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..63c3b505358cbf
--- /dev/null
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestReadMemory.py
@@ -0,0 +1,36 @@
+import lldb
+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):
+                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):
+                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))

labath · 2025-01-28T16:19:03Z

Gdb uses the same feature string to detect the packet, and a version with that fix is going to released soon. I've tested that an lldb with this patch can communicate with gdbserver with the fix (at least that they get far enough to start reading memory).

(We are also able to communicate with the gdbserver without that fix, but in that case we do that using the m packets (because gdbserver does not respond OK to our x0,0 probe))

jasonmolenda · 2025-01-28T22:50:36Z

For lldb-server/debugserver, we might want to distinguish if the connecting lldb is old-lldb (only knows the lldb x) or new-lldb (knows lldb x and gdb x). I think in GDBRemoteCommunicationClient::GetRemoteQSupported lldb should add binary-upload+ to the features list it sends to debugserver/lldb-server, and lldb-server/debugserver should reply with binary-upload+ in its response. Then lldb-server/debugserver knows to send a gdb-style x, and lldb knows to expect that response.

For right now, lldb-server and debugserver will default to lldb-x behavior, unless it was requested (binary-upload+) to use gdb-x behavior. We will switch that default behavior after a short while, when we are comfortable that old-lldb's will not be connecting to current-debugserver/lldb-servers.

When lldb is sniffing the capability of the remote stub, x0,0 -> OK means lldb-x unambiguously. x0,0 -> b and x0,0 -> Exx are gdb-x unambiguously. Any other response (e.g. empty-packet response, indicating an unknown packet) will make lldb fall back to using the asciihex m packet to read memory.

Does this seem like a good approach for compatibility?

(Note I said that lldb-server/debugserver should reply with binary-upload+ in its response to QSupported, but this isn't actually needed. After lldb sends binary-upload+ in the list of features, lldb-server/debugserver knows it should use gdb-x responses, and when lldb sends x0,0, it will reply b.)

labath · 2025-01-29T12:41:54Z

Let's continue the discussion here.

(It could work it's just not how gdb implemented it. Sniffing of zero-length reads was considered, but a qSupported feature was deemed more principled. I suppose one advantage of that is that it saves us one round-trip (after we no longer need to support/detect the lldb-x implementation))

jasonmolenda

This patch looks good to me, thanks for the explanation in the discourse of the points I misunderstood initially.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

lldb/test/API/functionalities/gdb_remote_client/TestReadMemory.py

DavidSpickett

LGTM, thanks for handling this.

Maybe the final say goes to @rocallahan since rr is the origin of the issue? We have to wait on GDB for a bit anyway I expect.

labath · 2025-01-30T14:43:50Z

GDB has already implemented this, and it looks like they'll do a new release this weekend: https://sourceware.org/pipermail/gdb/2025-January/051731.html

Robert, any last words before I merge this? :)

rocallahan · 2025-01-30T17:40:37Z

It sounds fine. rr HEAD uses the Gdb flavour for GDB and the old-LLDB flavor for LLDB. After a decent amount of time has passed I will update to the new regime.

rocallahan · 2025-01-30T17:41:09Z

And thanks!

labath · 2025-02-04T08:58:18Z

/cherry-pick 13d0318

llvmbot · 2025-02-04T09:04:47Z

/pull-request #125653

See also https://discourse.llvm.org/t/rfc-fixing-incompatibilties-of-the-x-packet-w-r-t-gdb/84288 and https://sourceware.org/pipermail/gdb/2025-January/051705.html (cherry picked from commit 13d0318)

labath requested review from DavidSpickett and jasonmolenda January 28, 2025 11:40

[lldb] Add support for gdb-style 'x' packet

bf011cc

DO NOT SUBMIT until the mechanism for detection of the packet format is confirmed.

labath force-pushed the x branch from dfecd93 to bf011cc Compare January 28, 2025 11:56

labath marked this pull request as ready for review January 28, 2025 16:15

labath requested a review from JDevlieghere as a code owner January 28, 2025 16:15

llvmbot added the lldb label Jan 28, 2025

jasonmolenda approved these changes Jan 30, 2025

View reviewed changes

DavidSpickett reviewed Jan 30, 2025

View reviewed changes

add comments, test for m

4bb49db

DavidSpickett approved these changes Jan 30, 2025

View reviewed changes

labath merged commit 13d0318 into llvm:main Jan 31, 2025
7 checks passed

labath deleted the x branch January 31, 2025 08:07

labath added this to the LLVM 20.X Release milestone Feb 4, 2025

[lldb] Add support for gdb-style 'x' packet #124733

[lldb] Add support for gdb-style 'x' packet #124733

Uh oh!

Conversation

labath commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labath commented Jan 28, 2025

Uh oh!

github-actions bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 28, 2025

Uh oh!

labath commented Jan 28, 2025

Uh oh!

jasonmolenda commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labath commented Jan 29, 2025

Uh oh!

jasonmolenda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavidSpickett left a comment

Choose a reason for hiding this comment

Uh oh!

labath commented Jan 30, 2025

Uh oh!

rocallahan commented Jan 30, 2025

Uh oh!

rocallahan commented Jan 30, 2025

Uh oh!

Uh oh!

labath commented Feb 4, 2025

Uh oh!

llvmbot commented Feb 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

labath commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

jasonmolenda commented Jan 28, 2025 •

edited

Loading