Moving Event Thread out of DAP

Author: qxy11

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

@@ -76,13 +76,37 @@ def test_by_name_waitFor(self):
         self.attach(program=program, waitFor=True)
         self.continue_and_verify_pid()
 
-    def test_attach_with_invalid_targetId(self):
+    def test_attach_with_missing_debuggerId_or_targetId(self):
         """
-        Test that attaching with an invalid targetId fails with the expected
-        error message.
+        Test that attaching with only one of debuggerId/targetId specified
+        fails with the expected error message.
         """
         self.build_and_create_debug_adapter()
 
+        # Test with only targetId specified (no debuggerId)
         resp = self.attach(targetId=99999, expectFailure=True)
         self.assertFalse(resp["success"])
-        self.assertIn("invalid target_id 99999 in attach config", resp["body"]["error"]["format"])
+        self.assertIn(
+            "Both debuggerId and targetId must be specified together",
+            resp["body"]["error"]["format"],
+        )
+
+    def test_attach_with_invalid_debuggerId_and_targetId(self):
+        """
+        Test that attaching with both debuggerId and targetId specified but
+        invalid fails with an appropriate error message.
+        """
+        self.build_and_create_debug_adapter()
+
+        # Attach with both debuggerId=9999 and targetId=99999 (both invalid).
+        # Since debugger ID 9999 likely doesn't exist in the global registry,
+        # we expect a validation error.
+        resp = self.attach(debuggerId=9999, targetId=99999, expectFailure=True)
+        self.assertFalse(resp["success"])
+        error_msg = resp["body"]["error"]["format"]
+        # Either error is acceptable - both indicate the debugger reuse
+        # validation is working correctly
+        self.assertTrue(
+            "Unable to find existing debugger" in error_msg
+            or f"Expected debugger/target not found error, got: {error_msg}"
+        )

lldb/tools/lldb-dap/DAP.cpp

@@ -1469,281 +1469,6 @@ void DAP::ProgressEventThread() {
   }
 }
 
