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 or 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=1 and targetId=99999 (both invalid).
+        # Since debugger ID 1 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/test/API/tools/lldb-dap/startDebugging/TestDAP_startDebugging.py

@@ -59,8 +59,8 @@ def test_startDebugging_debugger_reuse(self):
         # Send a startDebugging request with debuggerId and targetId
         # This simulates creating a child DAP session that reuses the debugger
         self.dap_server.request_evaluate(
-            f"`lldb-dap start-debugging attach '{{\"debuggerId\":{test_debugger_id},\"targetId\":{test_target_id}}}'",
-            context="repl"
+            f'`lldb-dap start-debugging attach \'{{"debuggerId":{test_debugger_id},"targetId":{test_target_id}}}\'',
+            context="repl",
         )
 
         self.continue_to_exit()
@@ -77,7 +77,13 @@ def test_startDebugging_debugger_reuse(self):
         self.assertEqual(request["arguments"]["request"], "attach")
 
         config = request["arguments"]["configuration"]
-        self.assertEqual(config["debuggerId"], test_debugger_id,
-                        "Reverse request should include debugger ID")
-        self.assertEqual(config["targetId"], test_target_id,
-                        "Reverse request should include target ID")
+        self.assertEqual(
+            config["debuggerId"],
+            test_debugger_id,
+            "Reverse request should include debugger ID",
+        )
+        self.assertEqual(
+            config["targetId"],
+            test_target_id,
+            "Reverse request should include target ID",
+        )

lldb/tools/lldb-dap/DAP.cpp

@@ -1467,260 +1467,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::SBEvent event;
-  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);
-
-  bool done = false;
-  while (!done) {
-    if (listener.WaitForEvent(1, event)) {
-      const auto event_mask = event.GetType();
-      if (lldb::SBProcess::EventIsProcessEvent(event)) {
-        lldb::SBProcess process = lldb::SBProcess::GetProcessFromEvent(event);
-        // Find the DAP instance that owns this process's target.
-        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(*dap_instance, process);
-              if (llvm::Error err = SendThreadStoppedEvent(*dap_instance))
-                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);
-            dap_instance->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() == dap_instance->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(*dap_instance, process);
-              dap_instance->SendTerminatedEvent();
-              done = true;
-            }
-            break;
-          }
-        } else if ((event_mask & lldb::SBProcess::eBroadcastBitSTDOUT) ||
-                   (event_mask & lldb::SBProcess::eBroadcastBitSTDERR)) {
-          SendStdOutStdErr(*dap_instance, process);
-        }
-      } else if (lldb::SBTarget::EventIsTargetEvent(event)) {
-        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(dap_instance->target, module, remove_module);
-            if (!p_module)
-              continue;
-
-            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}});
-            }
-          }
-        } else if (event_mask & lldb::SBTarget::eBroadcastBitNewTargetCreated) {
-          auto target = lldb::SBTarget::GetTargetFromEvent(event);
-
-          lldb::user_id_t target_id = target.GetGloballyUniqueID();
-          lldb::SBDebugger target_debugger = target.GetDebugger();
-          int debugger_id = target_debugger.GetID();
-
-          // We create an attach config that contains the debugger ID and target
-          // ID. The new DAP instance will use these IDs to find the existing
-          // debugger and target via FindDebuggerWithID and
-          // FindTargetByGloballyUniqueID.
-          llvm::json::Object attach_config;
-
-          attach_config.try_emplace("type", "lldb");
-          attach_config.try_emplace("debuggerId", debugger_id);
-          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));
-        }
-      } else if (lldb::SBBreakpoint::EventIsBreakpointEvent(event)) {
-        lldb::SBBreakpoint bp =
-            lldb::SBBreakpoint::GetBreakpointFromEvent(event);
-        if (!bp.IsValid())
-          continue;
-
-        lldb::SBTarget event_target = bp.GetTarget();
-
-        // Find the DAP instance that owns this target.
-        DAP *dap_instance = DAPSessionManager::FindDAP(event_target);
-        if (!dap_instance)
-          continue;
-
-        if (event_mask & lldb::SBTarget::eBroadcastBitBreakpointChanged) {
-          auto event_type =
-              lldb::SBBreakpoint::GetBreakpointEventTypeFromEvent(event);
-          auto breakpoint = Breakpoint(*dap_instance, bp);
-          // 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) &&
-              breakpoint.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 =
-                breakpoint.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)));
-          }
-        }
-
-      } else if (lldb::SBThread::EventIsThreadEvent(event)) {
-        HandleThreadEvent(event);
-      } else if (event_mask & lldb::eBroadcastBitError ||
-                 event_mask & lldb::eBroadcastBitWarning) {
-        // 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())
-            continue;
-
-          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());
-        }
-      } else if (event.BroadcasterMatchesRef(broadcaster)) {
-        if (event_mask & eBroadcastBitStopEventThread) {
-          done = true;
-        }
-      }
-    }
-  }
-}
-
-void DAP::HandleThreadEvent(const lldb::SBEvent &event) {
-  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());
-  }
-}
-
 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,8 +478,6 @@ struct DAP final : public DAPTransport::MessageHandler {
 
   /// Event threads.
   /// @{
-  void EventThread();
-  void HandleThreadEvent(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;
 }