llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/137900.diff 6 Files Affected: - (modified) lldb/tools/lldb-dap/DAP.cpp (+3-3) - (modified) lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp (+6-4) - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.cpp (+4-4) - (modified) lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp (+2-2) - (modified) lldb/tools/lldb-dap/LLDBUtils.cpp (+11) - (modified) lldb/tools/lldb-dap/LLDBUtils.h (+10-1) ``````````diff 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); `````````` </details> https://github.com/llvm/llvm-project/pull/137900 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
