[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 received
   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.
 - Add synchronization for ignoring the first stop event. I originally
   wanted to do all the launching and attaching in asynchronous mode,
   but that's a little bit trickier than expected. On macOS, I'm getting
   an additional stop when we hit the dyld breakpoint. This might be
   something I come back to, but for now I'm keeping the old behavior
   with the proposed synchronization from #137920.

Background: https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125

Author: JDevlieghere

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) {
@@ -892,11 +892,14 @@ llvm::Error DAP::Loop() {
             errWrapper.SetErrorString(llvm::toString(std::move(err)).c_str());
             return errWrapper;
           }
+          bool is_launch_attach = 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 (req->command == "launch" || req->command == "attach")
+              is_launch_attach = true;
           }
 
           const std::optional<CancelArguments> cancel_args =
@@ -924,7 +927,10 @@ llvm::Error DAP::Loop() {
 
           {
             std::lock_guard<std::mutex> guard(m_queue_mutex);
-            m_queue.push_back(std::move(*next));
+            if (is_launch_attach)
+              m_launch_attach_queue.push_back(std::move(*next));
+            else
+              m_queue.push_back(std::move(*next));
           }
           m_queue_cv.notify_one();
         }
@@ -938,15 +944,24 @@ 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() ||
+             (configuration_done && !m_launch_attach_queue.empty());
+    });
 
-    if (m_queue.empty())
+    if (disconnecting)
       break;
 
-    Message next = m_queue.front();
-    m_queue.pop_front();
+    auto &active_queue = configuration_done && !m_launch_attach_queue.empty()
+                             ? m_launch_attach_queue
+                             : m_queue;
+
+    assert(!active_queue.empty());
+
+    Message next = active_queue.front();
+    active_queue.pop_front();
 
     if (!HandleObject(next))
       return llvm::createStringError(llvm::inconvertibleErrorCode(),

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;
@@ -214,6 +214,13 @@ struct DAP {
   /// The initial thread list upon attaching.
   std::optional<llvm::json::Array> initial_thread_list;
 
+  /// Synchronization of stop events between the event and request thread.
+  /// @{
+  std::mutex ignore_stop_mutex;
+  std::condition_variable ignore_stop_cv;
+  bool ignore_next_stop;
+  /// @}
+
   /// Creates a new DAP sessions.
   ///
   /// \param[in] log
@@ -417,8 +424,11 @@ 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;
+  /// Dedicated queue for launching and attaching.
+  std::deque<protocol::Message> m_launch_attach_queue;
+  std::mutex m_queue_mutex;
   std::condition_variable m_queue_cv;
 
   std::mutex m_cancelled_requests_mutex;

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;
   }
 

lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp

@@ -128,6 +128,9 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
     return;
   }
 
+  std::unique_lock<std::mutex> lock(dap.ignore_stop_mutex);
+  dap.ignore_next_stop = !dap.stop_at_entry;
+
   if ((pid == LLDB_INVALID_PROCESS_ID || gdb_remote_port == invalid_port) &&
       wait_for) {
     char attach_msg[256];
@@ -181,9 +184,24 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
     // 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));
+  }
+
+  // Notify the event thread that it should ignore the next stop event.
+  lock.unlock();
+  dap.ignore_stop_cv.notify_one();
+
+  // Make sure the process is attached and stopped before proceeding.
+  error = dap.WaitForProcessToStop(std::chrono::seconds(timeout_seconds));
+
+  // FIXME: Unify this between launch & attach.
+  if (dap.stop_at_entry) {
+    // 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();
   }
 
   if (error.Success() && core_file.empty()) {
@@ -204,10 +222,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

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

lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp

@@ -161,20 +161,22 @@ static void EventThreadFunction(DAP &dap) {
           case lldb::eStateSuspended:
             break;
           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);
+            {
+              std::unique_lock<std::mutex> lock(dap.ignore_stop_mutex);
+              if (dap.ignore_next_stop) {
+                DAP_LOG(dap.log, "Ignoring process stop event");
                 SendThreadStoppedEvent(dap);
+                dap.ignore_next_stop = false;
+                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();
@@ -284,6 +286,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 +341,8 @@ llvm::Expected<InitializeResponseBody> InitializeRequestHandler::Run(
   return dap.GetCapabilities();
 }
 
+void InitializeRequestHandler::PostRun() const {
+  dap.SendJSON(CreateEventObject("initialized"));
+}
+
 } // namespace lldb_dap

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

lldb/tools/lldb-dap/Handler/RequestHandler.cpp

@@ -238,6 +238,9 @@ llvm::Error BaseRequestHandler::LaunchProcess(
   launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
                              lldb::eLaunchFlagStopAtEntry);
 
+  std::unique_lock<std::mutex> lock(dap.ignore_stop_mutex);
+  dap.ignore_next_stop = !dap.stop_at_entry;
+
   if (arguments.runInTerminal) {
     if (llvm::Error err = RunInTerminal(dap, arguments))
       return err;
@@ -259,12 +262,27 @@ llvm::Error BaseRequestHandler::LaunchProcess(
     // 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());
+  }
+
+  // Notify the event thread that it should ignore the next stop event.
+  lock.unlock();
+  dap.ignore_stop_cv.notify_one();
+
+  // 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());
+
+  // FIXME: Unify this between launch & attach.
+  if (!dap.stop_at_entry) {
+    // 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();
   }
 
   return llvm::Error::success();

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