[Lldb-commits] [lldb] b7c449a - [lldb-dap] Don't emit a removed module event for unseen modules (#139324)
via lldb-commits
lldb-commits at lists.llvm.org
Fri May 9 23:34:09 PDT 2025
Author: Jonas Devlieghere
Date: 2025-05-09T23:34:05-07:00
New Revision: b7c449ac0b0c4ccbe99937052c9428960cea7664
URL: https://github.com/llvm/llvm-project/commit/b7c449ac0b0c4ccbe99937052c9428960cea7664
DIFF: https://github.com/llvm/llvm-project/commit/b7c449ac0b0c4ccbe99937052c9428960cea7664.diff
LOG: [lldb-dap] Don't emit a removed module event for unseen modules (#139324)
Added:
lldb/test/API/tools/lldb-dap/module-event/Makefile
lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py
lldb/test/API/tools/lldb-dap/module-event/main.cpp
lldb/test/API/tools/lldb-dap/module-event/other.c
Modified:
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
lldb/test/API/tools/lldb-dap/module/TestDAP_module.py
lldb/tools/lldb-dap/DAP.cpp
lldb/tools/lldb-dap/DAP.h
lldb/tools/lldb-dap/Handler/ModulesRequestHandler.cpp
lldb/tools/lldb-dap/JSONUtils.cpp
lldb/tools/lldb-dap/JSONUtils.h
Removed:
################################################################################
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index e10342b72f4f0..c974866306d2a 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/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":
diff --git a/lldb/test/API/tools/lldb-dap/module-event/Makefile b/lldb/test/API/tools/lldb-dap/module-event/Makefile
new file mode 100644
index 0000000000000..99d79b8053878
--- /dev/null
+++ b/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
diff --git a/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py b/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py
new file mode 100644
index 0000000000000..c216b5d92823c
--- /dev/null
+++ b/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()
diff --git a/lldb/test/API/tools/lldb-dap/module-event/main.cpp b/lldb/test/API/tools/lldb-dap/module-event/main.cpp
new file mode 100644
index 0000000000000..711471b9fadd0
--- /dev/null
+++ b/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
+}
diff --git a/lldb/test/API/tools/lldb-dap/module-event/other.c b/lldb/test/API/tools/lldb-dap/module-event/other.c
new file mode 100644
index 0000000000000..dd164597269dc
--- /dev/null
+++ b/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;
+}
diff --git a/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py b/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py
index 210819cfdd732..3fc0f752ee39e 100644
--- a/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py
+++ b/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)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 80c150018f0b3..9a3cc32d8c324 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/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)));
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index ecde222c9263c..cbda57d27e8d9 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/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
diff --git a/lldb/tools/lldb-dap/Handler/ModulesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ModulesRequestHandler.cpp
index ed51d395768c4..d37f302b06958 100644
--- a/lldb/tools/lldb-dap/Handler/ModulesRequestHandler.cpp
+++ b/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;
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index e184e7602ef14..bd2e6630a126e 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/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));
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 7c7cf620f0c06..9c4dd0584bd21 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/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
///
More information about the lldb-commits
mailing list