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 <jo...@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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to