[lldb][DynamicLoader] Fix lldb unable to stop at _dl_debug_state if user set it before the process launched. #88792

ZequanWu · 2024-04-15T20:43:04Z

If user sets a breakpoint at _dl_debug_state before the process launched, the breakpoint is not resolved yet. When lldb loads dynamic loader module, it's created with Target::GetOrCreateModule which notifies any pending breakpoint to resolve. However, the module's sections are not loaded at this time. They are loaded after returned from Target::GetOrCreateModule. This change fixes it by manually resolving breakpoints after creating dynamic loader module.

…ser set it before the process launched.

llvmbot · 2024-04-15T20:43:39Z

@llvm/pr-subscribers-lldb

Author: Zequan Wu (ZequanWu)

Changes

If user sets a breakpoint at _dl_debug_state before the process launched, the breakpoint is not resolved yet. When lldb loads dynamic loader module, it's created with Target::GetOrCreateModule which notifies any pending breakpoint to resolve. However, the module's sections are not loaded at this time. They are loaded after returned from Target::GetOrCreateModule. This change fixes it by manually resolving breakpoints after creating dynamic loader module.

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

4 Files Affected:

(modified) lldb/include/lldb/Target/Target.h (+3)
(modified) lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp (+5)
(modified) lldb/source/Target/Target.cpp (+10-7)
(modified) lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py (+17)

diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 2c2e6b2831ccee..3554ef0ea5a140 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -878,6 +878,9 @@ class Target : public std::enable_shared_from_this<Target>,
   // the address of its previous instruction and return that address.
   lldb::addr_t GetBreakableLoadAddress(lldb::addr_t addr);
 
+  void UpdateBreakpoints(ModuleList &module_list, bool added,
+                         bool delete_locations);
+
   void ModulesDidLoad(ModuleList &module_list);
 
   void ModulesDidUnload(ModuleList &module_list, bool delete_locations);
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
index 9baf86da4dc799..901fa53682da8e 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -576,6 +576,11 @@ ModuleSP DynamicLoaderPOSIXDYLD::LoadInterpreterModule() {
     UpdateLoadedSections(module_sp, LLDB_INVALID_ADDRESS, m_interpreter_base,
                          false);
     m_interpreter_module = module_sp;
+    // Manually update breakpoints after updating loaded sections, because they
+    // cannot be resolve at the time when creating this module.
+    ModuleList module_list;
+    module_list.Append(module_sp);
+    m_process->GetTarget().UpdateBreakpoints(module_list, true, false);
     return module_sp;
   }
   return nullptr;
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 09b0ac42631d1a..ec2dea5cc238d3 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1687,6 +1687,13 @@ void Target::NotifyModulesRemoved(lldb_private::ModuleList &module_list) {
   ModulesDidUnload(module_list, false);
 }
 
