[lldb] Add QSupported key to report watchpoint types supported #80376
Conversation
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.
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) Changesdebugserver 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. Full diff: https://github.com/llvm/llvm-project/pull/80376.diff 7 Files Affected:
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) {
|
|
The QSupported packet from debugserver also has a "compression minimum size" field it adds (see the change to RNBRemote.cpp) and the QEnableCompression packet accepts that field as well. I must have had a reason for adding this when I added compression to the protocol, but lldb doesn't use this. I'll remove it in a separate patch. |
|
Just to record that I thought about it, I agree with not trying to make the names architecture neutral. For example one might say that because range watchpoints on mips and aarch64 have overlapping functionality (no pun intended) that we could report that we have "range" as a type for either of them. This leads to a situation where the functionality that doesn't overlap becomes much harder to handle and detect. So I agree with If we later want to (and kinda already do) group these by general functionality, we can do that in lldb in the algorithms you added for working out the watchpoint resources. (and sod's law, someone will propose to add |
Yeah I thought about specifying a more structured name, like "ARCH-STYLE" so we'd have "x86_64-mask" in addition to "aarch64-mask", but my guess is a lot of targets will work fine with "power of 2, up to sizeof(void*)" and never need to adjust this. Ted Woodward pointed out that the PowerPC watchpoints are quite different, so if we support that target we'll definitely need an additional algorithm for sure. |
WatchpointHardwareFeature is set by the Process plugin to WatchpointAlgorithms::AtomizeWatchpointRequest to indicate which types of watchpoints this target/stub support. It's entirely internal and should be in lldb-private-enumerations.h. To make a bitfield enum work with the bitfield, & and |, operators without lots of casting, I need the LLDB_MARK_AS_BITMASK_ENUM() constexpr template stuff from lldb-enumerations.h. It might be better to put this and FLAGS_ENUM() in their own file, but I don't think I'm messing up with any layering violations by having lldb-private-enumerations.h include lldb-enumerations.h to get them so I'll start with this.
|
@bulbazord in the most recent commit I moved this internal-only enum from lldb-enumerations.h to lldb-private-enumerations.h, but I need to use the constexpr templatey thing that LLDB_MARK_AS_BITMASK_ENUM() defines for the enum so I can use bit-wise | & operations without casting everywhere; that's defined in lldb-enumerations.h so I included the public enums in the lldb-private-enumerations.h. It seems like it's probably not a great choice, but the other one is breaking out this and FLAGS_ENUM etc into a little lldb-common-enumerations.h or something. What do you think? |
lldb/docs/lldb-gdb-remote.txt
Outdated
|
|
||
| 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 shown: |
There was a problem hiding this comment.
"In the example above"
Not the greatest thing in the world but it's not terrible either. We use the lldb public enumerations everywhere in private code, so as long as we're not going the other way this is ok to do. I will say, by moving the definitions from |
Addressing feedback from David Spickett.
|
Thanks for the feedback Alex & David, I think this one might be good enough now. |
…80376) 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. (cherry picked from commit 5953532)
…-watchpoint-capability [lldb] Add QSupported key to report watchpoint types supported (llvm#80376)
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.