[Lldb-commits] [lldb] [lldb] Add QSupported key to report watchpoint types supported (PR #80376)

Jason Molenda via lldb-commits lldb-commits at lists.llvm.org
Thu Feb 1 19:11:21 PST 2024


https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/80376

>From 70a518030f2b23ca130a8d0ea667729d7985795c Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 1 Feb 2024 17:46:03 -0800
Subject: [PATCH 1/2] [lldb] Add QSupported key to report watchpoint types
 supported

debugserver on arm64 devices can manage both Byte Address Select
watchpoints (1-8 bytes) and MASK watchpoints (8 bytes-2 gigabytes).
This adds a SupportedWatchpointTypes key to the QSupported response
from debugserver with a list of these, so lldb can take full advantage
of them when creating larger regions with a single hardware watchpoint.

Also add documentation for this, and two other lldb extensions, to
the lldb-gdb-remote.txt documentation.

Re-enable TestLargeWatchpoint.py on Darwin systems when testing with
the in-tree built debugserver.  I can remove the "in-tree built
debugserver" in the future when this new key is handled by an Xcode
debugserver.
---
 lldb/docs/lldb-gdb-remote.txt                 | 37 +++++++++++++++++++
 .../tools/lldb-server/gdbremote_testcase.py   |  2 +
 .../GDBRemoteCommunicationClient.cpp          | 21 +++++++++++
 .../gdb-remote/GDBRemoteCommunicationClient.h |  4 ++
 .../Process/gdb-remote/ProcessGDBRemote.cpp   | 11 +-----
 .../large-watchpoint/TestLargeWatchpoint.py   |  5 ---
 lldb/tools/debugserver/source/RNBRemote.cpp   | 30 ++++++++-------
 7 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/lldb/docs/lldb-gdb-remote.txt b/lldb/docs/lldb-gdb-remote.txt
index 58269e4c2b688..8db2fbc47b165 100644
--- a/lldb/docs/lldb-gdb-remote.txt
+++ b/lldb/docs/lldb-gdb-remote.txt
@@ -38,7 +38,44 @@ read packet: +
 read packet: $OK#9a
 send packet: +
 
+//----------------------------------------------------------------------
+// "QSupported"
+//
+// BRIEF
+//  Query the GDB remote server for features it supports
+//
+// PRIORITY TO IMPLEMENT
+//  Optional.
+//----------------------------------------------------------------------
 
+QSupported is a standard GDB Remote Serial Protocol packet, but
+there are several additions to the response that lldb can parse.
+An example exchange:
+
+send packet: qSupported:xmlRegisters=i386,arm,mips,arc;multiprocess+;fork-events+;vfork-events+
+
+read packet: qXfer:features:read+;PacketSize=20000;qEcho+;native-signals+;SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;SupportedWatchpointTypes =aarch64-mask,aarch64-bas;
+
+In this example, three lldb extensions are reported:
+  PacketSize=20000
+    The base16 maximum packet size that the GDB Remote Serial stub
+    can handle.
+  SupportedCompressions=<item,item,...>
+    A list of compression types that the GDB Remote Serial stub can use to
+    compress packets when the QEnableCompression packet is used to request one
+    of them.
+  SupportedWatchpointTypes=<item,item,...>
+    A list of watchpoint types that this GDB Remote Serial stub can manage.
+    Currently defined names are:
+        x86_64       64-bit x86-64 watchpoints
+                     (1, 2, 4, 8 byte watchpoints aligned to those amounts)
+        aarch64-bas  AArch64 Byte Address Select watchpoints
+                     (any number of contiguous bytes within a doubleword)
+        aarch64-mask AArch64 MASK watchpoints
+                     (any power-of-2 region of memory from 8 to 2GB, aligned)
+
+lldb will default to sending power-of-2 watchpoints up to a pointer size
+(void*) in the target process if nothing is specified.
 
 //----------------------------------------------------------------------
 // "A" - launch args packet
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
index 3341b6e54a3bc..75522158b3221 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
@@ -921,6 +921,8 @@ def add_qSupported_packets(self, client_features=[]):
         "qSaveCore",
         "native-signals",
         "QNonStop",
+        "SupportedWatchpointTypes",
+        "SupportedCompressions",
     ]
 
     def parse_qSupported_response(self, context):
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 7bb4498418513..c625adc87cbd4 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -403,6 +403,22 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
         x.split(compressions, ',');
         if (!compressions.empty())
           MaybeEnableCompression(compressions);
