[Lldb-commits] [lldb] [lldb-dap] Fix raciness in launch and attach tests (PR #137920)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Wed Apr 30 21:00:55 PDT 2025
https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/137920
>From 1bdae983cdde60453c213a8086033c84dce3fb2d Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Wed, 30 Apr 2025 17:19:49 -0700
Subject: [PATCH 1/2] [lldb-dap] Fix raciness in launch and attach tests
#137920
---
.../test/tools/lldb-dap/lldbdap_testcase.py | 2 +-
.../tools/lldb-dap/attach/TestDAP_attach.py | 1 -
.../attach/TestDAP_attachByPortNum.py | 1 -
lldb/tools/lldb-dap/DAP.cpp | 4 ++-
lldb/tools/lldb-dap/DAP.h | 11 +++++++
.../lldb-dap/Handler/AttachRequestHandler.cpp | 33 +++++++++++++------
.../ConfigurationDoneRequestHandler.cpp | 18 ++++++++++
.../Handler/InitializeRequestHandler.cpp | 21 ++++++++----
.../tools/lldb-dap/Handler/RequestHandler.cpp | 21 +++++++-----
lldb/tools/lldb-dap/Handler/RequestHandler.h | 5 ++-
10 files changed, 88 insertions(+), 29 deletions(-)
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index ee5272850b9a8..0d81b7d80102f 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -103,7 +103,7 @@ def verify_breakpoint_hit(self, breakpoint_ids):
match_desc = "breakpoint %s." % (breakpoint_id)
if match_desc in description:
return
- self.assertTrue(False, "breakpoint not hit")
+ self.assertTrue(False, f"breakpoint not hit: stopped_events={stopped_events}")
def verify_stop_exception_info(self, expected_description, timeout=timeoutval):
"""Wait for the process we are debugging to stop, and verify the stop
diff --git a/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py b/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py
index 6f70316821c8c..dcdfada2ff4c2 100644
--- a/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py
+++ b/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py
@@ -25,7 +25,6 @@ def spawn_and_wait(program, delay):
process.wait()
- at skipIf
class TestDAP_attach(lldbdap_testcase.DAPTestCaseBase):
def set_and_hit_breakpoint(self, continueToExit=True):
source = "main.c"
diff --git a/lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py b/lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py
index 51f62b79f3f4f..152e504af6d14 100644
--- a/lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py
+++ b/lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py
@@ -19,7 +19,6 @@
import socket
- at skip
class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
default_timeout = 20
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 4cb0d8e49004c..e3140a1231b00 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -85,7 +85,9 @@ DAP::DAP(Log *log, const ReplMode default_repl_mode,
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),
+ configuration_done_sent(false),
+ first_stop_state(FirstStopState::NoStopEvent),
+ waiting_for_run_in_terminal(false),
progress_event_reporter(
[&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
reverse_request_seq(0), repl_mode(default_repl_mode) {
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 88eedb0860cf1..7a50b0f682fe3 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -189,6 +189,17 @@ struct DAP {
// the old process here so we can detect this case and keep running.
lldb::pid_t restarting_process_id;
bool configuration_done_sent;
+
+ enum class FirstStopState {
+ NoStopEvent,
+ PendingStopEvent,
+ IgnoredStopEvent,
+ };
+ std::mutex first_stop_mutex;
+ std::condition_variable first_stop_cv;
+ FirstStopState first_stop_state;
+
+ bool ignore_next_stop;
llvm::StringMap<std::unique_ptr<BaseRequestHandler>> request_handlers;
bool waiting_for_run_in_terminal;
ProgressEventReporter progress_event_reporter;
diff --git a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
index 7084d30f2625b..c6c1b86922fc2 100644
--- a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
@@ -158,8 +158,15 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
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);
+ // Connect remote will generate a stopped event even in synchronous
+ // mode.
+ {
+ std::lock_guard<std::mutex> guard(dap.first_stop_mutex);
+ dap.first_stop_state = DAP::FirstStopState::PendingStopEvent;
+ dap.target.ConnectRemote(listener, connect_url.c_str(), "gdb-remote",
+ error);
+ }
+ dap.first_stop_cv.notify_one();
} else {
// Attach by process name or id.
dap.target.Attach(attach_info, error);
@@ -168,15 +175,21 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
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;
+ {
+ std::lock_guard<std::mutex> guard(dap.first_stop_mutex);
+ dap.first_stop_state = DAP::FirstStopState::PendingStopEvent;
+ // 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;
+ }
}
+ dap.first_stop_cv.notify_one();
+
// 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();
diff --git a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
index f39bbdefdbb95..1395c75549215 100644
--- a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
@@ -51,6 +51,24 @@ void ConfigurationDoneRequestHandler::operator()(
FillResponse(request, response);
dap.SendJSON(llvm::json::Value(std::move(response)));
dap.configuration_done_sent = true;
+
+ {
+ // Temporary unlock the API mutex to avoid a deadlock between the API mutex
+ // and the first stop mutex.
+ lock.unlock();
+
+ // Block until we have either ignored the fist stop event or we didn't
+ // generate one because we attached or launched in synchronous mode.
+ std::unique_lock<std::mutex> stop_lock(dap.first_stop_mutex);
+ dap.first_stop_cv.wait(stop_lock, [&] {
+ return dap.first_stop_state == DAP::FirstStopState::NoStopEvent ||
+ dap.first_stop_state == DAP::FirstStopState::IgnoredStopEvent;
+ });
+
+ // Relock the API mutex.
+ lock.lock();
+ }
+
if (dap.stop_at_entry)
SendThreadStoppedEvent(dap);
else {
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index ce34c52bcc334..e4033d22822d5 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -167,14 +167,23 @@ static void EventThreadFunction(DAP &dap) {
// 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);
+ {
+ std::unique_lock<std::mutex> lock(dap.first_stop_mutex);
+ if (dap.first_stop_state ==
+ DAP::FirstStopState::PendingStopEvent) {
+ dap.first_stop_state = DAP::FirstStopState::IgnoredStopEvent;
+ lock.unlock();
+ dap.first_stop_cv.notify_one();
+ continue;
}
}
+
+ // 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:
dap.WillContinue();
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index 7a75cd93abc19..136d2ca6825c3 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -188,13 +188,12 @@ void BaseRequestHandler::Run(const Request &request) {
return;
}
- lldb::SBMutex lock = dap.GetAPIMutex();
- std::lock_guard<lldb::SBMutex> guard(lock);
-
// FIXME: After all the requests have migrated from LegacyRequestHandler >
// RequestHandler<> we should be able to move this into
// RequestHandler<>::operator().
+ lock.lock();
operator()(request);
+ lock.unlock();
// FIXME: After all the requests have migrated from LegacyRequestHandler >
// RequestHandler<> we should be able to check `debugger.InterruptRequest` and
@@ -250,11 +249,17 @@ llvm::Error BaseRequestHandler::LaunchProcess(
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;
+ {
+ std::lock_guard<std::mutex> guard(dap.first_stop_mutex);
+ dap.first_stop_state = DAP::FirstStopState::PendingStopEvent;
+
+ // 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;
+ }
+ dap.first_stop_cv.notify_all();
// The custom commands might have created a new target so we should use the
// selected target after these commands are run.
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index fa3d76ed4a125..f9ed5ac1cd6e9 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -37,7 +37,8 @@ struct DAP;
/// the RequestHandler template subclass instead.
class BaseRequestHandler {
public:
- BaseRequestHandler(DAP &dap) : dap(dap) {}
+ BaseRequestHandler(DAP &dap)
+ : dap(dap), mutex(dap.GetAPIMutex()), lock(mutex, std::defer_lock) {}
/// BaseRequestHandler are not copyable.
/// @{
@@ -80,6 +81,8 @@ class BaseRequestHandler {
/// @}
DAP &dap;
+ lldb::SBMutex mutex;
+ mutable std::unique_lock<lldb::SBMutex> lock;
};
/// FIXME: Migrate callers to typed RequestHandler for improved type handling.
>From c8f4f0aff8c1940ff0b8bdd3253b1930025c3e07 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Wed, 30 Apr 2025 20:45:25 -0700
Subject: [PATCH 2/2] Block before sending the response to the
configurationDone.
---
.../Handler/ConfigurationDoneRequestHandler.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
index 1395c75549215..850b9eb7a98a8 100644
--- a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
@@ -47,11 +47,6 @@ namespace lldb_dap {
void ConfigurationDoneRequestHandler::operator()(
const llvm::json::Object &request) const {
- llvm::json::Object response;
- FillResponse(request, response);
- dap.SendJSON(llvm::json::Value(std::move(response)));
- dap.configuration_done_sent = true;
-
{
// Temporary unlock the API mutex to avoid a deadlock between the API mutex
// and the first stop mutex.
@@ -69,6 +64,11 @@ void ConfigurationDoneRequestHandler::operator()(
lock.lock();
}
+ 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 {
More information about the lldb-commits
mailing list