[lldb-dap] Adjusting startup flow to better handle async operations.

lldb/tools/lldb-dap/DAP.cpp

@@ -120,11 +120,12 @@ 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(false),
+      restarting_process_id(LLDB_INVALID_PROCESS_ID),
       waiting_for_run_in_terminal(false),
       progress_event_reporter(
           [&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
-      reverse_request_seq(0), repl_mode(default_repl_mode) {
+      reverse_request_seq(0), repl_mode(default_repl_mode),
+      m_configuration_done(false) {
   configuration.preInitCommands = std::move(pre_init_commands);
   RegisterRequests();
 }
@@ -916,20 +917,10 @@ llvm::Error DAP::Loop() {
             return errWrapper;
           }
 
-          // The launch sequence is special and we need to carefully handle
-          // packets in the right order. Until we've handled configurationDone,
-          bool add_to_pending_queue = false;
-
           if (const protocol::Request *req =
-                  std::get_if<protocol::Request>(&*next)) {
-            llvm::StringRef command = req->command;
-            if (command == "disconnect")
-              disconnecting = true;
-            if (!configuration_done)
-              add_to_pending_queue =
-                  command != "initialize" && command != "configurationDone" &&
-                  command != "disconnect" && !command.ends_with("Breakpoints");
-          }
+                  std::get_if<protocol::Request>(&*next);
+              req && req->command == "disconnect")
+            disconnecting = true;
 
           const std::optional<CancelArguments> cancel_args =
               getArgumentsIfRequest<CancelArguments>(*next, "cancel");
@@ -956,8 +947,7 @@ llvm::Error DAP::Loop() {
 
           {
             std::lock_guard<std::mutex> guard(m_queue_mutex);
-            auto &queue = add_to_pending_queue ? m_pending_queue : m_queue;
-            queue.push_back(std::move(*next));
+            m_queue.push_back(std::move(*next));
           }
           m_queue_cv.notify_one();
         }
@@ -1255,14 +1245,25 @@ void DAP::SetConfiguration(const protocol::Configuration &config,
     SetThreadFormat(*configuration.customThreadFormat);
 }
 
+bool DAP::GetConfigurationDone() {
+  std::lock_guard<std::mutex> guard(m_configuration_done_mutex);
+  return m_configuration_done;
+}
+
 void DAP::SetConfigurationDone() {
-  {
-    std::lock_guard<std::mutex> guard(m_queue_mutex);
-    std::copy(m_pending_queue.begin(), m_pending_queue.end(),
-              std::front_inserter(m_queue));
-    configuration_done = true;
-  }
-  m_queue_cv.notify_all();
+  std::lock_guard<std::mutex> guard(m_configuration_done_mutex);
+  m_configuration_done = true;
+  for (auto &cb : m_configuration_done_callbacks)
+    cb();
+  m_configuration_done_callbacks.clear();
+}
+
+void DAP::OnConfigurationDone(llvm::unique_function<void()> &&callback) {
+  std::lock_guard<std::mutex> guard(m_configuration_done_mutex);
+  if (m_configuration_done)
+    callback();
+  else
+    m_configuration_done_callbacks.emplace_back(std::move(callback));
 }
 
 void DAP::SetFrameFormat(llvm::StringRef format) {

lldb/tools/lldb-dap/DAP.h

@@ -187,7 +187,6 @@ 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;
   bool waiting_for_run_in_terminal;
   ProgressEventReporter progress_event_reporter;
   // Keep track of the last stop thread index IDs as threads won't go away
@@ -257,14 +256,26 @@ struct DAP {
   /// Configures the debug adapter for launching/attaching.
   void SetConfiguration(const protocol::Configuration &confing, bool is_attach);
 
+  /// Returns true if the debug session has received the `configurationDone`
+  /// request.
+  bool GetConfigurationDone();
+
+  /// Marks that the debug session has received the `configurationDone` request.
   void SetConfigurationDone();
 
+  /// Registers a callback that is fired after `configurationDone` is received.
+  ///
+  /// NOTE: If `configurationDone` has already been received, the callback will
+  /// be invoked immediately.
+  void OnConfigurationDone(llvm::unique_function<void()> &&callback);
+
   /// Configure source maps based on the current `DAPConfiguration`.
   void ConfigureSourceMaps();
 
   /// Serialize the JSON value into a string and send the JSON packet to the
   /// "out" stream.
   void SendJSON(const llvm::json::Value &json);
+
   /// Send the given message to the client
   void Send(const protocol::Message &message);
 
@@ -336,7 +347,7 @@ struct DAP {
   lldb::SBTarget CreateTarget(lldb::SBError &error);
 
   /// Set given target object as a current target for lldb-dap and start
-  /// listeing for its breakpoint events.
+  /// listening for its breakpoint events.
   void SetTarget(const lldb::SBTarget target);
 
   bool HandleObject(const protocol::Message &M);
@@ -377,7 +388,7 @@ struct DAP {
     });
   }
 
-  /// The set of capablities supported by this adapter.
+  /// The set of capabilities supported by this adapter.
   protocol::Capabilities GetCapabilities();
 
   /// Debuggee will continue from stopped state.
@@ -444,13 +455,16 @@ struct DAP {
 
   /// Queue for all incoming messages.
   std::deque<protocol::Message> m_queue;
-  std::deque<protocol::Message> m_pending_queue;
   std::mutex m_queue_mutex;
   std::condition_variable m_queue_cv;
 
   std::mutex m_cancelled_requests_mutex;
   llvm::SmallSet<int64_t, 4> m_cancelled_requests;
 
+  std::mutex m_configuration_done_mutex;
+  bool m_configuration_done;
+  std::vector<llvm::unique_function<void()>> m_configuration_done_callbacks;
+
   std::mutex m_active_request_mutex;
   const protocol::Request *m_active_request;
 };

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 || dap.focus_tid == LLDB_INVALID_THREAD_ID) {
+  if (!dap.GetConfigurationDone() || dap.focus_tid == LLDB_INVALID_THREAD_ID) {
     return;
   }
 

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

@@ -28,21 +28,22 @@ namespace lldb_dap {
 ///
 /// Since attaching is debugger/runtime specific, the arguments for this request
 /// are not part of this specification.
-Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
+void AttachRequestHandler::Run(const AttachRequestArguments &args,
+                               Reply<AttachResponse> reply) const {
   // Validate that we have a well formed attach request.
   if (args.attachCommands.empty() && args.coreFile.empty() &&
       args.configuration.program.empty() &&
       args.pid == LLDB_INVALID_PROCESS_ID &&
       args.gdbRemotePort == LLDB_DAP_INVALID_PORT)
-    return make_error<DAPError>(
+    return reply(make_error<DAPError>(
         "expected one of 'pid', 'program', 'attachCommands', "
-        "'coreFile' or 'gdb-remote-port' to be specified");
+        "'coreFile' or 'gdb-remote-port' to be specified"));
 
   // Check if we have mutually exclusive arguments.
   if ((args.pid != LLDB_INVALID_PROCESS_ID) &&
       (args.gdbRemotePort != LLDB_DAP_INVALID_PORT))
-    return make_error<DAPError>(
-        "'pid' and 'gdb-remote-port' are mutually exclusive");
+    return reply(make_error<DAPError>(
+        "'pid' and 'gdb-remote-port' are mutually exclusive"));
 
   dap.SetConfiguration(args.configuration, /*is_attach=*/true);
   if (!args.coreFile.empty())
@@ -59,20 +60,20 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
 
   // Run any initialize LLDB commands the user specified in the launch.json
   if (llvm::Error err = dap.RunInitCommands())
-    return err;
+    return reply(std::move(err));
 
   dap.ConfigureSourceMaps();
 
   lldb::SBError error;
   lldb::SBTarget target = dap.CreateTarget(error);
   if (error.Fail())
-    return ToError(error);
+    return reply(ToError(error));
 
   dap.SetTarget(target);
 
   // Run any pre run LLDB commands the user specified in the launch.json
   if (Error err = dap.RunPreRunCommands())
-    return err;
+    return reply(std::move(err));
 
   if ((args.pid == LLDB_INVALID_PROCESS_ID ||
        args.gdbRemotePort == LLDB_DAP_INVALID_PORT) &&
@@ -94,14 +95,16 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
       // user that their command failed or the debugger is in an unexpected
       // state.
       if (llvm::Error err = dap.RunAttachCommands(args.attachCommands))
-        return err;
+        return reply(std::move(err));
 
-      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.SetTarget(dap.debugger.GetSelectedTarget());
 
       // Validate the attachCommand results.
       if (!dap.target.GetProcess().IsValid())
-        return make_error<DAPError>(
-            "attachCommands failed to attach to a process");
+        return reply(make_error<DAPError>(
+            "attachCommands failed to attach to a process"));
     } else if (!args.coreFile.empty()) {
       dap.target.LoadCore(args.coreFile.data(), error);
     } else if (args.gdbRemotePort != LLDB_DAP_INVALID_PORT) {
@@ -129,35 +132,36 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
   // Make sure the process is attached and stopped.
   error = dap.WaitForProcessToStop(args.configuration.timeout);
   if (error.Fail())
-    return ToError(error);
+    return reply(ToError(error));
 
   if (args.coreFile.empty() && !dap.target.GetProcess().IsValid())
-    return make_error<DAPError>("failed to attach to process");
+    return reply(make_error<DAPError>("failed to attach to process"));
 
   dap.RunPostRunCommands();
 
-  return Error::success();
-}
-
-void AttachRequestHandler::PostRun() const {
-  if (!dap.target.GetProcess().IsValid())
-    return;
-
-  // 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);
-
-  SendProcessEvent(dap, Attach);
-
-  if (dap.stop_at_entry)
-    SendThreadStoppedEvent(dap);
-  else
-    dap.target.GetProcess().Continue();
+  dap.OnConfigurationDone([&, reply = std::move(reply)]() {
+    // Ensure we have a valid process still, otherwise a run command may have
+    // left us in a bad state.
+    if (!dap.target.GetProcess().IsValid())
+      return reply(make_error<DAPError>("invalid process"));
+    reply(Error::success());
+
+    // 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);
+
+    SendProcessEvent(dap, Attach);
+
+    if (dap.stop_at_entry)
+      SendThreadStoppedEvent(dap);
+    else
+      dap.target.GetProcess().Continue();
+  });
 }
 
 } // namespace lldb_dap

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

@@ -21,11 +21,12 @@ using namespace lldb_dap::protocol;
 namespace lldb_dap {
 
 /// Launch request; value of command field is 'launch'.
-Error LaunchRequestHandler::Run(const LaunchRequestArguments &arguments) const {
+void LaunchRequestHandler::Run(const LaunchRequestArguments &arguments,
+                               Reply<LaunchResponse> reply) const {
   // Validate that we have a well formed launch request.
   if (!arguments.launchCommands.empty() && arguments.runInTerminal)
-    return make_error<DAPError>(
-        "'launchCommands' and 'runInTerminal' are mutually exclusive");
+    return reply(make_error<DAPError>(
+        "'launchCommands' and 'runInTerminal' are mutually exclusive"));
 
   dap.SetConfiguration(arguments.configuration, /*is_attach=*/false);
   dap.last_launch_request = arguments;
@@ -43,49 +44,50 @@ Error LaunchRequestHandler::Run(const LaunchRequestArguments &arguments) const {
   // This is run before target is created, so commands can't do anything with
   // the targets - preRunCommands are run with the target.
   if (Error err = dap.RunInitCommands())
-    return err;
+    return reply(std::move(err));
 
   dap.ConfigureSourceMaps();
 
   lldb::SBError error;
   lldb::SBTarget target = dap.CreateTarget(error);
   if (error.Fail())
-    return ToError(error);
+    return reply(ToError(error));
 
   dap.SetTarget(target);
 
   // Run any pre run LLDB commands the user specified in the launch.json
   if (Error err = dap.RunPreRunCommands())
-    return err;
+    return reply(std::move(err));
 
   if (Error err = LaunchProcess(arguments))
-    return err;
+    return reply(std::move(err));
 
   dap.RunPostRunCommands();
 
-  return Error::success();
-}
-
-void LaunchRequestHandler::PostRun() const {
-  if (!dap.target.GetProcess().IsValid())
-    return;
-
-  // 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);
-
-  // Attach happens when launching with runInTerminal.
-  SendProcessEvent(dap, dap.is_attach ? Attach : Launch);
-
-  if (dap.stop_at_entry)
-    SendThreadStoppedEvent(dap);
-  else
-    dap.target.GetProcess().Continue();
+  dap.OnConfigurationDone([&, reply = std::move(reply)]() {
+    // Ensure we have a valid process still, otherwise a run command may have
+    // left us in a bad state.
+    if (!dap.target.GetProcess().IsValid())
+      return reply(make_error<DAPError>("invalid process"));
+    reply(Error::success());
+
+    // 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);
+
+    // Attach happens when launching with runInTerminal.
+    SendProcessEvent(dap, dap.is_attach ? Attach : Launch);
+
+    if (dap.stop_at_entry)
+      SendThreadStoppedEvent(dap);
+    else
+      dap.target.GetProcess().Continue();
+  });
 }
 
 } // namespace lldb_dap