[lldb-dap] Fix raciness in launch and attach tests #137920

Author: JDevlieghere

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

lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py

@@ -25,7 +25,6 @@ def spawn_and_wait(program, delay):
     process.wait()
 
 
-@skipIf
 class TestDAP_attach(lldbdap_testcase.DAPTestCaseBase):
     def set_and_hit_breakpoint(self, continueToExit=True):
         source = "main.c"

lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py

@@ -19,7 +19,6 @@
 import socket
 
 
-@skip
 class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
     default_timeout = 20
 

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) {

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;

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();

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 {

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();

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.

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.