Skip to content

[lldb] Call Target::ClearAllLoadedSections even earlier #140228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 22, 2025
Merged

Conversation

labath
Copy link
Collaborator

@labath labath commented May 16, 2025

This reapplies #138892, which was reverted in
5fb9dca due to failures on windows.

Windows loads modules from the Process class, and it does that quite
early, and it kinda makes sense which is why I'm moving the clearing
code even earlier.

The original commit message was:

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.

labath added 2 commits May 16, 2025 11:22
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 llvm#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.
This reapplies llvm#138892, which was reverted in
5fb9dca due to failures on windows.

Windows loads modules from the Process class, and it does that quite
early, and it kinda makes sense which is why I'm moving the clearing
code even earlier.
@labath labath requested a review from jasonmolenda May 16, 2025 09:40
@labath labath requested a review from JDevlieghere as a code owner May 16, 2025 09:40
@llvmbot llvmbot added the lldb label May 16, 2025
@llvmbot
Copy link
Member

llvmbot commented May 16, 2025

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

This reapplies #138892, which was reverted in
5fb9dca due to failures on windows.

Windows loads modules from the Process class, and it does that quite
early, and it kinda makes sense which is why I'm moving the clearing
code even earlier.

The original commit message was:

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/140228.diff

3 Files Affected:

  • (modified) lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp (-1)
  • (modified) lldb/source/Target/Process.cpp (+3)
  • (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 578ab12268ea3..1270d57423c7b 100644
--- a/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
+++ b/lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
@@ -872,7 +872,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..c377feec86c16 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2675,6 +2675,7 @@ Status Process::LaunchPrivate(ProcessLaunchInfo &launch_info, StateType &state,
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
   m_os_up.reset();
+  GetTarget().ClearAllLoadedSections();
 
   {
     std::lock_guard<std::mutex> guard(m_process_input_reader_mutex);
@@ -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(
@@ -2984,6 +2986,7 @@ Status Process::Attach(ProcessAttachInfo &attach_info) {
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
   m_os_up.reset();
+  GetTarget().ClearAllLoadedSections();
 
   lldb::pid_t attach_pid = attach_info.GetProcessID();
   Status error;
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

Copy link
Collaborator

@jasonmolenda jasonmolenda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@labath labath merged commit 53a5bea into llvm:main May 22, 2025
12 checks passed
@labath labath deleted the clear branch May 22, 2025 12:20
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025
This reapplies llvm#138892, which
was reverted in

llvm@5fb9dca
due to failures on windows.

Windows loads modules from the Process class, and it does that quite
early, and it kinda makes sense which is why I'm moving the clearing
code even earlier.

The original commit message was:

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
llvm#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.
ajaden-codes pushed a commit to Jaddyen/llvm-project that referenced this pull request Jun 6, 2025
This reapplies llvm#138892, which
was reverted in

llvm@5fb9dca
due to failures on windows.

Windows loads modules from the Process class, and it does that quite
early, and it kinda makes sense which is why I'm moving the clearing
code even earlier.

The original commit message was:

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
llvm#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants