[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

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) {
@@ -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(),

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;

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

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

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

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

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