[Lldb-commits] [lldb] [lldb-dap] Change the launch sequence (PR #138219)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Fri May 2 10:05:59 PDT 2025
https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/138219
>From e7333489e50b1efe16203e8fd7b6dc7f2dcd4439 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Fri, 2 May 2025 09:59:01 -0700
Subject: [PATCH] [lldb-dap] Change the launch sequence
This PR changes how we treat the launch sequence in lldb-dap.
- Send the initialized event after we finish handling the initialize
request, rather than after we finish attaching or launching.
- Delay handling the launch and attach request until we have handled
the configurationDone request. The latter is now largely a NO-OP and
only exists to signal lldb-dap that it can handle the launch and
attach requests.
- Delay handling the initial threads requests until we have handled
the launch or attach request.
- Make all attaching and launching asynchronous, including when we have
attach or launch commands. That removes the need to synchronize
between the request and event thread.
Background: https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125
---
lldb/tools/lldb-dap/DAP.cpp | 53 +++++++---
lldb/tools/lldb-dap/DAP.h | 6 +-
lldb/tools/lldb-dap/EventHelper.cpp | 2 +-
.../lldb-dap/Handler/AttachRequestHandler.cpp | 98 ++++++++++---------
.../ConfigurationDoneRequestHandler.cpp | 14 +--
.../Handler/InitializeRequestHandler.cpp | 44 ++++-----
.../lldb-dap/Handler/LaunchRequestHandler.cpp | 2 -
.../tools/lldb-dap/Handler/RequestHandler.cpp | 69 ++++++++-----
lldb/tools/lldb-dap/Handler/RequestHandler.h | 1 +
9 files changed, 163 insertions(+), 126 deletions(-)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 4cb0d8e49004c..470cde5ae69b0 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -84,8 +84,8 @@ DAP::DAP(Log *log, const ReplMode default_repl_mode,
: log(log), transport(transport), broadcaster("lldb-dap"),
exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID),
stop_at_entry(false), is_attach(false),
- restarting_process_id(LLDB_INVALID_PROCESS_ID),
- configuration_done_sent(false), waiting_for_run_in_terminal(false),
+ restarting_process_id(LLDB_INVALID_PROCESS_ID), configuration_done(false),
+ waiting_for_run_in_terminal(false),
progress_event_reporter(
[&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
reverse_request_seq(0), repl_mode(default_repl_mode) {
@@ -893,10 +893,23 @@ llvm::Error DAP::Loop() {
return errWrapper;
}
+ // The launch sequence is special and we need to carefully handle
+ // packets in the right order. The launch and attach requests cannot
+ // be answered until we've gotten the confgigurationDone request. We
+ // can't answer the threads request until we've successfully launched
+ // or attached.
+ bool is_part_of_launch_sequence = false;
+
if (const protocol::Request *req =
- std::get_if<protocol::Request>(&*next);
- req && req->command == "disconnect") {
- disconnecting = true;
+ std::get_if<protocol::Request>(&*next)) {
+ if (req->command == "disconnect")
+ disconnecting = true;
+ if (!configuration_done) {
+ if (req->command == "launch" || req->command == "attach")
+ is_part_of_launch_sequence = true;
+ if (req->command == "threads" && !m_launch_sequence_queue.empty())
+ is_part_of_launch_sequence = true;
+ }
}
const std::optional<CancelArguments> cancel_args =
@@ -924,7 +937,10 @@ llvm::Error DAP::Loop() {
{
std::lock_guard<std::mutex> guard(m_queue_mutex);
- m_queue.push_back(std::move(*next));
+ if (is_part_of_launch_sequence)
+ m_launch_sequence_queue.push_back(std::move(*next));
+ else
+ m_queue.push_back(std::move(*next));
}
m_queue_cv.notify_one();
}
@@ -938,15 +954,30 @@ llvm::Error DAP::Loop() {
StopEventHandlers();
});
- while (!disconnecting || !m_queue.empty()) {
+ while (true) {
std::unique_lock<std::mutex> lock(m_queue_mutex);
- m_queue_cv.wait(lock, [&] { return disconnecting || !m_queue.empty(); });
+ m_queue_cv.wait(lock, [&] {
+ return disconnecting || !m_queue.empty() ||
+ (!m_launch_sequence_queue.empty() && configuration_done);
+ });
- if (m_queue.empty())
+ if (disconnecting)
break;
- Message next = m_queue.front();
- m_queue.pop_front();
+ bool is_launch_seqeuence =
+ !m_launch_sequence_queue.empty() && configuration_done;
+
+ auto &active_queue =
+ is_launch_seqeuence ? m_launch_sequence_queue : m_queue;
+
+ assert(!active_queue.empty() &&
+ "shouldn't have gotten past the wait with an empty queue");
+
+ Message next = active_queue.front();
+ active_queue.pop_front();
+
+ // Unlock while we're processing the event.
+ lock.unlock();
if (!HandleObject(next))
return llvm::createStringError(llvm::inconvertibleErrorCode(),
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 88eedb0860cf1..4f038b66757c8 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -188,7 +188,7 @@ struct DAP {
// shutting down the entire adapter. When we're restarting, we keep the id of
// the old process here so we can detect this case and keep running.
lldb::pid_t restarting_process_id;
- bool configuration_done_sent;
+ bool configuration_done;
llvm::StringMap<std::unique_ptr<BaseRequestHandler>> request_handlers;
bool waiting_for_run_in_terminal;
ProgressEventReporter progress_event_reporter;
@@ -417,8 +417,10 @@ struct DAP {
lldb::SBMutex GetAPIMutex() const { return target.GetAPIMutex(); }
private:
- std::mutex m_queue_mutex;
+ /// Queue for all incoming messages.
std::deque<protocol::Message> m_queue;
+ std::deque<protocol::Message> m_launch_sequence_queue;
+ std::mutex m_queue_mutex;
std::condition_variable m_queue_cv;
std::mutex m_cancelled_requests_mutex;
diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp
index 2c659f39f4b66..ed2d8700c26b0 100644
--- a/lldb/tools/lldb-dap/EventHelper.cpp
+++ b/lldb/tools/lldb-dap/EventHelper.cpp
@@ -222,7 +222,7 @@ void SendContinuedEvent(DAP &dap) {
// If the focus thread is not set then we haven't reported any thread status
// to the client, so nothing to report.
- if (!dap.configuration_done_sent || dap.focus_tid == LLDB_INVALID_THREAD_ID) {
+ if (!dap.configuration_done || dap.focus_tid == LLDB_INVALID_THREAD_ID) {
return;
}
diff --git a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
index 7084d30f2625b..26bd46e587c6d 100644
--- a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
@@ -137,55 +137,67 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
dap.SendOutput(OutputType::Console,
llvm::StringRef(attach_msg, attach_msg_len));
}
- 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
+ {
+ // Perform the launch in synchronous mode so that we don't have to worry
+ // about process state changes during the launch.
ScopeSyncMode scope_sync_mode(dap.debugger);
-
- if (core_file.empty()) {
- if ((pid != LLDB_INVALID_PROCESS_ID) &&
- (gdb_remote_port != invalid_port)) {
- // If both pid and port numbers are specified.
- error.SetErrorString("The user can't specify both pid and port");
- } else if (gdb_remote_port != invalid_port) {
- // If port is specified and pid is not.
- lldb::SBListener listener = dap.debugger.GetListener();
-
- // If the user hasn't provided the hostname property, default localhost
- // being used.
- std::string connect_url =
- llvm::formatv("connect://{0}:", gdb_remote_hostname);
- connect_url += std::to_string(gdb_remote_port);
- dap.target.ConnectRemote(listener, connect_url.c_str(), "gdb-remote",
- error);
+ if (attachCommands.empty()) {
+ // No "attachCommands", just attach normally.
+ if (core_file.empty()) {
+ if ((pid != LLDB_INVALID_PROCESS_ID) &&
+ (gdb_remote_port != invalid_port)) {
+ // If both pid and port numbers are specified.
+ error.SetErrorString("The user can't specify both pid and port");
+ } else if (gdb_remote_port != invalid_port) {
+ // If port is specified and pid is not.
+ lldb::SBListener listener = dap.debugger.GetListener();
+
+ // If the user hasn't provided the hostname property, default
+ // localhost being used.
+ std::string connect_url =
+ llvm::formatv("connect://{0}:", gdb_remote_hostname);
+ connect_url += std::to_string(gdb_remote_port);
+ dap.target.ConnectRemote(listener, connect_url.c_str(), "gdb-remote",
+ error);
+ } else {
+ // Attach by process name or id.
+ dap.target.Attach(attach_info, error);
+ }
} else {
- // Attach by process name or id.
- dap.target.Attach(attach_info, error);
+ dap.target.LoadCore(core_file.data(), error);
}
} else {
- dap.target.LoadCore(core_file.data(), error);
- }
- } 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
- // is no valid process after running these commands, we have failed.
- if (llvm::Error err = dap.RunAttachCommands(attachCommands)) {
- response["success"] = false;
- EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
+ // 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 is no valid process after running these commands, we have failed.
+ if (llvm::Error err = dap.RunAttachCommands(attachCommands)) {
+ response["success"] = false;
+ EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
+ dap.SendJSON(llvm::json::Value(std::move(response)));
+ return;
+ }
+ // The custom commands might have created a new target so we should use
+ // the selected target after these commands are run.
+ dap.target = dap.debugger.GetSelectedTarget();
}
- // The custom commands might have created a new target so we should use the
- // selected target after these commands are run.
- dap.target = dap.debugger.GetSelectedTarget();
-
- // Make sure the process is attached and stopped before proceeding as the
- // the launch commands are not run using the synchronous mode.
- error = dap.WaitForProcessToStop(std::chrono::seconds(timeout_seconds));
}
+ // Make sure the process is attached and stopped.
+ error = dap.WaitForProcessToStop(std::chrono::seconds(timeout_seconds));
+
+ // Clients can request a baseline of currently existing threads after
+ // we acknowledge the configurationDone request.
+ // Client requests the baseline of currently existing threads after
+ // a successful or attach by sending a 'threads' request
+ // right after receiving the configurationDone response.
+ // Obtain the list of threads before we resume the process
+ dap.initial_thread_list =
+ GetThreads(dap.target.GetProcess(), dap.thread_format);
+
+ if (!dap.stop_at_entry)
+ dap.target.GetProcess().Continue();
+
if (error.Success() && core_file.empty()) {
auto attached_pid = dap.target.GetProcess().GetProcessID();
if (attached_pid == LLDB_INVALID_PROCESS_ID) {
@@ -204,10 +216,8 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
}
dap.SendJSON(llvm::json::Value(std::move(response)));
- if (error.Success()) {
+ if (error.Success())
SendProcessEvent(dap, Attach);
- dap.SendJSON(CreateEventObject("initialized"));
- }
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
index f39bbdefdbb95..a10cd0c832782 100644
--- a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
@@ -47,21 +47,11 @@ namespace lldb_dap {
void ConfigurationDoneRequestHandler::operator()(
const llvm::json::Object &request) const {
+ dap.configuration_done = true;
+
llvm::json::Object response;
FillResponse(request, response);
dap.SendJSON(llvm::json::Value(std::move(response)));
- dap.configuration_done_sent = true;
- if (dap.stop_at_entry)
- SendThreadStoppedEvent(dap);
- else {
- // Client requests the baseline of currently existing threads after
- // a successful launch or attach by sending a 'threads' request
- // right after receiving the configurationDone response.
- // Obtain the list of threads before we resume the process
- dap.initial_thread_list =
- GetThreads(dap.target.GetProcess(), dap.thread_format);
- dap.target.GetProcess().Continue();
- }
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index ce34c52bcc334..aa947d3cb5ab9 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -140,43 +140,28 @@ static void EventThreadFunction(DAP &dap) {
lldb::SBProcess process = lldb::SBProcess::GetProcessFromEvent(event);
if (event_mask & lldb::SBProcess::eBroadcastBitStateChanged) {
auto state = lldb::SBProcess::GetStateFromEvent(event);
+
+ DAP_LOG(dap.log, "State = {0}", state);
switch (state) {
+ case lldb::eStateConnected:
+ case lldb::eStateDetached:
case lldb::eStateInvalid:
- // Not a state event
- break;
case lldb::eStateUnloaded:
break;
- case lldb::eStateConnected:
- break;
case lldb::eStateAttaching:
- break;
- case lldb::eStateLaunching:
- break;
- case lldb::eStateStepping:
- break;
case lldb::eStateCrashed:
- break;
- case lldb::eStateDetached:
- break;
- case lldb::eStateSuspended:
- break;
+ case lldb::eStateLaunching:
case lldb::eStateStopped:
- // We launch and attach in synchronous mode then the first stop
- // event will not be delivered. If we use "launchCommands" during a
- // launch or "attachCommands" during an attach we might some process
- // stop events which we do not want to send an event for. We will
- // manually send a stopped event in request_configurationDone(...)
- // so don't send any before then.
- if (dap.configuration_done_sent) {
- // Only report a stopped event if the process was not
- // automatically restarted.
- if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
- SendStdOutStdErr(dap, process);
- SendThreadStoppedEvent(dap);
- }
+ case lldb::eStateSuspended:
+ // Only report a stopped event if the process was not
+ // automatically restarted.
+ if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
+ SendStdOutStdErr(dap, process);
+ SendThreadStoppedEvent(dap);
}
break;
case lldb::eStateRunning:
+ case lldb::eStateStepping:
dap.WillContinue();
SendContinuedEvent(dap);
break;
@@ -284,6 +269,7 @@ llvm::Expected<InitializeResponseBody> InitializeRequestHandler::Run(
// Do not source init files until in/out/err are configured.
dap.debugger = lldb::SBDebugger::Create(false);
dap.debugger.SetInputFile(dap.in);
+ dap.target = dap.debugger.GetDummyTarget();
llvm::Expected<int> out_fd = dap.out.GetWriteFileDescriptor();
if (!out_fd)
@@ -338,4 +324,8 @@ llvm::Expected<InitializeResponseBody> InitializeRequestHandler::Run(
return dap.GetCapabilities();
}
+void InitializeRequestHandler::PostRun() const {
+ dap.SendJSON(CreateEventObject("initialized"));
+}
+
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp
index 3e4532e754ec6..ffd61b040e55f 100644
--- a/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp
@@ -72,8 +72,6 @@ void LaunchRequestHandler::PostRun() const {
// Attach happens when launching with runInTerminal.
SendProcessEvent(dap, dap.is_attach ? Attach : Launch);
}
-
- dap.SendJSON(CreateEventObject("initialized"));
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index 7a75cd93abc19..82c6e1b0071a9 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -162,7 +162,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
dap.target.GetProcess().Continue();
// Now that the actual target is just starting (i.e. exec was just invoked),
- // we return the debugger to its async state.
+ // we return the debugger to its sync state.
scope_sync_mode.reset();
// If sending the notification failed, the launcher should be dead by now and
@@ -238,35 +238,50 @@ llvm::Error BaseRequestHandler::LaunchProcess(
launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
lldb::eLaunchFlagStopAtEntry);
- if (arguments.runInTerminal) {
- if (llvm::Error err = RunInTerminal(dap, arguments))
- return err;
- } else if (launchCommands.empty()) {
- 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
+ {
+ // Perform the launch in synchronous mode so that we don't have to worry
+ // about process state changes during the launch.
ScopeSyncMode scope_sync_mode(dap.debugger);
- dap.target.Launch(launch_info, error);
- if (error.Fail())
- return llvm::make_error<DAPError>(error.GetCString());
- } else {
- // Set the launch info so that run commands can access the configured
- // launch details.
- dap.target.SetLaunchInfo(launch_info);
- if (llvm::Error err = dap.RunLaunchCommands(launchCommands))
- return err;
-
- // The custom commands might have created a new target so we should use the
- // selected target after these commands are run.
- dap.target = dap.debugger.GetSelectedTarget();
- // Make sure the process is launched and stopped at the entry point before
- // proceeding as the launch commands are not run using the synchronous
- // mode.
- lldb::SBError error = dap.WaitForProcessToStop(arguments.timeout);
- if (error.Fail())
- return llvm::make_error<DAPError>(error.GetCString());
+
+ if (arguments.runInTerminal) {
+ if (llvm::Error err = RunInTerminal(dap, arguments))
+ return err;
+ } else if (launchCommands.empty()) {
+ lldb::SBError error;
+ dap.target.Launch(launch_info, error);
+ if (error.Fail())
+ return llvm::make_error<DAPError>(error.GetCString());
+ } else {
+ // Set the launch info so that run commands can access the configured
+ // launch details.
+ dap.target.SetLaunchInfo(launch_info);
+ if (llvm::Error err = dap.RunLaunchCommands(launchCommands))
+ return err;
+
+ // The custom commands might have created a new target so we should use
+ // the selected target after these commands are run.
+ dap.target = dap.debugger.GetSelectedTarget();
+ }
}
+ // Make sure the process is launched and stopped at the entry point before
+ // proceeding.
+ lldb::SBError error = dap.WaitForProcessToStop(arguments.timeout);
+ if (error.Fail())
+ return llvm::make_error<DAPError>(error.GetCString());
+
+ // Clients can request a baseline of currently existing threads after
+ // we acknowledge the configurationDone request.
+ // Client requests the baseline of currently existing threads after
+ // a successful or attach by sending a 'threads' request
+ // right after receiving the configurationDone response.
+ // Obtain the list of threads before we resume the process
+ dap.initial_thread_list =
+ GetThreads(dap.target.GetProcess(), dap.thread_format);
+
+ if (!dap.stop_at_entry)
+ dap.target.GetProcess().Continue();
+
return llvm::Error::success();
}
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index fa3d76ed4a125..dec35c5dce1e5 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -282,6 +282,7 @@ class InitializeRequestHandler
static llvm::StringLiteral GetCommand() { return "initialize"; }
llvm::Expected<protocol::InitializeResponseBody>
Run(const protocol::InitializeRequestArguments &args) const override;
+ void PostRun() const override;
};
class LaunchRequestHandler
More information about the lldb-commits
mailing list