https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/139324
>From 018bb069e873007501d639dceaf8581eb34ddbb9 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Fri, 9 May 2025 13:18:49 -0700 Subject: [PATCH 1/3] [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 --- .../test/tools/lldb-dap/dap_server.py | 6 -- .../API/tools/lldb-dap/module-event/Makefile | 12 ++++ .../module-event/TestDAP_module_event.py | 55 +++++++++++++++++++ .../API/tools/lldb-dap/module-event/main.cpp | 22 ++++++++ .../API/tools/lldb-dap/module-event/other.c | 5 ++ .../tools/lldb-dap/module/TestDAP_module.py | 10 ++-- lldb/tools/lldb-dap/DAP.cpp | 39 +++++++++---- lldb/tools/lldb-dap/DAP.h | 8 +++ .../Handler/ModulesRequestHandler.cpp | 17 +++++- lldb/tools/lldb-dap/JSONUtils.cpp | 7 ++- lldb/tools/lldb-dap/JSONUtils.h | 7 ++- 11 files changed, 161 insertions(+), 27 deletions(-) create mode 100644 lldb/test/API/tools/lldb-dap/module-event/Makefile create mode 100644 lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py create mode 100644 lldb/test/API/tools/lldb-dap/module-event/main.cpp create mode 100644 lldb/test/API/tools/lldb-dap/module-event/other.c 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 /// >From 4b794400d431e9a181ba50a8d6b6f5804d53ab81 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Fri, 9 May 2025 13:39:00 -0700 Subject: [PATCH 2/3] Fix discrepancy between black and darker --- .../test/API/tools/lldb-dap/module-event/TestDAP_module_event.py | 1 - 1 file changed, 1 deletion(-) 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 index 819640e5598bd..c216b5d92823c 100644 --- 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 @@ -7,7 +7,6 @@ class TestDAP_module_event(lldbdap_testcase.DAPTestCaseBase): - def test_module_event(self): program = self.getBuildArtifact("a.out") self.build_and_launch(program, stopOnEntry=True) >From c01964e93fa4c1a76c44480308493d91fcf5951c Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Fri, 9 May 2025 16:51:08 -0700 Subject: [PATCH 3/3] Hold the lock while iterating the modules --- lldb/tools/lldb-dap/DAP.cpp | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 828d7d4873156..483af1d661b51 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -1558,6 +1558,7 @@ void DAP::EventThread() { event_mask & lldb::SBTarget::eBroadcastBitSymbolsChanged) { 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); @@ -1569,24 +1570,21 @@ void DAP::EventThread() { 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"; + 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 { - // 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"; - } + reason = "changed"; } } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