+      } else if (x.consume_front("SupportedWatchpointTypes=")) {
+        llvm::SmallVector<llvm::StringRef, 4> watchpoint_types;
+        x.split(watchpoint_types, ',');
+        m_watchpoint_types =
+            WatchpointHardwareFeature::eWatchpointHardwareFeatureUnknown;
+        for (auto wp_type : watchpoint_types) {
+          if (wp_type == "x86_64")
+            m_watchpoint_types |=
+                WatchpointHardwareFeature::eWatchpointHardwareX86;
+          if (wp_type == "aarch64-mask")
+            m_watchpoint_types |=
+                WatchpointHardwareFeature::eWatchpointHardwareArmMASK;
+          if (wp_type == "aarch64-bas")
+            m_watchpoint_types |=
+                WatchpointHardwareFeature::eWatchpointHardwareArmBAS;
+        }
       } else if (x.consume_front("PacketSize=")) {
         StringExtractorGDBRemote packet_response(x);
         m_max_packet_size =
@@ -1828,6 +1844,11 @@ std::optional<uint32_t> GDBRemoteCommunicationClient::GetWatchpointSlotCount() {
   return num;
 }
 
+WatchpointHardwareFeature
+GDBRemoteCommunicationClient::GetSupportedWatchpointTypes() {
+  return m_watchpoint_types;
+}
+
 std::optional<bool> GDBRemoteCommunicationClient::GetWatchpointReportedAfter() {
   if (m_qHostInfo_is_valid == eLazyBoolCalculate)
     GetHostInfo();
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index 866b0773d86d5..f25b3f9dd1a6d 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -199,6 +199,8 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
 
   std::optional<bool> GetWatchpointReportedAfter();
 
+  lldb::WatchpointHardwareFeature GetSupportedWatchpointTypes();
+
   const ArchSpec &GetHostArchitecture();
 
   std::chrono::seconds GetHostDefaultPacketTimeout();
@@ -581,6 +583,8 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
   lldb::tid_t m_curr_tid_run = LLDB_INVALID_THREAD_ID;
 
   uint32_t m_num_supported_hardware_watchpoints = 0;
+  lldb::WatchpointHardwareFeature m_watchpoint_types =
+      lldb::WatchpointHardwareFeature::eWatchpointHardwareFeatureUnknown;
   uint32_t m_low_mem_addressing_bits = 0;
   uint32_t m_high_mem_addressing_bits = 0;
 
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 4e3447e767c35..629b191f3117a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3156,16 +3156,7 @@ Status ProcessGDBRemote::EnableWatchpoint(WatchpointSP wp_sp, bool notify) {
 
   ArchSpec target_arch = GetTarget().GetArchitecture();
   WatchpointHardwareFeature supported_features =
-      eWatchpointHardwareFeatureUnknown;
-
-  // LWP_TODO: enable MASK watchpoint for arm64 debugserver
-  // when it reports that it supports them.
-  if (target_arch.GetTriple().getOS() == llvm::Triple::MacOSX &&
-      target_arch.GetTriple().getArch() == llvm::Triple::aarch64) {
-#if 0
-       supported_features |= eWatchpointHardwareArmMASK;
-#endif
-  }
+      m_gdb_comm.GetSupportedWatchpointTypes();
 
   std::vector<WatchpointResourceSP> resources =
       WatchpointAlgorithms::AtomizeWatchpointRequest(
diff --git a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py
index f7ceb47c0b615..c5e161497e628 100644
--- a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py
+++ b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py
@@ -25,11 +25,6 @@ def continue_and_report_stop_reason(self, process, iter_str):
     @skipIf(archs=no_match(["arm64", "arm64e", "aarch64"]))
     @skipUnlessDarwin
 
-    # LWP_TODO: until debugserver advertises that it supports
-    # MASK watchpoints, this test can't be enabled, lldb won't
-    # try to send watchpoints larger than 8 bytes.
-    @skipIfDarwin
-
     # debugserver only gained the ability to watch larger regions
     # with this patch.
     @skipIfOutOfTreeDebugserver
diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp b/lldb/tools/debugserver/source/RNBRemote.cpp
index 224ed033fc421..20384920823d2 100644
--- a/lldb/tools/debugserver/source/RNBRemote.cpp
+++ b/lldb/tools/debugserver/source/RNBRemote.cpp
@@ -3557,10 +3557,10 @@ static bool GetProcessNameFrom_vAttach(const char *&p,
 rnb_err_t RNBRemote::HandlePacket_qSupported(const char *p) {
   uint32_t max_packet_size = 128 * 1024; // 128KBytes is a reasonable max packet
                                          // size--debugger can always use less
-  char buf[256];
-  snprintf(buf, sizeof(buf),
-           "qXfer:features:read+;PacketSize=%x;qEcho+;native-signals+",
-           max_packet_size);
+  std::stringstream reply;
+  reply << "qXfer:features:read+;PacketSize=" << std::hex << max_packet_size
+        << ";";
+  reply << "qEcho+;native-signals+;";
 
   bool enable_compression = false;
   (void)enable_compression;
@@ -3574,15 +3574,19 @@ rnb_err_t RNBRemote::HandlePacket_qSupported(const char *p) {
 #endif
 
   if (enable_compression) {
-    strcat(buf, ";SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;"
-                "DefaultCompressionMinSize=");
-    char numbuf[16];
-    snprintf(numbuf, sizeof(numbuf), "%zu", m_compression_minsize);
-    numbuf[sizeof(numbuf) - 1] = '\0';
-    strcat(buf, numbuf);
-  } 
-
-  return SendPacket(buf);
+    reply << "SupportedCompressions=lzfse,zlib-deflate,lz4,lzma;";
+    reply << "DefaultCompressionMinSize=" << std::dec << m_compression_minsize
+          << ";";
+  }
+
+#if (defined(__arm64__) || defined(__aarch64__))
+  reply << "SupportedWatchpointTypes=aarch64-mask,aarch64-bas;";
+#endif
+#if defined(__x86_64__)
+  reply << "SupportedWatchpointTypes=x86_64;";
+#endif
+
+  return SendPacket(reply.str().c_str());
 }
 
 static bool process_does_not_exist (nub_process_t pid) {

>From 17e0b7b06cd0cf2fe4f9e6c1f338afb536ec7796 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 1 Feb 2024 19:10:53 -0800
Subject: [PATCH 2/2] Clarify 'size of pointer' language.

---
 lldb/docs/lldb-gdb-remote.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lldb/docs/lldb-gdb-remote.txt b/lldb/docs/lldb-gdb-remote.txt
index 8db2fbc47b165..a291f6bd150eb 100644
--- a/lldb/docs/lldb-gdb-remote.txt
+++ b/lldb/docs/lldb-gdb-remote.txt
@@ -74,8 +74,8 @@ In this example, three lldb extensions are reported:
         aarch64-mask AArch64 MASK watchpoints
                      (any power-of-2 region of memory from 8 to 2GB, aligned)
 
-lldb will default to sending power-of-2 watchpoints up to a pointer size
-(void*) in the target process if nothing is specified.
+lldb will default to sending power-of-2 watchpoints up to a pointer size,
+`sizeof(void*)`, in the target process if nothing is specified.
 
 //----------------------------------------------------------------------
 // "A" - launch args packet



More information about the lldb-commits mailing list