[lldb] Call Target::ClearAllLoadedSections earlier

[labath]

Minidump files contain explicit information about load addresses of modules, so it can load them itself. This works on other platforms, but fails on darwin because DynamicLoaderDarwin nukes the loaded module list on initialization (which happens after the core file plugin has done its work).

This used to work until #109477, which enabled the dynamic loader plugins for minidump files in order to get them to provide access to TLS.

Clearing the load list makes sense, but I think we could do it earlier in the process, so that both Process and DynamicLoader plugins get a chance to load modules. This patch does that by calling the function early in the launch/attach/load core flows.

This fixes TestDynamicValue.py:test_from_core_file on darwin.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

Minidump files contain explicit information about load addresses of modules, so it can load them itself. This works on other platforms, but fails on darwin because DynamicLoaderDarwin nukes the loaded module list on initialization (which happens after the core file plugin has done its work).

This used to work until #109477, which enabled the dynamic loader plugins for minidump files in order to get them to provide access to TLS.

Clearing the load list makes sense, but I think we could do it earlier in the process, so that both Process and DynamicLoader plugins get a chance to load modules. This patch does that by calling the function early in the launch/attach/load core flows.

This fixes TestDynamicValue.py:test_from_core_file on darwin.

---

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

3 Files Affected:

• (modified) lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp (-1)
• (modified) lldb/source/Target/Process.cpp (+4)
• (modified) lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py (-1)

diff --git a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
index e25c4ff55e408..8bf01aa168342 100644
--- a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
+++ b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
@@ -871,7 +871,6 @@ void DynamicLoaderDarwin::PrivateInitialize(Process *process) {
                StateAsCString(m_process->GetState()));
   Clear(true);
   m_process = process;
-  m_process->GetTarget().ClearAllLoadedSections();
 }
 
 // Member function that gets called when the process state changes.
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 13ff12b4ff953..7c5512598bbb6 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2763,6 +2763,7 @@ Status Process::LaunchPrivate(ProcessLaunchInfo &launch_info, StateType &state,
   }
 
   if (state == eStateStopped || state == eStateCrashed) {
+    GetTarget().ClearAllLoadedSections();
     DidLaunch();
 
     // Now that we know the process type, update its signal responses from the
@@ -2799,6 +2800,7 @@ Status Process::LaunchPrivate(ProcessLaunchInfo &launch_info, StateType &state,
 }
 
 Status Process::LoadCore() {
+  GetTarget().ClearAllLoadedSections();
   Status error = DoLoadCore();
   if (error.Success()) {
     ListenerSP listener_sp(
@@ -3094,6 +3096,8 @@ void Process::CompleteAttach() {
   Log *log(GetLog(LLDBLog::Process | LLDBLog::Target));
   LLDB_LOGF(log, "Process::%s()", __FUNCTION__);
 
+  GetTarget().ClearAllLoadedSections();
+
   // Let the process subclass figure out at much as it can about the process
   // before we go looking for a dynamic loader plug-in.
   ArchSpec process_arch;
diff --git a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
index cd95a9ff3fe8c..faa35421ff60b 100644
--- a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
+++ b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
@@ -282,7 +282,6 @@ def test_from_forward_decl(self):
 
     @no_debug_info_test
     @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24663")
-    @expectedFailureDarwin  # dynamic loader unloads modules
     @expectedFailureAll(archs=["arm"]) # Minidump saving not implemented
     def test_from_core_file(self):
         """Test fetching C++ dynamic values from core files. Specifically, test

[jasonmolenda]

Looks good to me, we should clear them on both attach & launch which is what the Darwin DynamicLoader plugin was doing.

I was a little surprised that Minidump is picking up the host native DyanmicLoader plugin at all - seems like the static dynamic loader might make more sense, so it can say what binaries are loaded & where they're loaded - but I've never looked at the Minidump code, I'm sure there are reasons why it works this way.

[labath]

Looks good to me, we should clear them on both attach & launch which is what the Darwin DynamicLoader plugin was doing.

I was a little surprised that Minidump is picking up the host native DyanmicLoader plugin at all - seems like the static dynamic loader might make more sense, so it can say what binaries are loaded & where they're loaded - but I've never looked at the Minidump code, I'm sure there are reasons why it works this way.

It originally did that, but then this changed last year in order to get access to thread-local data, which is also a responsibility of the dynamic loader class. At least for posix systems (I'm not sure about Darwin, as the dynamic loader is more tightly integrated into the OS) it also kind of makes sense because the dynamic loader can work off of an (elf) core file (if it contains enough information). For minidumps, some of that work is wasted though, because the minidump contains explicit load addresses of all modules. So, it would kind of make sense to separate the logic for loading the modules from "loading of TLS", but it's not quite clear how to do that as two are quite intertwined.

[labath]

Yeah, this is kind of my fault as I introduced this way back when. I think at that point we didn't have test-specific headers, which made this option more appealing.