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 /// _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