+void Target::UpdateBreakpoints(ModuleList &module_list, bool added,
+                               bool delete_locations) {
+  m_breakpoint_list.UpdateBreakpoints(module_list, added, delete_locations);
+  m_internal_breakpoint_list.UpdateBreakpoints(module_list, added,
+                                               delete_locations);
+}
+
 void Target::ModulesDidLoad(ModuleList &module_list) {
   const size_t num_images = module_list.GetSize();
   if (m_valid && num_images) {
@@ -1694,8 +1701,7 @@ void Target::ModulesDidLoad(ModuleList &module_list) {
       ModuleSP module_sp(module_list.GetModuleAtIndex(idx));
       LoadScriptingResourceForModule(module_sp, this);
     }
-    m_breakpoint_list.UpdateBreakpoints(module_list, true, false);
-    m_internal_breakpoint_list.UpdateBreakpoints(module_list, true, false);
+    UpdateBreakpoints(module_list, true, false);
     if (m_process_sp) {
       m_process_sp->ModulesDidLoad(module_list);
     }
@@ -1713,8 +1719,7 @@ void Target::SymbolsDidLoad(ModuleList &module_list) {
       }
     }
 
-    m_breakpoint_list.UpdateBreakpoints(module_list, true, false);
-    m_internal_breakpoint_list.UpdateBreakpoints(module_list, true, false);
+    UpdateBreakpoints(module_list, true, false);
     auto data_sp =
         std::make_shared<TargetEventData>(shared_from_this(), module_list);
     BroadcastEvent(eBroadcastBitSymbolsLoaded, data_sp);
@@ -1727,9 +1732,7 @@ void Target::ModulesDidUnload(ModuleList &module_list, bool delete_locations) {
     auto data_sp =
         std::make_shared<TargetEventData>(shared_from_this(), module_list);
     BroadcastEvent(eBroadcastBitModulesUnloaded, data_sp);
-    m_breakpoint_list.UpdateBreakpoints(module_list, false, delete_locations);
-    m_internal_breakpoint_list.UpdateBreakpoints(module_list, false,
-                                                 delete_locations);
+    UpdateBreakpoints(module_list, false, delete_locations);
 
     // If a module was torn down it will have torn down the 'TypeSystemClang's
     // that we used as source 'ASTContext's for the persistent variables in
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py b/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
index ea242952e409ec..a7e23fae14a21f 100644
--- a/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -671,3 +671,20 @@ def test_breakpoint_statistics_hitcount(self):
         self.assertNotEqual(breakpoints_stats, None)
         for breakpoint_stats in breakpoints_stats:
             self.assertIn("hitCount", breakpoint_stats)
+
+    @skipIf(oslist=no_match(["linux"]))
+    def test_break_at__dl_debug_state(self):
+        """
+        Test lldb is able to stop at _dl_debug_state if it is set before the
+        process is launched.
+        """
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("target create %s" % exe)
+        bpid = lldbutil.run_break_set_by_symbol(self, "_dl_debug_state",
+                                                num_expected_locations=0)
+        self.runCmd("run")
+        self.assertIsNotNone(
+            lldbutil.get_one_thread_stopped_at_breakpoint_id(self.process(),
+                                                             bpid)
+        )

github-actions · 2024-04-15T20:45:50Z

✅ With the latest revision this PR passed the Python code formatter.

jimingham · 2024-04-15T21:22:08Z

When a shared library gets loaded, its sections have to have been loaded before asking the breakpoint system to try to set new breakpoints. Presumably this is how it happens for all the other shared library loads, or breakpoints in shared libraries wouldn't work.

I'm missing why the _dl_debug_state breakpoint is special here, such that it requires a force load of the section info? How does that happen?

jasonmolenda · 2024-04-15T22:15:08Z

Skimming DynamicLoaderPOSIXDYLD (as it is written today), it looks like attach/launch call SetRendezvousBreakpoint() which (1) loads ld.so into lldb if it isn't already there, and (2) sets the breakpoint to be notified about future binaries loading. It loads ld.so via LoadInterpreterModule which Finds It somehow (I stopped reading), adds it to the Target, and sets the section load addresses, returns the module_sp. Then it sets breakpoints on a half dozen notification function names in the ld.so Module. This is all special-cased for ld.so, and we do not evaluate user breakpoints because we didn't call Target::ModulesDidLoad for ld.so.

The code path for other binaries goes through DynamicLoaderPOSIXDYLD::RefreshModules which does the normal approach of adding binaries to the Target, setting the load addresses, then calling ModulesDidLoad which will evaluate breakpoints that may need to be set in the new binaries.

It seems like a simpler fix would be to have DynamicLoaderPOSIXDYLD::LoadInterpreterModule create a ModuleList with the ModuleSP of ld.so that it has just added to Target and set its section load addresses, then call ModulesDidLoad on it so that user breakpoints are evaluated and inserted. The patch as it is written right now is taking one part of ModulesDidLoad and putting it in a separate method that DynamicLoaderPOSIXDYLD::LoadInterpreterModule is calling -- why not just call ModulesDidLoad and delegate this (and possibly other binary-just-loaded) behaviors to it?

ZequanWu · 2024-04-16T15:25:36Z

why not just call ModulesDidLoad and delegate this (and possibly other binary-just-loaded) behaviors to it?

That's what I did first, but it breaks the test TestModuleLoadedNotifys.ModuleLoadedNotifysTestCase.test_launch_notifications because of extra broadcaster event. So, I moved out the part just updating breakpoints.

ZequanWu · 2024-04-16T15:28:18Z

I'm missing why the _dl_debug_state breakpoint is special here, such that it requires a force load of the section info? How does that happen?

Jason's comment explains it well. It's because ld.so's loading is special here. Normal binaries are loaded via DynamicLoaderPOSIXDYLD::RefreshModules which updates loaded section addresses.

labath · 2024-04-16T15:44:27Z

why not just call ModulesDidLoad and delegate this (and possibly other binary-just-loaded) behaviors to it?

That's what I did first, but it breaks the test TestModuleLoadedNotifys.ModuleLoadedNotifysTestCase.test_launch_notifications because of extra broadcaster event. So, I moved out the part just updating breakpoints.

The point of the test is to ensure you don't get a hundred notifications, one for each module. Having one notification for ld.so, and 99 for the rest of the modules is ok. It should be fine to just update the test to match new reality.

ZequanWu · 2024-04-16T18:16:22Z

why not just call ModulesDidLoad and delegate this (and possibly other binary-just-loaded) behaviors to it?

That's what I did first, but it breaks the test TestModuleLoadedNotifys.ModuleLoadedNotifysTestCase.test_launch_notifications because of extra broadcaster event. So, I moved out the part just updating breakpoints.

The point of the test is to ensure you don't get a hundred notifications, one for each module. Having one notification for ld.so, and 99 for the rest of the modules is ok. It should be fine to just update the test to match new reality.

Updated to use Target::ModulesDisLoad and the test.

jasonmolenda

This looks good, thanks for the revisions.

labath · 2024-04-17T12:31:21Z

why not just call ModulesDidLoad and delegate this (and possibly other binary-just-loaded) behaviors to it?

That's what I did first, but it breaks the test TestModuleLoadedNotifys.ModuleLoadedNotifysTestCase.test_launch_notifications because of extra broadcaster event. So, I moved out the part just updating breakpoints.

The point of the test is to ensure you don't get a hundred notifications, one for each module. Having one notification for ld.so, and 99 for the rest of the modules is ok. It should be fine to just update the test to match new reality.

Updated to use Target::ModulesDisLoad and the test.

Oh, so the problem is that the loaded event gets sent twice (and not that it gets sent as a separate event, as I originally thought). While it's not the end of the world (it appears the mac loader does something similar as well), this does seem like something that we could prevent. Can you check where does the second event get sent from? Is it by any chance when we go through this place ? If so, it should be fairly easy to refactor that code to avoid putting the interpreter module into the event list when it is already loaded.

ZequanWu · 2024-04-17T15:28:59Z

Can you check where does the second event get sent from?

The first event is sent when creating the module for ld.so:

llvm-project/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

Line 574 in 458328a

if (ModuleSP module_sp = target.GetOrCreateModule(module_spec,

. It gets notified at here:

llvm-project/lldb/source/Target/Target.cpp

Line 1660 in 458328a

ModulesDidLoad(my_module_list);

. The second event is sent because I explicitly calls ModulesDidLoad in this change (the current version) after the creating the ld.so module.

Is it by any chance when we go through this place ?

It doesn't go through that place when loading ld.so module. That's for other binaries (non-dynamic-linker binaries).

labath · 2024-04-17T17:04:43Z

So, could the fix be as simple as passing notify=false in the first call ?

ZequanWu · 2024-04-17T17:13:07Z

So, could the fix be as simple as passing notify=false in the first call ?

Yeah, absolutely. Updated.

github-actions · 2024-04-17T17:15:50Z

✅ With the latest revision this PR passed the C/C++ code formatter.

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

[lldb][DynamicLoader] Fix lldb unable to stop at _dl_debug_state if u…

26528cd

…ser set it before the process launched.

ZequanWu requested review from labath and jasonmolenda April 15, 2024 20:43

ZequanWu requested a review from JDevlieghere as a code owner April 15, 2024 20:43

llvmbot added the lldb label Apr 15, 2024

ZequanWu added 2 commits April 16, 2024 11:45

format

29d5861

Update to use Target::ModulesDidLoad.

735b8d2

jasonmolenda approved these changes Apr 16, 2024

View reviewed changes

disable first notification

7cf2628

labath reviewed Apr 17, 2024

View reviewed changes

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp Outdated Show resolved Hide resolved

labath approved these changes Apr 17, 2024

View reviewed changes

address comments

407b6df

ZequanWu merged commit 60b90b5 into llvm:main Apr 17, 2024

ZequanWu deleted the break-_dl_debug_state branch May 14, 2024 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb][DynamicLoader] Fix lldb unable to stop at _dl_debug_state if user set it before the process launched. #88792

[lldb][DynamicLoader] Fix lldb unable to stop at _dl_debug_state if user set it before the process launched. #88792

Uh oh!

ZequanWu commented Apr 15, 2024

Uh oh!

llvmbot commented Apr 15, 2024

Uh oh!

github-actions bot commented Apr 15, 2024 •

edited

Loading

Uh oh!

jimingham commented Apr 15, 2024

Uh oh!

jasonmolenda commented Apr 15, 2024

Uh oh!

ZequanWu commented Apr 16, 2024 •

edited

Loading

Uh oh!

ZequanWu commented Apr 16, 2024

Uh oh!

labath commented Apr 16, 2024

Uh oh!

ZequanWu commented Apr 16, 2024

Uh oh!

jasonmolenda left a comment

Uh oh!

labath commented Apr 17, 2024 •

edited

Loading

Uh oh!

ZequanWu commented Apr 17, 2024 •

edited

Loading

Uh oh!

labath commented Apr 17, 2024

Uh oh!

ZequanWu commented Apr 17, 2024

Uh oh!

github-actions bot commented Apr 17, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

[lldb][DynamicLoader] Fix lldb unable to stop at _dl_debug_state if user set it before the process launched. #88792

[lldb][DynamicLoader] Fix lldb unable to stop at _dl_debug_state if user set it before the process launched. #88792

Uh oh!

Conversation

ZequanWu commented Apr 15, 2024

Uh oh!

llvmbot commented Apr 15, 2024

Uh oh!

github-actions bot commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimingham commented Apr 15, 2024

Uh oh!

jasonmolenda commented Apr 15, 2024

Uh oh!

ZequanWu commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZequanWu commented Apr 16, 2024

Uh oh!

labath commented Apr 16, 2024

Uh oh!

ZequanWu commented Apr 16, 2024

Uh oh!

jasonmolenda left a comment

Choose a reason for hiding this comment

Uh oh!

labath commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZequanWu commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labath commented Apr 17, 2024

Uh oh!

ZequanWu commented Apr 17, 2024

Uh oh!

github-actions bot commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 15, 2024 •

edited

Loading

ZequanWu commented Apr 16, 2024 •

edited

Loading

labath commented Apr 17, 2024 •

edited

Loading

ZequanWu commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading