[lldb-dap] Add an RAII helper for synchronous mode (NFC) #137900
Merged
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
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesWhile debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes. Full diff: https://github.com/llvm/llvm-project/pull/137900.diff 6 Files Affected:
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index b593353110787..4cb0d8e49004c 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -820,12 +820,12 @@ llvm::Error DAP::Disconnect(bool terminateDebuggee) {
case lldb::eStateCrashed:
case lldb::eStateSuspended:
case lldb::eStateStopped:
- case lldb::eStateRunning:
- debugger.SetAsync(false);
+ case lldb::eStateRunning: {
+ ScopeSyncMode scope_sync_mode(debugger);
error = terminateDebuggee ? process.Kill() : process.Detach();
- debugger.SetAsync(true);
break;
}
+ }
SendTerminatedEvent();
diff --git a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
index 3ef87cbef873c..7084d30f2625b 100644
--- a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
@@ -9,6 +9,7 @@
#include "DAP.h"
#include "EventHelper.h"
#include "JSONUtils.h"
+#include "LLDBUtils.h"
#include "RequestHandler.h"
#include "lldb/API/SBListener.h"
#include "llvm/Support/FileSystem.h"
@@ -138,9 +139,11 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
}
if (attachCommands.empty()) {
// No "attachCommands", just attach normally.
+
// Disable async events so the attach will be successful when we return from
// the launch call and the launch will happen synchronously
- dap.debugger.SetAsync(false);
+ ScopeSyncMode scope_sync_mode(dap.debugger);
+
if (core_file.empty()) {
if ((pid != LLDB_INVALID_PROCESS_ID) &&
(gdb_remote_port != invalid_port)) {
@@ -161,10 +164,9 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
// Attach by process name or id.
dap.target.Attach(attach_info, error);
}
- } else
+ } else {
dap.target.LoadCore(core_file.data(), error);
- // Reenable async events
- dap.debugger.SetAsync(true);
+ }
} else {
// We have "attachCommands" that are a set of commands that are expected
// to execute the commands after which a process should be created. If there
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index b7d3c8ced69f1..7a75cd93abc19 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -10,6 +10,7 @@
#include "DAP.h"
#include "Handler/ResponseHandler.h"
#include "JSONUtils.h"
+#include "LLDBUtils.h"
#include "Protocol/ProtocolBase.h"
#include "Protocol/ProtocolRequests.h"
#include "RunInTerminal.h"
@@ -138,7 +139,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
else
return pid.takeError();
- dap.debugger.SetAsync(false);
+ std::optional<ScopeSyncMode> scope_sync_mode(dap.debugger);
lldb::SBError error;
dap.target.Attach(attach_info, error);
@@ -162,7 +163,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
// Now that the actual target is just starting (i.e. exec was just invoked),
// we return the debugger to its async state.
- dap.debugger.SetAsync(true);
+ scope_sync_mode.reset();
// If sending the notification failed, the launcher should be dead by now and
// the async didAttach notification should have an error message, so we
@@ -244,9 +245,8 @@ llvm::Error BaseRequestHandler::LaunchProcess(
lldb::SBError error;
// Disable async events so the launch will be successful when we return from
// the launch call and the launch will happen synchronously
- dap.debugger.SetAsync(false);
+ ScopeSyncMode scope_sync_mode(dap.debugger);
dap.target.Launch(launch_info, error);
- dap.debugger.SetAsync(true);
if (error.Fail())
return llvm::make_error<DAPError>(error.GetCString());
} else {
diff --git a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
index e6f2d9ec669cb..47fedf9dd779c 100644
--- a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
@@ -9,6 +9,7 @@
#include "DAP.h"
#include "EventHelper.h"
#include "JSONUtils.h"
+#include "LLDBUtils.h"
#include "Protocol/ProtocolRequests.h"
#include "RequestHandler.h"
#include "llvm/Support/JSON.h"
@@ -121,8 +122,8 @@ void RestartRequestHandler::operator()(
// Stop the current process if necessary. The logic here is similar to
// CommandObjectProcessLaunchOrAttach::StopProcessIfNecessary, except that
// we don't ask the user for confirmation.
- dap.debugger.SetAsync(false);
if (process.IsValid()) {
+ ScopeSyncMode scope_sync_mode(dap.debugger);
lldb::StateType state = process.GetState();
if (state != lldb::eStateConnected) {
process.Kill();
@@ -131,7 +132,6 @@ void RestartRequestHandler::operator()(
// for threads of the process we are terminating.
dap.thread_ids.clear();
}
- dap.debugger.SetAsync(true);
// FIXME: Should we run 'preRunCommands'?
// FIXME: Should we add a 'preRestartCommands'?
diff --git a/lldb/tools/lldb-dap/LLDBUtils.cpp b/lldb/tools/lldb-dap/LLDBUtils.cpp
index 7c71d385e9f7e..957ece482ec1b 100644
--- a/lldb/tools/lldb-dap/LLDBUtils.cpp
+++ b/lldb/tools/lldb-dap/LLDBUtils.cpp
@@ -235,4 +235,15 @@ std::string GetStringValue(const lldb::SBStructuredData &data) {
return str;
}
+ScopeSyncMode::ScopeSyncMode(lldb::SBDebugger &debugger)
+ : m_debugger(debugger) {
+ assert(m_debugger.GetAsync() && "Debugger not in async mode!");
+ m_debugger.SetAsync(false);
+}
+
+ScopeSyncMode::~ScopeSyncMode() {
+ assert(!m_debugger.GetAsync() && "Debugger not in async mode!");
+ m_debugger.SetAsync(true);
+}
+
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/LLDBUtils.h b/lldb/tools/lldb-dap/LLDBUtils.h
index 610ebce83566c..05d987a965888 100644
--- a/lldb/tools/lldb-dap/LLDBUtils.h
+++ b/lldb/tools/lldb-dap/LLDBUtils.h
@@ -198,6 +198,16 @@ class TelemetryDispatcher {
lldb::SBDebugger *debugger;
};
+/// RAII utility to put the debugger temporarily into synchronous mode.
+class ScopeSyncMode {
+public:
+ ScopeSyncMode(lldb::SBDebugger &debugger);
+ ~ScopeSyncMode();
+
+private:
+ lldb::SBDebugger &m_debugger;
+};
+
/// Get the stop-disassembly-display settings
///
/// \param[in] debugger
@@ -207,7 +217,6 @@ class TelemetryDispatcher {
/// The value of the stop-disassembly-display setting
lldb::StopDisassemblyType GetStopDisassemblyDisplay(lldb::SBDebugger &debugger);
-
/// Take ownership of the stored error.
llvm::Error ToError(const lldb::SBError &error);
|
JDevlieghere
force-pushed
the
raii-sync-mode
branch
from
bd22e8a to
e545b97
Compare
ashgti
reviewed
Apr 30, 2025
lldb/tools/lldb-dap/LLDBUtils.cpp
Outdated
| @@ -235,4 +235,15 @@ std::string GetStringValue(const lldb::SBStructuredData &data) { | |||
| return str; | |||
| } | |||
|
|
|||
| ScopeSyncMode::ScopeSyncMode(lldb::SBDebugger &debugger) | |||
| : m_debugger(debugger) { | |||
| assert(m_debugger.GetAsync() && "Debugger not in asynchronous mode!"); | |||
There was a problem hiding this comment.
If I had a initCommand that toggled this, would that trigger this assert? e.g.
initCommands: ["script lldb.debugger.SetAsync(False)"]
Should this be a warning or error message instead of an assert?
There was a problem hiding this comment.
Yeah, that would create all kind of havoc, including triggering this assert. I'm not convinced this is the right place to flag that (and show a warning/error to the user). We should either prevent the user from changing that behind our back, or make sure we always reset it before giving control to lldb-dap. I'll remove the assert from this PR.
There was a problem hiding this comment.
I think it’s okay for scripts to have a level of control over the debugger behavior since they’re pretty helpful for extending the debugger, but I agree that if we have some assumptions about the debugger state it’s good to make sure they’re well known.
There was a problem hiding this comment.
It's not just okay to allow scripts to temporarily change the Sync vrs. Async state of the debugger, it's pretty close to necessary. Since we don't have a way from the SB API's to hijack the Process listener, the only way you can write a command that does "step, check something, step again" is to change the debugger mode to Sync.
We could solve this by forcing everyone to write scripted thread plans for this sort of thing, but that's conceptually more difficult, and I think a lot of people aren't willing to figure that out.
There was a problem hiding this comment.
BTW, Sync vrs. Async really should be a property of the process not the debugger. It doesn't so much matter when there's one target/process per debugger, but if you have multiple processes, you can't guarantee that all of them want to be in either sync or async mode at the same time.
There was a problem hiding this comment.
And sync vrs. async is purely a matter of what we do with process events, so it could reasonably be a process property.
While debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes.
JDevlieghere
force-pushed
the
raii-sync-mode
branch
from
e545b97 to
c7c0c4c
Compare
ashgti
approved these changes
Apr 30, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16952 Here is the relevant piece of the build log for the reference |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
While debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
While debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
While debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
While debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes.
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
While debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes.
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.
While debugging the flakiness of the launch and attach tests, I noticed that we have some places in lldb-dap where we put the debugger in synchronous mode and have an early exit, that would leave the debugger in this state. This PR introduces an RAII helper to avoid such mistakes.