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

On macOS, lldb-dap is sending module events with reason "removed" for
modules that we never told the client about in the first place. This is
because when we  create a target with a binary, by default all of its
dependent libraries are loaded too. When we start executing and see the
that the binary and dyld are loaded, we throw away the image list and
then as dyld tell us about the correct binaries being loaded in the
process, we add them.

This PR addresses the issue by keeping track which modules the DAP
client knows about. Clients can find out about modules either through
the modules request, or through module events. Only when we have told a
client about a module, we send module changed or module removed events.

This PR also reduces the amount of data sent for removed module events.
The DAP specification [1] says that for removed module events, only the
ID is used.

Fixes #139323

[1] https://microsoft.github.io/debug-adapter-protocol/specification#Events_Module

Author: JDevlieghere

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

@@ -134,7 +134,6 @@ def __init__(self, recv, send, init_commands, log_file=None):
         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 @@ def handle_recv_packet(self, packet):
                 # 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,55 @@
+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 @@ def checkSymbolsLoadedWithSize():
         # 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

@@ -105,16 +105,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())
@@ -1566,18 +1556,43 @@ 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);
           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;
+            {
+              std::lock_guard<std::mutex> guard(modules_mutex);
+              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

@@ -460,13 +460,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));