-// All events from the debugger, target, process, thread and frames are
-// received in this function that runs in its own thread. We are using a
-// "FILE *" to output packets back to VS Code and they have mutexes in them
-// them prevent multiple threads from writing simultaneously so no locking
-// is required.
-void DAP::EventThread() {
-  llvm::set_thread_name("lldb.DAP.client." + m_client_name + ".event_handler");
-  lldb::SBListener listener = debugger.GetListener();
-  broadcaster.AddListener(listener, eBroadcastBitStopEventThread);
-  debugger.GetBroadcaster().AddListener(
-      listener, lldb::eBroadcastBitError | lldb::eBroadcastBitWarning);
-
-  // listen for thread events.
-  listener.StartListeningForEventClass(
-      debugger, lldb::SBThread::GetBroadcasterClassName(),
-      lldb::SBThread::eBroadcastBitStackChanged);
-
-  lldb::SBEvent event;
-  bool done = false;
-  while (!done) {
-    if (!listener.WaitForEvent(UINT32_MAX, event))
-      continue;
-
-    const uint32_t event_mask = event.GetType();
-    if (lldb::SBProcess::EventIsProcessEvent(event)) {
-      HandleProcessEvent(event, /*&process_exited=*/done);
-    } else if (lldb::SBTarget::EventIsTargetEvent(event)) {
-      HandleTargetEvent(event);
-    } else if (event_mask & lldb::SBTarget::eBroadcastBitNewTargetCreated) {
-      HandleNewTargetEvent(event);
-    } else if (lldb::SBBreakpoint::EventIsBreakpointEvent(event)) {
-      HandleBreakpointEvent(event);
-    } else if (lldb::SBThread::EventIsThreadEvent(event)) {
-      HandleThreadEvent(event);
-    } else if (event_mask & lldb::eBroadcastBitError ||
-               event_mask & lldb::eBroadcastBitWarning) {
-      HandleDiagnosticEvent(event);
-    } else if (event.BroadcasterMatchesRef(broadcaster)) {
-      if (event_mask & eBroadcastBitStopEventThread) {
-        done = true;
-      }
-    }
-  }
-}
-
-void DAP::HandleProcessEvent(const lldb::SBEvent &event, bool &process_exited) {
-  lldb::SBProcess process = lldb::SBProcess::GetProcessFromEvent(event);
-  const uint32_t event_mask = event.GetType();
-  DAP *dap_instance = DAPSessionManager::FindDAP(process.GetTarget());
-  if (!dap_instance) {
-    DAP_LOG(log, "Unable to find DAP instance for process {0}",
-            process.GetProcessID());
-    continue;
-  }
-
-  if (event_mask & lldb::SBProcess::eBroadcastBitStateChanged) {
-    auto state = lldb::SBProcess::GetStateFromEvent(event);
-    switch (state) {
-    case lldb::eStateConnected:
-    case lldb::eStateDetached:
-    case lldb::eStateInvalid:
-    case lldb::eStateUnloaded:
-      break;
-    case lldb::eStateAttaching:
-    case lldb::eStateCrashed:
-    case lldb::eStateLaunching:
-    case lldb::eStateStopped:
-    case lldb::eStateSuspended:
-      // Only report a stopped event if the process was not
-      // automatically restarted.
-      if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
-        SendStdOutStdErr(*this, process);
-        if (llvm::Error err = SendThreadStoppedEvent(*this))
-          DAP_LOG_ERROR(dap_instance->log, std::move(err),
-                              "({1}) reporting thread stopped: {0}",
-                              dap_instance->m_client_name);
-      }
-      break;
-    case lldb::eStateRunning:
-    case lldb::eStateStepping:
-      dap_instance->WillContinue();
-      SendContinuedEvent(*dap_instance);
-      break;
-    case lldb::eStateExited:
-      lldb::SBStream stream;
-      process.GetStatus(stream);
-      SendOutput(OutputType::Console, stream.GetData());
-
-      // When restarting, we can get an "exited" event for the process we
-      // just killed with the old PID, or even with no PID. In that case
-      // we don't have to terminate the session.
-      if (process.GetProcessID() == LLDB_INVALID_PROCESS_ID ||
-          process.GetProcessID() == restarting_process_id) {
-        dap_instance->restarting_process_id = LLDB_INVALID_PROCESS_ID;
-      } else {
-        // Run any exit LLDB commands the user specified in the
-        // launch.json
-        dap_instance->RunExitCommands();
-        SendProcessExitedEvent(*this, process);
-        dap_instance->SendTerminatedEvent();
-        process_exited = true;
-      }
-      break;
-    }
-  } else if ((event_mask & lldb::SBProcess::eBroadcastBitSTDOUT) ||
-             (event_mask & lldb::SBProcess::eBroadcastBitSTDERR)) {
-    SendStdOutStdErr(*this, process);
-  }
-}
-
-void DAP::HandleNewTargetEvent(const lldb::SBEvent &event) {
-  auto target = lldb::SBTarget::GetTargetFromEvent(event);
-  // Generate unique target ID and store the target for handoff.
-  lldb::user_id_t target_id = target.GetGloballyUniqueID();
-  DAPSessionManager::GetInstance().StoreTargetById(target_id,
-                                                      debugger);
-
-  // We create an attach config that will select the unique
-  // target ID of the created target. The DAP instance will attach to
-  // this existing target and the debug session will be ready to go.
-  llvm::json::Object attach_config;
-
-  // If we have a process name, add command to attach to the same
-  // process name.
-  attach_config.try_emplace("type", "lldb");
-  attach_config.try_emplace("targetId", target_id);
-  const char *session_name = target.GetTargetSessionName();
-  attach_config.try_emplace("name", session_name);
-
-  // 2. Construct the main 'startDebugging' request arguments.
-  llvm::json::Object start_debugging_args{
-      {"request", "attach"},
-      {"configuration", std::move(attach_config)}};
-
-  // Send the request. Note that this is a reverse request, so you don't
-  // expect a direct response in the same way as a client request.
-  SendReverseRequest<LogFailureResponseHandler>(
-      "startDebugging", std::move(start_debugging_args));
-}
-
-void DAP::HandleTargetEvent(const lldb::SBEvent &event) {
-  const uint32_t event_mask = event.GetType();
-  if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded ||
-      event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded ||
-      event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded ||
-      event_mask & lldb::SBTarget::eBroadcastBitSymbolsChanged) {
-    
-    lldb::SBTarget event_target =
-    lldb::SBTarget::GetTargetFromEvent(event);
-    // Find the DAP instance that owns this target.
-    DAP *dap_instance = DAPSessionManager::FindDAP(event_target);
-    if (!dap_instance)
-      continue;
-    const uint32_t num_modules = lldb::SBTarget::GetNumModulesFromEvent(event);
-    const bool remove_module =
-        event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded;
-
-    // NOTE: Both mutexes must be acquired to prevent deadlock when
-    // handling `modules_request`, which also requires both locks.
-    lldb::SBMutex api_mutex = GetAPIMutex();
-    const std::scoped_lock<lldb::SBMutex, std::mutex> guard(
-              api_mutex, dap_instance->modules_mutex);
-    for (uint32_t i = 0; i < num_modules; ++i) {
-      lldb::SBModule module =
-          lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
-
-      std::optional<protocol::Module> p_module =
-          CreateModule(target, module, remove_module);
-      if (!p_module)
-        continue;
-
-      const llvm::StringRef module_id = p_module->id;
-
-      const bool module_exists = dap_instance->modules.contains(module_id);
-      if (remove_module && module_exists) {
-        dap_instance->modules.erase(module_id);
-        dap_instance->Send(protocol::Event{"module",
-                             ModuleEventBody{std::move(p_module).value(),
-                                             ModuleEventBody::eReasonRemoved}});
-      } else if (module_exists) {
-        dap_instance->Send(protocol::Event{"module",
-                             ModuleEventBody{std::move(p_module).value(),
-                                             ModuleEventBody::eReasonChanged}});
-      } else if (!remove_module) {
-        dap_instance->modules.insert(module_id);
-        dap_instance->Send(protocol::Event{"module",
-                             ModuleEventBody{std::move(p_module).value(),
-                                             ModuleEventBody::eReasonNew}});
-      }
-    }
-  }
-}
-
-void DAP::HandleBreakpointEvent(const lldb::SBEvent &event) {
-  const uint32_t event_mask = event.GetType();
-  if (!(event_mask & lldb::SBTarget::eBroadcastBitBreakpointChanged))
-    return;
-
-  auto bp_from_event = lldb::SBBreakpoint::GetBreakpointEventTypeFromEvent(event);
-  lldb::SBBreakpoint bp_from_event =
-            lldb::SBBreakpoint::GetBreakpointFromEvent(event);
-  if (!bp_from_event.IsValid())
-    continue;
-
-  lldb::SBTarget event_target = bp_from_event.GetTarget();
-
-  // Find the DAP instance that owns this target.
-  DAP *dap_instance = DAPSessionManager::FindDAP(event_target);
-  if (!dap_instance)
-    continue;
-  auto bp =
-      Breakpoint(*dap_instance, lldb::SBBreakpoint::GetBreakpointFromEvent(event));
-  // If the breakpoint was set through DAP, it will have the
-  // BreakpointBase::kDAPBreakpointLabel. Regardless of whether
-  // locations were added, removed, or resolved, the breakpoint isn't
-  // going away and the reason is always "changed".
-  if ((event_type & lldb::eBreakpointEventTypeLocationsAdded ||
-       event_type & lldb::eBreakpointEventTypeLocationsRemoved ||
-       event_type & lldb::eBreakpointEventTypeLocationsResolved) &&
-      bp.MatchesName(BreakpointBase::kDAPBreakpointLabel)) {
-    // As the DAP client already knows the path of this breakpoint, we
-    // don't need to send it back as part of the "changed" event. This
-    // avoids sending paths that should be source mapped. Note that
-    // CreateBreakpoint doesn't apply source mapping and certain
-    // implementation ignore the source part of this event anyway.
-    protocol::Breakpoint protocol_bp = bp.ToProtocolBreakpoint();
-
-    // "source" is not needed here, unless we add adapter data to be
-    // saved by the client.
-    if (protocol_bp.source && !protocol_bp.source->adapterData)
-      protocol_bp.source = std::nullopt;
-
-    llvm::json::Object body;
-    body.try_emplace("breakpoint", protocol_bp);
-    body.try_emplace("reason", "changed");
-
-    llvm::json::Object bp_event = CreateEventObject("breakpoint");
-    bp_event.try_emplace("body", std::move(body));
-
-    dap_instance->SendJSON(llvm::json::Value(std::move(bp_event)));
-  }
-}
-
-void DAP::HandleThreadEvent(const lldb::SBEvent &event) {
-  const uint32_t event_type = event.GetType();
-
-  if (event_type & lldb::SBThread::eBroadcastBitStackChanged) {
-    const lldb::SBThread evt_thread = lldb::SBThread::GetThreadFromEvent(event);
-    SendInvalidatedEvent(*this, {InvalidatedEventBody::eAreaStacks},
-                         evt_thread.GetThreadID());
-  }
-}
-
-void DAP::HandleDiagnosticEvent(const lldb::SBEvent &event) {
-  const lldb::SBStructuredData data =
-      lldb::SBDebugger::GetDiagnosticFromEvent(event);
-  // Global debugger events - send to all DAP instances.
-  std::vector<DAP *> active_instances =
-      DAPSessionManager::GetInstance().GetActiveSessions();
-  for (DAP *dap_instance : active_instances) {
-    if (!dap_instance)
-      continue;
-
-    lldb::SBStructuredData data =
-        lldb::SBDebugger::GetDiagnosticFromEvent(event);
-    if (!data.IsValid())
-      return;
-
-    std::string type = GetStringValue(data.GetValueForKey("type"));
-    std::string message = GetStringValue(data.GetValueForKey("message"));
-    dap_instance->SendOutput(OutputType::Important,
-              llvm::formatv("{0}: {1}", type, message).str());
-  }
-}
-
 std::vector<protocol::Breakpoint> DAP::SetSourceBreakpoints(
     const protocol::Source &source,
     const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints) {

lldb/tools/lldb-dap/DAP.h

@@ -417,6 +417,9 @@ struct DAP final : public DAPTransport::MessageHandler {
 
   lldb::SBMutex GetAPIMutex() const { return target.GetAPIMutex(); }
 
+  /// Get the client name for this DAP session.
+  llvm::StringRef GetClientName() const { return m_client_name; }
+
   void StartEventThread();
   void StartProgressEventThread();
 
@@ -475,13 +478,6 @@ struct DAP final : public DAPTransport::MessageHandler {
 
   /// Event threads.
   /// @{
-  void EventThread();
-  void HandleProcessEvent(const lldb::SBEvent &event, bool &process_exited);
-  void HandleTargetEvent(const lldb::SBEvent &event);
-  void HandleNewTargetEvent(const lldb::SBEvent &event);
-  void HandleBreakpointEvent(const lldb::SBEvent &event);
-  void HandleThreadEvent(const lldb::SBEvent &event);
-  void HandleDiagnosticEvent(const lldb::SBEvent &event);
   void ProgressEventThread();
 
   /// Event thread is a shared pointer in case we have a multiple

lldb/tools/lldb-dap/DAPForward.h

@@ -28,6 +28,7 @@ namespace lldb {
 class SBAttachInfo;
 class SBBreakpoint;
 class SBBreakpointLocation;
+class SBBroadcaster;
 class SBCommandInterpreter;
 class SBCommandReturnObject;
 class SBCommunication;

lldb/tools/lldb-dap/DAPSessionManager.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 #include "DAPSessionManager.h"
 #include "DAP.h"
+#include "EventHelper.h"
 #include "lldb/API/SBBroadcaster.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBTarget.h"
@@ -108,7 +109,8 @@ DAPSessionManager::GetEventThreadForDebugger(lldb::SBDebugger debugger,
   // Create a new event thread and store it.
   auto new_thread_sp = std::make_shared<ManagedEventThread>(
       requesting_dap->broadcaster,
-      std::thread(&DAP::EventThread, requesting_dap));
+      std::thread(EventThread, debugger, requesting_dap->broadcaster,
+                  requesting_dap->m_client_name, requesting_dap->log));
   m_debugger_event_threads[debugger_id] = new_thread_sp;
   return new_thread_sp;
 }