Skip to content

Conversation

@JDevlieghere
Copy link
Member

I got a bug report where a pedantic DAP client complains about getting two "new" module events for the same UUID. This is caused by the dyld transition from the on-disk dyld to the shared cache dyld, which share the same UUID. The transition is not generating an unloaded event (because we're not really unloading dyld) but we do get a loaded event (because the load address changed). This PR fixes the issue by relying on the modules set as the source of truth instead of relying on the event type.

I got a bug report where a pedantic DAP client complains about getting
two "new" module events for the same UUID. This is caused by the dyld
transition from the on-disk dyld to the shared cache dyld, which share
the same UUID. The transition is not generating an unloaded
event (because we're not really unloading dyld) but we do
get a loaded event (because the load address changed). This PR fixes the
issue by relying on the modules set as the source of truth instead of
relying on the event type.
@llvmbot
Copy link
Member

llvmbot commented May 20, 2025

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

I got a bug report where a pedantic DAP client complains about getting two "new" module events for the same UUID. This is caused by the dyld transition from the on-disk dyld to the shared cache dyld, which share the same UUID. The transition is not generating an unloaded event (because we're not really unloading dyld) but we do get a loaded event (because the load address changed). This PR fixes the issue by relying on the modules set as the source of truth instead of relying on the event type.


Full diff: https://github.com/llvm/llvm-project/pull/140810.diff

1 Files Affected:

  • (modified) lldb/tools/lldb-dap/DAP.cpp (+4-9)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 3419b2c3a841b..f6241b7d98456 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -1292,15 +1292,7 @@ void DAP::EventThread() {
 
             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 (modules.contains(module_id)) {
               if (event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded) {
                 modules.erase(module_id);
                 reason = "removed";
@@ -1308,6 +1300,9 @@ void DAP::EventThread() {
               } else {
                 reason = "changed";
               }
+            } else {
+              modules.insert(module_id);
+              reason = "new";
             }
 
             llvm::json::Object body;

@JDevlieghere JDevlieghere merged commit 7b51339 into llvm:main May 21, 2025
13 checks passed
@JDevlieghere JDevlieghere deleted the lldb-dap-modules-event-bugfix-2 branch May 21, 2025 01:52
JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request May 22, 2025
I got a bug report where a pedantic DAP client complains about getting
two "new" module events for the same UUID. This is caused by the dyld
transition from the on-disk dyld to the shared cache dyld, which share
the same UUID. The transition is not generating an unloaded event
(because we're not really unloading dyld) but we do get a loaded event
(because the load address changed). This PR fixes the issue by relying
on the modules set as the source of truth instead of relying on the
event type.

Back-ported from commit 7b51339
JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request May 22, 2025
…ort-2

[lldb-dap] Avoid double 'new' events for dyld on Darwin (llvm#140810)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants