[LLDB] Add a way to avoid manual stepping over breakpoints when resuming #165760
Conversation
|
@llvm/pr-subscribers-lldb Author: Walter Erquinigo (walter-erquinigo) ChangesCurrently, the LLDB client will try to step over breakpoints as part of the resume operation. However, some servers don't need that functionality because of performance reasons (e.g. there are too many threads to step over). This has been mentioned in https://discourse.llvm.org/t/lldb-resume-without-single-stepping-over-breakpoints/88538 This patch adds an lldb-server extension that disables the default behavior of LLDB. I ended up using the extension list instead of qHostInfo because qHostInfo is currently building a static list, and this feature is more of a feature of the server than of the platform or host. Full diff: https://github.com/llvm/llvm-project/pull/165760.diff 8 Files Affected:
diff --git a/lldb/include/lldb/Host/common/NativeProcessProtocol.h b/lldb/include/lldb/Host/common/NativeProcessProtocol.h
index 06b36c2cc9eb5..b119809e439fd 100644
--- a/lldb/include/lldb/Host/common/NativeProcessProtocol.h
+++ b/lldb/include/lldb/Host/common/NativeProcessProtocol.h
@@ -264,8 +264,9 @@ class NativeProcessProtocol {
memory_tagging = (1u << 6),
savecore = (1u << 7),
siginfo_read = (1u << 8),
+ resume_without_disabling_breakpoints = (1u << 9),
- LLVM_MARK_AS_BITMASK_ENUM(siginfo_read)
+ LLVM_MARK_AS_BITMASK_ENUM(resume_without_disabling_breakpoints)
};
class Manager {
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index 8f5892e16cedf..29a676107ec12 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -3076,6 +3076,8 @@ void PruneThreadPlans();
/// false otherwise.
virtual bool SupportsMemoryTagging() { return false; }
+ virtual bool SupportsResumeWithoutDisablingBreakpoints() { return false; }
+
/// Does the final operation to read memory tags. E.g. sending a GDB packet.
/// It assumes that ReadMemoryTags has checked that memory tagging is enabled
/// and has expanded the memory range as needed.
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 11f164c2426ce..915ec3cafe6de 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -373,6 +373,7 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
m_supports_reverse_continue = eLazyBoolNo;
m_supports_reverse_step = eLazyBoolNo;
m_supports_multi_mem_read = eLazyBoolNo;
+ m_supports_resume_without_disabling_breakpoints = eLazyBoolNo;
m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
// not, we assume no limit
@@ -434,6 +435,8 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
m_supports_reverse_step = eLazyBoolYes;
else if (x == "MultiMemRead+")
m_supports_multi_mem_read = eLazyBoolYes;
+ else if (x == "resume-without-disabling-breakpoints+")
+ m_supports_resume_without_disabling_breakpoints = eLazyBoolYes;
// Look for a list of compressions in the features list e.g.
// qXfer:features:read+;PacketSize=20000;qEcho+;SupportedCompressions=zlib-
// deflate,lzma
@@ -686,6 +689,13 @@ bool GDBRemoteCommunicationClient::GetMemoryTaggingSupported() {
return m_supports_memory_tagging == eLazyBoolYes;
}
+bool GDBRemoteCommunicationClient::
+ GetResumeWithoutDisablingBreakpointsSupported() {
+ if (m_supports_resume_without_disabling_breakpoints == eLazyBoolCalculate)
+ GetRemoteQSupported();
+ return m_supports_resume_without_disabling_breakpoints == eLazyBoolYes;
+}
+
DataBufferSP GDBRemoteCommunicationClient::ReadMemoryTags(lldb::addr_t addr,
size_t len,
int32_t type) {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index ad590a25d0f16..c298fc8aaed11 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -445,6 +445,8 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
bool GetMemoryTaggingSupported();
+ bool GetResumeWithoutDisablingBreakpointsSupported();
+
bool UsesNativeSignals();
lldb::DataBufferSP ReadMemoryTags(lldb::addr_t addr, size_t len,
@@ -577,6 +579,7 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
LazyBool m_supports_reverse_continue = eLazyBoolCalculate;
LazyBool m_supports_reverse_step = eLazyBoolCalculate;
LazyBool m_supports_multi_mem_read = eLazyBoolCalculate;
+ LazyBool m_supports_resume_without_disabling_breakpoints = eLazyBoolCalculate;
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/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index 2f62415446b7a..22e5c1667c011 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -4326,6 +4326,8 @@ std::vector<std::string> GDBRemoteCommunicationServerLLGS::HandleFeatures(
ret.push_back("memory-tagging+");
if (bool(plugin_features & Extension::savecore))
ret.push_back("qSaveCore+");
+ if (bool(plugin_features & Extension::resume_without_disabling_breakpoints))
+ ret.push_back("resume-without-disabling-breakpoints+");
// check for client features
m_extensions_supported = {};
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 3c4d9a1f1ad37..fb36c77d8dcd7 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2869,6 +2869,10 @@ bool ProcessGDBRemote::SupportsMemoryTagging() {
return m_gdb_comm.GetMemoryTaggingSupported();
}
+bool ProcessGDBRemote::SupportsResumeWithoutDisablingBreakpoints() {
+ return m_gdb_comm.GetResumeWithoutDisablingBreakpointsSupported();
+}
+
llvm::Expected<std::vector<uint8_t>>
ProcessGDBRemote::DoReadMemoryTags(lldb::addr_t addr, size_t len,
int32_t type) {
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index eb33b52b57441..52646095652e8 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -266,6 +266,10 @@ class ProcessGDBRemote : public Process,
bool SupportsMemoryTagging() override;
+ /// \returns true if the process doesn't need the client to disable
+ /// breakpoints before issuing a resume operation.
+ bool SupportsResumeWithoutDisablingBreakpoints() override;
+
/// Broadcaster event bits definitions.
enum {
eBroadcastBitAsyncContinue = (1 << 0),
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 77a1c40b95f70..1a00e5f48e5ac 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -558,41 +558,46 @@ bool ThreadList::WillResume(RunDirection &direction) {
direction = m_process.GetBaseDirection();
}
- // Give all the threads that are likely to run a last chance to set up their
- // state before we negotiate who is actually going to get a chance to run...
- // Don't set to resume suspended threads, and if any thread wanted to stop
- // others, only call setup on the threads that request StopOthers...
- if (thread_to_run != nullptr) {
- // See if any thread wants to run stopping others. If it does, then we
- // won't setup the other threads for resume, since they aren't going to get
- // a chance to run. This is necessary because the SetupForResume might add
- // "StopOthers" plans which would then get to be part of the who-gets-to-run
- // negotiation, but they're coming in after the fact, and the threads that
- // are already set up should take priority.
- if (thread_to_run->SetupToStepOverBreakpointIfNeeded(direction)) {
- // We only need to step over breakpoints when running forward, and the
- // step-over-breakpoint plan itself wants to run forward, so this
- // keeps our desired direction.
- assert(thread_to_run->GetCurrentPlan()->GetDirection() == direction);
- }
- } else {
- for (pos = m_threads.begin(); pos != end; ++pos) {
- ThreadSP thread_sp(*pos);
- if (thread_sp->GetResumeState() != eStateSuspended) {
- if (thread_sp->IsOperatingSystemPluginThread() &&
- !thread_sp->GetBackingThread())
- continue;
- if (thread_sp->SetupToStepOverBreakpointIfNeeded(direction)) {
- // We only need to step over breakpoints when running forward, and the
- // step-over-breakpoint plan itself wants to run forward, so this
- // keeps our desired direction.
- assert(thread_sp->GetCurrentPlan()->GetDirection() == direction);
- // You can't say "stop others" and also want yourself to be suspended.
- assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
- thread_to_run = thread_sp;
- if (thread_sp->ShouldRunBeforePublicStop()) {
- // This takes precedence, so if we find one of these, service it:
- break;
+ // If the process doesn't need the client to disable breakpoints before
+ // issuing a resume operation, we can skip the step-over breakpoint plan
+ // setup.
+ if (!m_process.SupportsResumeWithoutDisablingBreakpoints()) {
+ // Give all the threads that are likely to run a last chance to set up their
+ // state before we negotiate who is actually going to get a chance to run...
+ // Don't set to resume suspended threads, and if any thread wanted to stop
+ // others, only call setup on the threads that request StopOthers...
+ if (thread_to_run != nullptr) {
+ // See if any thread wants to run stopping others. If it does, then we
+ // won't setup the other threads for resume, since they aren't going to get
+ // a chance to run. This is necessary because the SetupForResume might add
+ // "StopOthers" plans which would then get to be part of the who-gets-to-run
+ // negotiation, but they're coming in after the fact, and the threads that
+ // are already set up should take priority.
+ if (thread_to_run->SetupToStepOverBreakpointIfNeeded(direction)) {
+ // We only need to step over breakpoints when running forward, and the
+ // step-over-breakpoint plan itself wants to run forward, so this
+ // keeps our desired direction.
+ assert(thread_to_run->GetCurrentPlan()->GetDirection() == direction);
+ }
+ } else {
+ for (pos = m_threads.begin(); pos != end; ++pos) {
+ ThreadSP thread_sp(*pos);
+ if (thread_sp->GetResumeState() != eStateSuspended) {
+ if (thread_sp->IsOperatingSystemPluginThread() &&
+ !thread_sp->GetBackingThread())
+ continue;
+ if (thread_sp->SetupToStepOverBreakpointIfNeeded(direction)) {
+ // We only need to step over breakpoints when running forward, and the
+ // step-over-breakpoint plan itself wants to run forward, so this
+ // keeps our desired direction.
+ assert(thread_sp->GetCurrentPlan()->GetDirection() == direction);
+ // You can't say "stop others" and also want yourself to be suspended.
+ assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
+ thread_to_run = thread_sp;
+ if (thread_sp->ShouldRunBeforePublicStop()) {
+ // This takes precedence, so if we find one of these, service it:
+ break;
+ }
}
}
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Currently, the LLDB client will try to step over breakpoints as part of the resume operation. However, some servers don't need that functionality because of performance reasons (e.g. there are too many threads to step over). This has been mentioned in https://discourse.llvm.org/t/lldb-resume-without-single-stepping-over-breakpoints/88538 This patch adds an lldb-server extension that disables the default behavior of LLDB. I ended up using the extension list instead of qHostInfo because qHostInfo is currently building a static list, and this feature is more of a feature of the server than of the platform or host.
walter-erquinigo
force-pushed
the
upstream/werquinigo/continue
branch
from
0d0ec0a to
dd5b17f
Compare
| (*pos)->DiscardThreadPlans(true); | ||
| } | ||
|
|
||
| void ThreadList::SetUpStepOverBreakpointBeforeResumeIfNeeded( |
There was a problem hiding this comment.
@jimingham , I ended up removing some comments that seems not to reflect exactly the source code here. Let me know what you think
There was a problem hiding this comment.
That's better. It still doesn't stop someone from adding so code below this that isn't breakpoint related, which will then work on every platform but yours because they didn't notice this return. If the early return & breakpoint handling were in a separate method, then that mistake wouldn't be possible.
Not terribly important, however.
dmpots
left a comment
dmpots
left a comment
There was a problem hiding this comment.
Is there anyway we can add a test for this behavior? Maybe something in https://github.com/llvm/llvm-project/blob/main/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp?
It would be nice to have something that shows we never send the "disable-singlstep-enable" packets for the breakpoint when this feature is enabled.
I did explore this, but I couldn't find a reasonable way. Code paths that do stepping + breakpoints require a full lldb-server in the test. |
You might be able to write a test using the fake gdb-server method employed by the GDBRemoteTestBase derived tests in |
thank you!! I'll try that |
4 participants
Currently, the LLDB client will try to step over breakpoints as part of the resume operation. However, some servers don't need that functionality because of performance reasons (e.g. there are too many threads to step over). This has been mentioned in https://discourse.llvm.org/t/lldb-resume-without-single-stepping-over-breakpoints/88538
This patch adds an lldb-server extension that disables the default behavior of LLDB. I ended up using the extension list instead of qHostInfo because qHostInfo is currently building a static list, and this feature is more of a feature of the server than of the platform or host.