[lldb-dap] Don't emit a removed module event for unseen modules (#139324)

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

@@ -134,7 +134,6 @@ class DebugCommunication(object):
         self.thread_stop_reasons = {}
         self.progress_events = []
         self.reverse_requests = []
-        self.module_events = []
         self.sequence = 1
         self.threads = None
         self.recv_thread.start()
@@ -248,11 +247,6 @@ class DebugCommunication(object):
                 # and 'progressEnd' events. Keep these around in case test
                 # cases want to verify them.
                 self.progress_events.append(packet)
-            elif event == "module":
-                # Module events indicate that some information about a module has changed.
-                self.module_events.append(packet)
-                # no need to add 'module' event packets to our packets list
-                return keepGoing
 
         elif packet_type == "response":
             if packet["command"] == "disconnect":

lldb/test/API/tools/lldb-dap/module-event/Makefile

@@ -0,0 +1,12 @@
+CXX_SOURCES := main.cpp
+LD_EXTRAS := -Wl,-rpath "-Wl,$(shell pwd)"
+USE_LIBDL :=1
+
+a.out: libother
+
+include Makefile.rules
+
+# The following shared library will be used to test breakpoints under dynamic loading
+libother:  other.c
+	"$(MAKE)" -f $(MAKEFILE_RULES) \
+		DYLIB_ONLY=YES DYLIB_C_SOURCES=other.c DYLIB_NAME=other

lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py

@@ -0,0 +1,54 @@
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbdap_testcase
+import re
+
+
+class TestDAP_module_event(lldbdap_testcase.DAPTestCaseBase):
+    def test_module_event(self):
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program, stopOnEntry=True)
+
+        source = "main.cpp"
+        breakpoint1_line = line_number(source, "// breakpoint 1")
+        breakpoint2_line = line_number(source, "// breakpoint 2")
+        breakpoint3_line = line_number(source, "// breakpoint 3")
+
+        breakpoint_ids = self.set_source_breakpoints(
+            source, [breakpoint1_line, breakpoint2_line, breakpoint3_line]
+        )
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        # We're now stopped at breakpoint 1 before the dlopen. Flush all the module events.
+        event = self.dap_server.wait_for_event("module", 0.25)
+        while event is not None:
+            event = self.dap_server.wait_for_event("module", 0.25)
+
+        # Continue to the second breakpoint, before the dlclose.
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        # Make sure we got a module event for libother.
+        event = self.dap_server.wait_for_event("module", 5)
+        self.assertTrue(event, "didn't get a module event")
+        module_name = event["body"]["module"]["name"]
+        module_id = event["body"]["module"]["id"]
+        self.assertEqual(event["body"]["reason"], "new")
+        self.assertIn("libother", module_name)
+
+        # Continue to the third breakpoint, after the dlclose.
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        # Make sure we got a module event for libother.
+        event = self.dap_server.wait_for_event("module", 5)
+        self.assertTrue(event, "didn't get a module event")
+        reason = event["body"]["reason"]
+        self.assertEqual(event["body"]["reason"], "removed")
+        self.assertEqual(event["body"]["module"]["id"], module_id)
+
+        # The removed module event should omit everything but the module id.
+        # Check that there's no module name in the event.
+        self.assertNotIn("name", event["body"]["module"])
+
+        self.continue_to_exit()

lldb/test/API/tools/lldb-dap/module-event/main.cpp

@@ -0,0 +1,22 @@
+#include <dlfcn.h>
+#include <stdio.h>
+
+int main(int argc, char const *argv[]) {
+
+#if defined(__APPLE__)
+  const char *libother_name = "libother.dylib";
+#else
+  const char *libother_name = "libother.so";
+#endif
+
+  printf("before dlopen\n"); // breakpoint 1
+  void *handle = dlopen(libother_name, RTLD_NOW);
+  int (*foo)(int) = (int (*)(int))dlsym(handle, "foo");
+  foo(12);
+
+  printf("before dlclose\n"); // breakpoint 2
+  dlclose(handle);
+  printf("after dlclose\n"); // breakpoint 3
+
+  return 0; // breakpoint 1
+}

lldb/test/API/tools/lldb-dap/module-event/other.c

@@ -0,0 +1,5 @@
+extern int foo(int x) {
+  int y = x + 42; // break other
+  int z = y + 42;
+  return z;
+}

lldb/test/API/tools/lldb-dap/module/TestDAP_module.py

@@ -60,13 +60,15 @@ class TestDAP_module(lldbdap_testcase.DAPTestCaseBase):
         # Collect all the module names we saw as events.
         module_new_names = []
         module_changed_names = []
-        for module_event in self.dap_server.module_events:
-            module_name = module_event["body"]["module"]["name"]
+        module_event = self.dap_server.wait_for_event("module", 1)
+        while module_event is not None:
             reason = module_event["body"]["reason"]
             if reason == "new":
-                module_new_names.append(module_name)
+                module_new_names.append(module_event["body"]["module"]["name"])
             elif reason == "changed":
-                module_changed_names.append(module_name)
+                module_changed_names.append(module_event["body"]["module"]["name"])
+
+            module_event = self.dap_server.wait_for_event("module", 1)
 
         # Make sure we got an event for every active module.
         self.assertNotEqual(len(module_new_names), 0)

lldb/tools/lldb-dap/DAP.cpp

