[lldb][Commands] Check that modules reports that they're loaded #66144

chelcassanova · 2023-09-12T21:15:16Z

This provides a test to ensure that when modules are loaded via the add-dsym command that they're loaded properly and broadcast the Target::eBroadcastBitSymbolsLoaded bit.

Also refactors Debugger::eBroadcastSymbolChange to Debugger::eBroadcastBitSymbolChanged to match the naming convention for other event bits used in the debugger.

llvmbot · 2023-09-12T21:16:24Z

@llvm/pr-subscribers-lldb

Changes

This exposes the `eBroadcastBitSymbolChange` bit to the SB API to show when a symbol change event has been broadcast as this wasn't being done before. Also refactors `eBroadcastSymbolChange` to `eBroadcastBitSymbolChange` to match the naming convention for other event bits used in the debugger. -- Full diff: https://github.com//pull/66144.diff

5 Files Affected:

(modified) lldb/include/lldb/API/SBDebugger.h (+2)
(modified) lldb/include/lldb/Core/Debugger.h (+1-1)
(modified) lldb/source/Commands/CommandObjectTarget.cpp (+4)
(modified) lldb/source/Core/Debugger.cpp (+2-2)
(modified) lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py (+24)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 29cf2c16fad4bd7..abb08e93d197a18 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -13,6 +13,7 @@
 
 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBPlatform.h"
+#include "lldb/API/SBStructuredData.h"
 
 namespace lldb_private {
 class CommandPluginInterfaceImplementation;
@@ -46,6 +47,7 @@ class LLDB_API SBDebugger {
       eBroadcastBitProgress = (1 << 0),
       eBroadcastBitWarning = (1 << 1),
       eBroadcastBitError = (1 << 2),
+      eBroadcastBitSymbolChange = (1 << 3),
   };
 
   SBDebugger();
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 5532cace606bfed..6cdf1a183b18e54 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -83,7 +83,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
     eBroadcastBitProgress = (1 << 0),
     eBroadcastBitWarning = (1 << 1),
     eBroadcastBitError = (1 << 2),
-    eBroadcastSymbolChange = (1 << 3),
+    eBroadcastBitSymbolChange = (1 << 3),
   };
 
   using DebuggerList = std::vector<lldb::DebuggerSP>;
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index 3e024ff91b382d7..8051b3826294ab3 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -4504,6 +4504,10 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
       if (process)
         process->Flush();
     }
+
+    if (result.Succeeded())
+      Debugger::ReportSymbolChange(module_spec);
+
     return result.Succeeded();
   }
 
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 7ec1efc64fe9383..924741f45040bf0 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1536,7 +1536,7 @@ void Debugger::ReportSymbolChange(const ModuleSpec &module_spec) {
     std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr);
     for (DebuggerSP debugger_sp : *g_debugger_list_ptr) {
       EventSP event_sp = std::make_shared<Event>(
-          Debugger::eBroadcastSymbolChange,
+          Debugger::eBroadcastBitSymbolChange,
           new SymbolChangeEventData(debugger_sp, module_spec));
       debugger_sp->GetBroadcaster().BroadcastEvent(event_sp);
     }
@@ -1844,7 +1844,7 @@ lldb::thread_result_t Debugger::DefaultEventHandler() {
 
   listener_sp->StartListeningForEvents(
       &m_broadcaster, eBroadcastBitProgress | eBroadcastBitWarning |
-                          eBroadcastBitError | eBroadcastSymbolChange);
+                          eBroadcastBitError | eBroadcastBitSymbolChange);
 
   // Let the thread that spawned us know that we have started up and that we
   // are now listening to all required events so no events get missed
diff --git a/lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py b/lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py
index d16ca32d79e6808..1d7caf3122dcccb 100644
--- a/lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py
+++ b/lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py
@@ -57,6 +57,30 @@ def test_add_dsym_with_dSYM_bundle(self):
         self.exe_name = "a.out"
         self.do_add_dsym_with_dSYM_bundle(self.exe_name)
 
