[lldb-dap] Refactor event thread

[da-viper]

Handle each event type in a different function

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

Handle each event type in a different function

---

Full diff: https://github.com/llvm/llvm-project/pull/166948.diff

2 Files Affected:

• (modified) lldb/tools/lldb-dap/DAP.cpp (+174-156)
• (modified) lldb/tools/lldb-dap/DAP.h (+4)

diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index f009a902f79e7..7534e76e885b9 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -1317,7 +1317,7 @@ void DAP::ProgressEventThread() {
   lldb::SBEvent event;
   bool done = false;
   while (!done) {
-    if (listener.WaitForEvent(1, event)) {
+    if (listener.WaitForEvent(UINT32_MAX, event)) {
       const auto event_mask = event.GetType();
       if (event.BroadcasterMatchesRef(broadcaster)) {
         if (event_mask & eBroadcastBitStopProgressThread) {
@@ -1375,7 +1375,6 @@ void DAP::ProgressEventThread() {
 // 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(
@@ -1386,169 +1385,176 @@ void DAP::EventThread() {
       debugger, lldb::SBThread::GetBroadcasterClassName(),
       lldb::SBThread::eBroadcastBitStackChanged);
 
+  lldb::SBEvent event;
   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);
-        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(log, std::move(err),
-                              "({1}) reporting thread stopped: {0}",
-                              m_client_name);
-            }
-            break;
-          case lldb::eStateRunning:
-          case lldb::eStateStepping:
-            WillContinue();
-            SendContinuedEvent(*this);
-            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) {
-              restarting_process_id = LLDB_INVALID_PROCESS_ID;
-            } else {
-              // Run any exit LLDB commands the user specified in the
-              // launch.json
-              RunExitCommands();
-              SendProcessExitedEvent(*this, process);
-              SendTerminatedEvent();
-              done = true;
-            }
-            break;
-          }
-        } else if ((event_mask & lldb::SBProcess::eBroadcastBitSTDOUT) ||
-                   (event_mask & lldb::SBProcess::eBroadcastBitSTDERR)) {
-          SendStdOutStdErr(*this, 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) {
-          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, 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;
-
-            llvm::StringRef module_id = p_module->id;
-
-            const bool module_exists = modules.contains(module_id);
-            if (remove_module && module_exists) {
-              modules.erase(module_id);
-              Send(protocol::Event{
-                  "module", ModuleEventBody{std::move(p_module).value(),
-                                            ModuleEventBody::eReasonRemoved}});
-            } else if (module_exists) {
-              Send(protocol::Event{
-                  "module", ModuleEventBody{std::move(p_module).value(),
-                                            ModuleEventBody::eReasonChanged}});
-            } else if (!remove_module) {
-              modules.insert(module_id);
-              Send(protocol::Event{
-                  "module", ModuleEventBody{std::move(p_module).value(),
-                                            ModuleEventBody::eReasonNew}});
-            }
-          }
-        }
-      } else if (lldb::SBBreakpoint::EventIsBreakpointEvent(event)) {
-        if (event_mask & lldb::SBTarget::eBroadcastBitBreakpointChanged) {
-          auto event_type =
-              lldb::SBBreakpoint::GetBreakpointEventTypeFromEvent(event);
-          auto bp = Breakpoint(
-              *this, 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));
-
-            SendJSON(llvm::json::Value(std::move(bp_event)));
-          }
-        }
+    if (!listener.WaitForEvent(UINT32_MAX, event))
+      continue;
 
-      } else if (lldb::SBThread::EventIsThreadEvent(event)) {
-        HandleThreadEvent(event);
-      } else if (event_mask & lldb::eBroadcastBitError ||
-                 event_mask & lldb::eBroadcastBitWarning) {
-        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"));
-        SendOutput(OutputType::Important,
-                   llvm::formatv("{0}: {1}", type, message).str());
-      } else if (event.BroadcasterMatchesRef(broadcaster)) {
-        if (event_mask & eBroadcastBitStopEventThread) {
-          done = true;
-        }
+    const uint32_t event_mask = event.GetType();
+    if (lldb::SBProcess::EventIsProcessEvent(event)) {
+      HandleProcessEvent(event, done);
+    } else if (lldb::SBTarget::EventIsTargetEvent(event)) {
+      HandleTargetEvent(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();
+  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(log, std::move(err),
+                        "({1}) reporting thread stopped: {0}", m_client_name);
+      }
+      break;
+    case lldb::eStateRunning:
+    case lldb::eStateStepping:
+      WillContinue();
+      SendContinuedEvent(*this);
+      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) {
+        restarting_process_id = LLDB_INVALID_PROCESS_ID;
+      } else {
+        // Run any exit LLDB commands the user specified in the
+        // launch.json
+        RunExitCommands();
+        SendProcessExitedEvent(*this, process);
+        SendTerminatedEvent();
+        process_exited = true;
+      }
+      break;
+    }
+  } else if ((event_mask & lldb::SBProcess::eBroadcastBitSTDOUT) ||
+             (event_mask & lldb::SBProcess::eBroadcastBitSTDERR)) {
+    SendStdOutStdErr(*this, process);
+  }
+}
+
+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) {
+    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,
+                                                            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 = modules.contains(module_id);
+      if (remove_module && module_exists) {
+        modules.erase(module_id);
+        Send(protocol::Event{"module",
+                             ModuleEventBody{std::move(p_module).value(),
+                                             ModuleEventBody::eReasonRemoved}});
+      } else if (module_exists) {
+        Send(protocol::Event{"module",
+                             ModuleEventBody{std::move(p_module).value(),
+                                             ModuleEventBody::eReasonChanged}});
+      } else if (!remove_module) {
+        modules.insert(module_id);
+        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 event_type = lldb::SBBreakpoint::GetBreakpointEventTypeFromEvent(event);
+  auto bp =
+      Breakpoint(*this, 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));
+
+    SendJSON(llvm::json::Value(std::move(bp_event)));
+  }
+}
+
 void DAP::HandleThreadEvent(const lldb::SBEvent &event) {
-  uint32_t event_type = event.GetType();
+  const uint32_t event_type = event.GetType();
 
   if (event_type & lldb::SBThread::eBroadcastBitStackChanged) {
     const lldb::SBThread evt_thread = lldb::SBThread::GetThreadFromEvent(event);
@@ -1557,6 +1563,18 @@ void DAP::HandleThreadEvent(const lldb::SBEvent &event) {
   }
 }
 
+void DAP::HandleDiagnosticEvent(const lldb::SBEvent &event) {
+  const 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"));
+  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) {
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index b4f111e4e720c..5d40341329f34 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -454,7 +454,11 @@ 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 HandleBreakpointEvent(const lldb::SBEvent &event);
   void HandleThreadEvent(const lldb::SBEvent &event);
+  void HandleDiagnosticEvent(const lldb::SBEvent &event);
   void ProgressEventThread();
 
   std::thread event_thread;

[ashgti]

LGTM!

[JDevlieghere]

I know I recently moved the event thread into the DAP class, but as its complexity has grown, maybe we should move it out again into its own class again?

[da-viper]

It came from the InitializeRequestHandler file. I don't mind moving it to EventHelper but the DAP file is not really large (approx 1701 LOC)

[JDevlieghere]

It came from the InitializeRequestHandler file. I don't mind moving it to EventHelper but the DAP file is not really large (approx 1701 LOC)

I'm not so much worried about LOC, it's more about abstraction. It definitely shouldn't go back into the request handler, but I do think it could be its own standalone thing (class).