[lldb-dap] Don't emit a removed module event for unseen modules #139324
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 llvm#139323 [1] https://microsoft.github.io/debug-adapter-protocol/specification#Events_Module
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesOn 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 we load all of its dependent libraries too. When we start executing and see the that the binary and dyld are loaded, we throw away the image list and let dyld tell us about the correct binaries to load. 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. It also makes the testing more robust. Fixes #139323 [1] https://microsoft.github.io/debug-adapter-protocol/specification#Events_Module Full diff: https://github.com/llvm/llvm-project/pull/139324.diff 11 Files Affected:
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..819640e5598bd
--- /dev/null
+++ b/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()
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 f6754b1f8d7a3..828d7d4873156 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/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,7 +1556,6 @@ 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) {
@@ -1574,10 +1563,36 @@ void DAP::EventThread() {
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)));
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index afeda8d81efb0..75e4ab0e23616 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 4409cf5b27e5b..e647bd23582c4 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/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));
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index d0e20729f4ed9..a5baf61c90f60 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -267,10 +267,15 @@ CreateBreakpoint(BreakpointBase *bp,
/// \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
///
|
|
✅ With the latest revision this PR passed the Python code formatter. |
ashgti
approved these changes
May 9, 2025
lldb/tools/lldb-dap/DAP.cpp
Outdated
| llvm::StringRef reason; | ||
| bool id_only = false; | ||
| { | ||
| std::lock_guard<std::mutex> guard(modules_mutex); |
There was a problem hiding this comment.
Should we grab this lock while we iterate the entire list of modules from the event?
I think we could we get a modules request in the middle of processing this list, which might be a bit inconsistent when we get back to handling the events.
ashgti
approved these changes
May 10, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/8574 Here is the relevant piece of the build log for the reference |
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
May 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 we load all of its dependent libraries too. When we start executing and see the that the binary and dyld are loaded, we throw away the image list and let dyld tell us about the correct binaries to load.
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. It also makes the testing more robust.
Fixes #139323
[1] https://microsoft.github.io/debug-adapter-protocol/specification#Events_Module