@@ -106,16 +106,6 @@ static uint64_t GetUintFromStructuredData(lldb::SBStructuredData &data,
   return keyValue.GetUnsignedIntegerValue();
 }
 
-static llvm::StringRef GetModuleEventReason(uint32_t event_mask) {
-  if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded)
-    return "new";
-  if (event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded)
-    return "removed";
-  assert(event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded ||
-         event_mask & lldb::SBTarget::eBroadcastBitSymbolsChanged);
-  return "changed";
-}
-
 /// Return string with first character capitalized.
 static std::string capitalize(llvm::StringRef str) {
   if (str.empty())
@@ -1571,18 +1561,41 @@ void DAP::EventThread() {
             event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded ||
             event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded ||
             event_mask & lldb::SBTarget::eBroadcastBitSymbolsChanged) {
-          llvm::StringRef reason = GetModuleEventReason(event_mask);
           const uint32_t num_modules =
               lldb::SBTarget::GetNumModulesFromEvent(event);
+          std::lock_guard<std::mutex> guard(modules_mutex);
           for (uint32_t i = 0; i < num_modules; ++i) {
             lldb::SBModule module =
                 lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
             if (!module.IsValid())
               continue;
+            llvm::StringRef module_id = module.GetUUIDString();
+            if (module_id.empty())
+              continue;
+
+            llvm::StringRef reason;
+            bool id_only = false;
+            if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded) {
+              modules.insert(module_id);
+              reason = "new";
+            } else {
+              // If this is a module we've never told the client about, don't
+              // send an event.
+              if (!modules.contains(module_id))
+                continue;
+
+              if (event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded) {
+                modules.erase(module_id);
+                reason = "removed";
+                id_only = true;
+              } else {
+                reason = "changed";
+              }
+            }
 
             llvm::json::Object body;
             body.try_emplace("reason", reason);
-            body.try_emplace("module", CreateModule(target, module));
+            body.try_emplace("module", CreateModule(target, module, id_only));
             llvm::json::Object module_event = CreateEventObject("module");
             module_event.try_emplace("body", std::move(body));
             SendJSON(llvm::json::Value(std::move(module_event)));

lldb/tools/lldb-dap/DAP.h

@@ -39,6 +39,7 @@
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Threading.h"
@@ -212,6 +213,13 @@ struct DAP {
   /// The initial thread list upon attaching.
   std::optional<llvm::json::Array> initial_thread_list;
 
+  /// Keep track of all the modules our client knows about: either through the
+  /// modules request or the module events.
+  /// @{
+  std::mutex modules_mutex;
+  llvm::StringSet<> modules;
+  /// @}
+
   /// Creates a new DAP sessions.
   ///
   /// \param[in] log

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

@@ -45,9 +45,20 @@ void ModulesRequestHandler::operator()(
   FillResponse(request, response);
 
   llvm::json::Array modules;
-  for (size_t i = 0; i < dap.target.GetNumModules(); i++) {
-    lldb::SBModule module = dap.target.GetModuleAtIndex(i);
-    modules.emplace_back(CreateModule(dap.target, module));
+
+  {
+    std::lock_guard<std::mutex> guard(dap.modules_mutex);
+    for (size_t i = 0; i < dap.target.GetNumModules(); i++) {
+      lldb::SBModule module = dap.target.GetModuleAtIndex(i);
+      if (!module.IsValid())
+        continue;
+
+      llvm::StringRef module_id = module.GetUUIDString();
+      if (!module_id.empty())
+        dap.modules.insert(module_id);
+
+      modules.emplace_back(CreateModule(dap.target, module));
+    }
   }
 
   llvm::json::Object body;

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -395,13 +395,18 @@ static std::string ConvertDebugInfoSizeToString(uint64_t debug_info) {
   return oss.str();
 }
 
-llvm::json::Value CreateModule(lldb::SBTarget &target, lldb::SBModule &module) {
+llvm::json::Value CreateModule(lldb::SBTarget &target, lldb::SBModule &module,
+                               bool id_only) {
   llvm::json::Object object;
   if (!target.IsValid() || !module.IsValid())
     return llvm::json::Value(std::move(object));
 
   const char *uuid = module.GetUUIDString();
   object.try_emplace("id", uuid ? std::string(uuid) : std::string(""));
+
+  if (id_only)
+    return llvm::json::Value(std::move(object));
+
   object.try_emplace("name", std::string(module.GetFileSpec().GetFilename()));
   char module_path_arr[PATH_MAX];
   module.GetFileSpec().GetPath(module_path_arr, sizeof(module_path_arr));

lldb/tools/lldb-dap/JSONUtils.h

@@ -206,10 +206,15 @@ void FillResponse(const llvm::json::Object &request,
 /// \param[in] module
 ///     A LLDB module object to convert into a JSON value
 ///
+/// \param[in] id_only
+///     Only include the module ID in the JSON value. This is used when sending
+///     a "removed" module event.
+///
 /// \return
 ///     A "Module" JSON object with that follows the formal JSON
 ///     definition outlined by Microsoft.
-llvm::json::Value CreateModule(lldb::SBTarget &target, lldb::SBModule &module);
+llvm::json::Value CreateModule(lldb::SBTarget &target, lldb::SBModule &module,
+                               bool id_only = false);
 
 /// Create a "Event" JSON object using \a event_name as the event name
 ///