+    @no_debug_info_test
+    def test_report_symbol_change(self):
+        """Test that when adding a symbol file, the eBroadcastBitSymbolChange event gets broadcasted."""
+        self.generate_main_cpp(version=1)
+        self.build(debug_info="dsym")
+
+        self.exe_name = "a.out"
+
+        # Get the broadcaster and listen for the symbol change event
+        self.broadcaster = self.dbg.GetBroadcaster()
+        self.listener = lldbutil.start_listening_from(
+            self.broadcaster, lldb.SBDebugger.eBroadcastBitSymbolChange
+        )
+
+        # Add the dSYM
+        self.do_add_dsym_with_success(self.exe_name)
+
+        # Get the next event
+        event = lldbutil.fetch_next_event(self, self.listener, self.broadcaster)
+
+        # Check that the event is valid
+        self.assertTrue(event.IsValid(), "Got a valid event.")
+
+
     def generate_main_cpp(self, version=0):
         """Generate main.cpp from main.cpp.template."""
         temp = os.path.join(self.getSourceDir(), self.template)

lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py

This exposes the `eBroadcastBitSymbolChange` bit to the SB API to show when a symbol change event has been broadcast as this wasn't being done before. Also refactors `eBroadcastSymbolChange` to `eBroadcastBitSymbolChange` to match the naming convention for other event bits used in the debugger.

clayborg

We should be able to extract the SBModuleSpec from the event in response to the eBroadcastBitSymbolChange event. This will require adding a static method to SBDebugger.h/.cpp to get the SBModuleSpec from the event:

  static lldb::SBModuleSpec
  GetModuleSpecFromEvent(const lldb::SBEvent &event);

You are passing a module specification to the Debugger::ReportSymbolChange(), so it should be easy to package this into a custom event and extract the data. Plenty of example of extracting data from events in SBDebugger, SBTarget and SBProcess

clayborg · 2023-09-12T22:11:10Z

lldb/include/lldb/Core/Debugger.h

@@ -83,7 +83,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
    eBroadcastBitProgress = (1 << 0),
    eBroadcastBitWarning = (1 << 1),
    eBroadcastBitError = (1 << 2),
-    eBroadcastSymbolChange = (1 << 3),
+    eBroadcastBitSymbolChange = (1 << 3),


This name led me to believe that a Symbol from a symbol table was changed, so it confused me just reading the code. Do we want this to be "eBroadcastBitSymbolFileAdded"?

If we change the name of the broadcast bit, we should also rename the SymbolChangeEventData.

Should it be SymbolFileAdded or SymbolFileChanged? You can (re)load a symbol file, which is something I've done with the JSON symbol file format.

Should it be SymbolFileAdded or SymbolFileChanged? You can (re)load a symbol file, which is something I've done with the JSON symbol file format.

I am fine with any name as long as it is clear that this is a symbol file change. The previous name led me to think a new symbol showed up in some symbol table. Luckily this enum is private, so we can rename as needed. But we should get the name as we want it in the SBDebugger.h file since we can't rename that after releasing it in the public API.

For clarity it can be renamed to eBroadcastBitSymbolFilesChanged in Debugger.h since we should remove this flag from the public view.

Yes, that's what Greg's suggesting 👍
PS: Probably just a typo, but it should be eBroadcastBitSymbolFileChanged (instead of Files).

Yes, I mistyped but fixed the typo in the fixup commit.

clayborg · 2023-09-12T22:11:30Z

lldb/include/lldb/API/SBDebugger.h

@@ -46,6 +47,7 @@ class LLDB_API SBDebugger {
      eBroadcastBitProgress = (1 << 0),
      eBroadcastBitWarning = (1 << 1),
      eBroadcastBitError = (1 << 2),
+      eBroadcastBitSymbolChange = (1 << 3),


See comment below about possible name change to "eBroadcastBitSymbolFileAdded"

clayborg · 2023-09-12T22:12:13Z

lldb/include/lldb/API/SBDebugger.h

@@ -13,6 +13,7 @@

 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBPlatform.h"
+#include "lldb/API/SBStructuredData.h"


This isn't needed in the header file right? Move to SBDebugger.cpp if it is still needed? No mention of SBStructuredData in SBDebugger.h

Correct, I left this in here by mistake.

jimingham · 2023-09-12T23:54:51Z

Okay, here's my understanding of what's going on here:

So what's going on here is that in order to support background fetching of dSYM's, there's a thread that calls dsymForUUID when someone tells it there's a binary with an unknown UUID, then when it gets the dSYM it needs to notify the system that the symbols for this module have changed.

It was inconvenient to call the Target::SymbolsDidLoad for all the targets from the background thread, so we instead made an event (eBroadcastSymbolChange, should be eBroadcastBitSymbolChange) whose whole reason for being is that its DoOnRemoval is the one that calls Target::SymbolsDidLoad, and then we can make a Listener that just polls continuously for this event. It is just pulling and discarding the event to cause SymbolsDidLoad, so it is really an internal event, it should not be exposed to other clients.

Instead, what will happen is that when the Debugger::eBroadcastBitSymbolChange event gets pulled from the event queue, it will run through the targets and call their SymbolsDidLoad if they have the UUID that just got loaded. Target::SymbolsDidLoad will in turn broadcast a Target::eBroadcastBitSymbolsLoaded event, that's the public one, and is correctly going to the Target's involved.

I think the only things that need doing here are:

Debugger::eBroadcastBitSymbolChange should NOT be public, it's internal to lldb and not useful to clients.
We should either have a Target::SymbolsDidChange that does the same work as Target::SymbolsDidLoad but sends Target::eBroadcastBitSymbolsChanged, or pass a bool in for changed vrs. load and dispatch the right event based on that.

Note, Jonas already added the Target::eBroadcastBitSymbolsChanged, but it wasn't getting dispatched because we didn't remember to tell ourselves the difference between Loaded and Changed.

clayborg · 2023-09-13T16:38:02Z

Okay, here's my understanding of what's going on here:

So what's going on here is that in order to support background fetching of dSYM's, there's a thread that calls dsymForUUID when someone tells it there's a binary with an unknown UUID, then when it gets the dSYM it needs to notify the system that the symbols for this module have changed.

It was inconvenient to call the Target::SymbolsDidLoad for all the targets from the background thread, so we instead made an event (eBroadcastSymbolChange, should be eBroadcastBitSymbolChange) whose whole reason for being is that its DoOnRemoval is the one that calls Target::SymbolsDidLoad, and then we can make a Listener that just polls continuously for this event. It is just pulling and discarding the event to cause SymbolsDidLoad, so it is really an internal event, it should not be exposed to other clients.

Instead, what will happen is that when the Debugger::eBroadcastBitSymbolChange event gets pulled from the event queue, it will run through the targets and call their SymbolsDidLoad if they have the UUID that just got loaded. Target::SymbolsDidLoad will in turn broadcast a Target::eBroadcastBitSymbolsLoaded event, that's the public one, and is correctly going to the Target's involved.

I think the only things that need doing here are:

Debugger::eBroadcastBitSymbolChange should NOT be public, it's internal to lldb and not useful to clients.

This should be documented in the Debugger.h header file next to the enum value to ensure. Might be good idea to start private event bits that never will go public at bit 31 and use the high bits of the bitfield so if we ever add a new event that will go into the public API, we don't have gaps. Though we can easily just change the internal bit if/when needed to not conflict with a new bit that might go public

We should either have a Target::SymbolsDidChange that does the same work as Target::SymbolsDidLoad but sends Target::eBroadcastBitSymbolsChanged, or pass a bool in for changed vrs. load and dispatch the right event based on that.

Does this API work for eBroadcastBitSymbolsLoaded:

  static lldb::SBModule lldb::SBTarget::GetModuleAtIndexFromEvent(const uint32_t idx, const lldb::SBEvent &event);

?

Note, Jonas already added the Target::eBroadcastBitSymbolsChanged, but it wasn't getting dispatched because we didn't remember to tell ourselves the difference between Loaded and Changed.

Fine with either adding or overloading this. I would be nice if the SBTarget::GetModuleAtIndexFromEvent() works for both of these?

clayborg · 2023-09-13T16:39:14Z

lldb/include/lldb/API/SBDebugger.h

@@ -46,6 +47,7 @@ class LLDB_API SBDebugger {
      eBroadcastBitProgress = (1 << 0),
      eBroadcastBitWarning = (1 << 1),
      eBroadcastBitError = (1 << 2),
+      eBroadcastBitSymbolChange = (1 << 3),


Sounds like this shouldn't be public per Jim's comments as the one from Debugger.h is designed to be private

Yes, as per Jim's comments this shouldn't be exposed anymore and clients should use the Target::eBroadcastBitSymbolsLoaded/Changed bits instead. Target::eBroadcastBitSymbolsChanged is in the Target header file but isn't exposed through the SB API and isn't sent using an event broadcast.

clayborg · 2023-09-13T16:40:37Z

lldb/include/lldb/Core/Debugger.h

@@ -83,7 +83,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
    eBroadcastBitProgress = (1 << 0),
    eBroadcastBitWarning = (1 << 1),
    eBroadcastBitError = (1 << 2),
-    eBroadcastSymbolChange = (1 << 3),
+    eBroadcastBitSymbolChange = (1 << 3),


Per Jim's comments this should be a private event bit that we don't want exposed in the public API. Best to rename to contain the work "Private" and possibly use bit 31:

eBroadcastBitPrivateSymbolChange = (1 << 31),

Would we want to both remove the bit from the SB API and use the higher-order bits for private events bits like this?

Yes, the SB API is public, so it shouldn't be there if it's private.

chelcassanova · 2023-09-13T20:13:01Z

lldb/source/Target/Target.cpp

@@ -66,6 +66,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SetVector.h"

+#include <_types/_uint32_t.h>


This was included by mistake

chelcassanova · 2023-09-13T21:55:09Z

Does this API work for eBroadcastBitSymbolsLoaded:
  static lldb::SBModule lldb::SBTarget::GetModuleAtIndexFromEvent(const uint32_t idx, const lldb::SBEvent &event);
?

Note, Jonas already added the Target::eBroadcastBitSymbolsChanged, but it wasn't getting dispatched because we didn't remember to tell ourselves the difference between Loaded and Changed.

Fine with either adding or overloading this. I would be nice if the SBTarget::GetModuleAtIndexFromEvent() works for both of these?

I can add this to the test as adding the bit shouldn't have changed the event_data.

clayborg · 2023-09-14T02:44:44Z

lldb/include/lldb/Core/Debugger.h

@@ -83,7 +83,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
    eBroadcastBitProgress = (1 << 0),
    eBroadcastBitWarning = (1 << 1),
    eBroadcastBitError = (1 << 2),
-    eBroadcastSymbolChange = (1 << 3),
+    eBroadcastBitSymbolFileChange = (1 << 3),


eBroadcastBitSymbolFileChanged? Add a 'd'?

clayborg · 2023-09-14T02:45:46Z

lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py

+        listener.WaitForEvent(1, event)
+
+        # Check that the event is valid
+        self.assertTrue(event.IsValid(), "Got a valid eBroadcastBitSymbolsLoaded event.")


Can we test that we can extract the SBModuleSpec for the module that changed from this event?

We can get the SBModules from the event using SBTarget::GetModuleAtIndexFromEvent and SBTarget::GetNumModulesFromEvent, unfortunately there's no way to get an SBModuleSpec from an SBModule.

We can get the SBModules from the event using SBTarget::GetModuleAtIndexFromEvent and SBTarget::GetNumModulesFromEvent, unfortunately there's no way to get an SBModuleSpec from an SBModule.

SBModule is fine!

Refactors `eBroadcastBitSymbolChange` to `eBroadcastBitSymbolChanged`

chelcassanova requested a review from a team as a code owner September 12, 2023 21:15

llvmbot added the lldb label Sep 12, 2023

JDevlieghere reviewed Sep 12, 2023

View reviewed changes

lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py Outdated Show resolved Hide resolved

JDevlieghere approved these changes Sep 12, 2023

View reviewed changes

clayborg reviewed Sep 12, 2023

View reviewed changes

chelcassanova force-pushed the symbol-change-event branch from 1a544b9 to 8754d93 Compare September 12, 2023 22:25

clayborg reviewed Sep 13, 2023

View reviewed changes

fixup! [lldb][Commands] Show symbol change bit in SB API

0ae976f

chelcassanova commented Sep 13, 2023

View reviewed changes

fixup! fixup! [lldb][Commands] Show symbol change bit in SB API

e39095b

clayborg reviewed Sep 14, 2023

View reviewed changes

chelcassanova added 2 commits September 14, 2023 13:34

fixup! fixup! fixup! [lldb][Commands] Show symbol change bit in SB API

677f4b5

[lldb][Commands] Refactor broadcast bit name

9d0a3ed

Refactors `eBroadcastBitSymbolChange` to `eBroadcastBitSymbolChanged`

chelcassanova changed the title ~~[lldb][Commands] Show symbol change bit in SB API~~ [lldb][Commands] Check that modules reports that they're loaded Sep 25, 2023

[lldb][Commands] Check that modules reports that they're loaded #66144

Are you sure you want to change the base?

[lldb][Commands] Check that modules reports that they're loaded #66144

Conversation

chelcassanova commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 12, 2023

Uh oh!

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimingham commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clayborg commented Sep 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chelcassanova commented Sep 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chelcassanova commented Sep 12, 2023 •

edited

Loading

jimingham commented Sep 12, 2023 •

edited

Loading