[Lldb-commits] [lldb] fix parallel module loading deadlock for Linux DYLD (PR #166480)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Nov 5 11:21:41 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Tom Yang (zhyty)
<details>
<summary>Changes</summary>
Another attempt at resolving the deadlock issue @<!-- -->GeorgeHuyubo discovered (his previous [attempt](https://github.com/llvm/llvm-project/pull/160225)).
This change can be summarized as the following:
* Plumb through a boolean flag to force no preload in `GetOrCreateModules` all the way through to `LoadModuleAtAddress`.
* Parallelize `Module::PreloadSymbols` separately from `Target::GetOrCreateModule` and its caller `LoadModuleAtAddress` (this is what avoids the deadlock).
These changes roughly maintain the performance characteristics of the previous implementation of parallel module loading. Testing on targets with between 5000 and 14000 modules, I saw similar numbers as before, often more than 10% faster in the new implementation across multiple trials for these massive targets. I think it's because we have less lock contention with this approach.
# The deadlock
As @<!-- -->GeorgeHuyubo explains in his [PR](https://github.com/llvm/llvm-project/pull/160225), the deadlock occurs from an ABBA deadlock that happens when a thread context-switches out of `Module::PreloadSymbols`, goes into `Target::GetOrCreateModule` for another module, possibly entering this block:
```
if (!module_sp) {
// The platform is responsible for finding and caching an appropriate
// module in the shared module cache.
if (m_platform_sp) {
error = m_platform_sp->GetSharedModule(
module_spec, m_process_sp.get(), module_sp, &search_paths,
&old_modules, &did_create_module);
} else {
error = Status::FromErrorString("no platform is currently set");
}
}
```
`Module::PreloadSymbols` holds a module-level mutex, and then `GetSharedModule` *attempts* to hold the mutex of the global shared `ModuleList`. So, this thread holds the module mutex, and waits on the global shared `ModuleList` mutex.
A competing thread may execute `Target::GetOrCreateModule`, enter the same block as above, grabbing the global shared `ModuleList` mutex. Then, in `ModuleList::GetSharedModule`, we eventually call `ModuleList::FindModules` which eventually waits for the `Module` mutex held by the first thread (via `Module::GetUUID`). Thus, we deadlock.
## Reproducing the deadlock
It might be worth noting that I've never been able to observe this deadlock issue during live debugging (e.g. launching or attaching to processes), however we were able to consistently reproduce this issue with coredumps when using the following settings:
```
(lldb) settings set target.parallel-module-load true
(lldb) settings set target.preload-symbols true
(lldb) settings set symbols.load-on-demand false
(lldb) target create --core /some/core/file/here
# deadlock happens
```
## How this change avoids this deadlock
This change avoids concurrent executions of `Module::PreloadSymbols` with `Target::GetOrCreateModule` by waiting until after the `Target::GetOrCreateModule` executions to run `Module::PreloadSymbols` in parallel. This avoids the ordering of holding a Module lock *then* the ModuleList lock, as `Target::GetOrCreateModule` executions maintain the ordering of the shared ModuleList lock first (from what I've read and tested).
## Why not read-write lock?
Some feedback in https://github.com/llvm/llvm-project/pull/160225 was to modify mutexes used in these components with read-write locks. This might be a good idea overall, but I don't think it would *easily* resolve this specific deadlock. `Module::PreloadSymbols` would probably need a write lock to Module, so even if we had a read lock in `Module::GetUUID` we would still contend. Maybe the `ModuleList` lock could be a read lock that converts to a write lock if it chooses to update the module, but it seems likely that some thread would try to update the shared module list and then the write lock would contend again.
Perhaps with deeper architectural changes, we could fix this issue?
# Other attempts
One downside of this approach (and the former approach of parallel module loading) is that each DYLD would need to implement this pattern themselves. With @<!-- -->clayborg's help, I looked at a few other approaches:
* In `Target::GetOrCreateModule`, backgrounding the `Module::PreloadSymbols` call by adding it directly to the thread pool via `Debugger::GetThreadPool().async()`. This required adding a lock to `Module::SetLoadAddress` (probably should be one there already) since `ObjectFileELF::SetLoadAddress` is not thread-safe (updates sections). Unfortunately, during execution, this causes the preload symbols to run synchronously with `Target::GetOrCreateModule`, preventing us from truly parallelizing the execution.
* In `Module::PreloadSymbols`, backgrounding the `symtab` and `sym_file` `PreloadSymbols` calls individually, but similar issues as the above.
* Passing a callback function like https://github.com/swiftlang/llvm-project/pull/10746 instead of the boolean I use in this change. It's functionally the same change IMO, with some design tradeoffs:
* Pro: the caller doesn't need to explicitly call `Module::PreloadSymbols` itself, and can instead call whatever function is passed into the callback.
* Con: the caller needs to delay the execution of the callback such that it occurs after the `GetOrCreateModule` logic, otherwise we run into the same issue. I thought this would be trickier for the caller, requiring some kinda condition variable or otherwise storing the calls to execute afterwards.
# Test Plan:
```
ninja check-lldb
```
---
Full diff: https://github.com/llvm/llvm-project/pull/166480.diff
10 Files Affected:
- (modified) lldb/include/lldb/Core/ModuleList.h (+3)
- (modified) lldb/include/lldb/Target/DynamicLoader.h (+15-2)
- (modified) lldb/include/lldb/Target/Target.h (+9-1)
- (modified) lldb/source/Core/DynamicLoader.cpp (+8-4)
- (modified) lldb/source/Core/ModuleList.cpp (+13)
- (modified) lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp (+25-15)
- (modified) lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h (+5-4)
- (modified) lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp (+4-2)
- (modified) lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h (+5-4)
- (modified) lldb/source/Target/Target.cpp (+5-3)
``````````diff
diff --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h
index e71f3b2bad6b4..8e87a2cb01df6 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -511,6 +511,9 @@ class ModuleList {
/// Atomically swaps the contents of this module list with \a other.
void Swap(ModuleList &other);
+ /// For each module in this ModuleList, preload its symbols.
+ void PreloadSymbols() const;
+
protected:
// Class typedefs.
typedef std::vector<lldb::ModuleSP>
diff --git a/lldb/include/lldb/Target/DynamicLoader.h b/lldb/include/lldb/Target/DynamicLoader.h
index 75bb6cb6bb907..271d5f761696c 100644
--- a/lldb/include/lldb/Target/DynamicLoader.h
+++ b/lldb/include/lldb/Target/DynamicLoader.h
@@ -207,10 +207,17 @@ class DynamicLoader : public PluginInterface {
/// resulting module at the virtual base address \p base_addr.
/// Note that this calls Target::GetOrCreateModule with notify being false,
/// so it is necessary to call Target::ModulesDidLoad afterwards.
+ ///
+ /// \param[in] defer_module_preload
+ /// If the module needs to be loaded, prevent the module from being
+ /// preloaded even if the user sets the preload symbols option. This is
+ /// used as a performance optimization should the caller preload the
+ /// modules in parallel after calling this function.
virtual lldb::ModuleSP LoadModuleAtAddress(const lldb_private::FileSpec &file,
lldb::addr_t link_map_addr,
lldb::addr_t base_addr,
- bool base_addr_is_offset);
+ bool base_addr_is_offset,
+ bool defer_module_preload = false);
/// Find/load a binary into lldb given a UUID and the address where it is
/// loaded in memory, or a slide to be applied to the file address.
@@ -352,7 +359,13 @@ class DynamicLoader : public PluginInterface {
protected:
// Utility methods for derived classes
- lldb::ModuleSP FindModuleViaTarget(const FileSpec &file);
+ /// Find a module in the target that matches the given file.
+ ///
+ /// \param[in] defer_module_preload
+ /// If the module needs to be loaded, prevent the module from being
+ /// preloaded even if the user sets the preload symbols option.
+ lldb::ModuleSP FindModuleViaTarget(const FileSpec &file,
+ bool defer_module_preload = false);
/// Checks to see if the target module has changed, updates the target
/// accordingly and returns the target executable module.
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index c375df248154f..af755508cbe74 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -649,12 +649,20 @@ class Target : public std::enable_shared_from_this<Target>,
/// will handle / summarize the failures in a custom way and
/// don't use these messages.
///
+ /// \param[in] defer_preload
+ /// If true, the module will not be preloaded even if
+ /// Target::GetPreloadSymbols() is true. This is useful when the caller
+ /// wishes to preload loaded modules in parallel after calling this
+ /// function for better performance. This is because it's currently not
+ /// thread-safe to do so during the execution of this function.
+ ///
/// \return
/// An empty ModuleSP will be returned if no matching file
/// was found. If error_ptr was non-nullptr, an error message
/// will likely be provided.
lldb::ModuleSP GetOrCreateModule(const ModuleSpec &module_spec, bool notify,
- Status *error_ptr = nullptr);
+ Status *error_ptr = nullptr,
+ bool defer_preload = false);
// Settings accessors
diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp
index 7580b15c02ce1..d1f2ae33af75e 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -154,7 +154,8 @@ DynamicLoader::GetSectionListFromModule(const ModuleSP module) const {
return sections;
}
-ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file) {
+ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file,
+ bool defer_module_preload) {
Target &target = m_process->GetTarget();
ModuleSpec module_spec(file, target.GetArchitecture());
if (UUID uuid = m_process->FindModuleUUID(file.GetPath())) {
@@ -165,7 +166,9 @@ ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file) {
if (ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec))
return module_sp;
- if (ModuleSP module_sp = target.GetOrCreateModule(module_spec, false))
+ if (ModuleSP module_sp = target.GetOrCreateModule(
+ module_spec, false /* notify */, nullptr /* error_ptr */,
+ defer_module_preload))
return module_sp;
return nullptr;
@@ -174,8 +177,9 @@ ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file) {
ModuleSP DynamicLoader::LoadModuleAtAddress(const FileSpec &file,
addr_t link_map_addr,
addr_t base_addr,
- bool base_addr_is_offset) {
- if (ModuleSP module_sp = FindModuleViaTarget(file)) {
+ bool base_addr_is_offset,
+ bool defer_module_preload) {
+ if (ModuleSP module_sp = FindModuleViaTarget(file, defer_module_preload)) {
UpdateLoadedSections(module_sp, link_map_addr, base_addr,
base_addr_is_offset);
return module_sp;
diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index c40612c1ced5e..c8594b4aa803a 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "lldb/Core/ModuleList.h"
+#include "lldb/Core/Debugger.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
@@ -26,6 +27,7 @@
#include "lldb/Utility/Log.h"
#include "lldb/Utility/UUID.h"
#include "lldb/lldb-defines.h"
+#include "llvm/Support/ThreadPool.h"
#if defined(_WIN32)
#include "lldb/Host/windows/PosixApi.h"
@@ -1357,3 +1359,14 @@ void ModuleList::Swap(ModuleList &other) {
m_modules_mutex, other.m_modules_mutex);
m_modules.swap(other.m_modules);
}
+
+void ModuleList::PreloadSymbols() const {
+ std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
+ llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
+ for (const ModuleSP &module_sp : m_modules)
+ task_group.async([module_sp] {
+ if (module_sp)
+ module_sp->PreloadSymbols();
+ });
+ // task group destructor waits for all tasks to complete
+}
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
index 326b6910b5267..2ca3f1eb6e27a 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -453,7 +453,8 @@ void DynamicLoaderPOSIXDYLD::RefreshModules() {
// exists for the duration of this call in `m_rendezvous`.
auto load_module_fn =
[this, &loaded_modules, &new_modules,
- &interpreter_module_mutex](const DYLDRendezvous::SOEntry &so_entry) {
+ &interpreter_module_mutex](const DYLDRendezvous::SOEntry &so_entry,
+ bool defer_module_preload) {
// Don't load a duplicate copy of ld.so if we have already loaded it
// earlier in LoadInterpreterModule. If we instead loaded then
// unloaded it later, the section information for ld.so would be
@@ -469,7 +470,8 @@ void DynamicLoaderPOSIXDYLD::RefreshModules() {
}
ModuleSP module_sp = LoadModuleAtAddress(
- so_entry.file_spec, so_entry.link_addr, so_entry.base_addr, true);
+ so_entry.file_spec, so_entry.link_addr, so_entry.base_addr,
+ true /* base_addr_is_offset */, defer_module_preload);
if (!module_sp.get())
return;
@@ -503,11 +505,14 @@ void DynamicLoaderPOSIXDYLD::RefreshModules() {
if (m_process->GetTarget().GetParallelModuleLoad()) {
llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
for (; I != E; ++I)
- task_group.async(load_module_fn, *I);
+ task_group.async(load_module_fn, *I, true /* defer_module_preload */);
task_group.wait();
+
+ if (m_process->GetTarget().GetPreloadSymbols())
+ new_modules.PreloadSymbols();
} else {
for (; I != E; ++I)
- load_module_fn(*I);
+ load_module_fn(*I, false /* defer_module_preload */);
}
m_process->GetTarget().ModulesDidLoad(new_modules);
@@ -644,12 +649,12 @@ ModuleSP DynamicLoaderPOSIXDYLD::LoadInterpreterModule() {
return nullptr;
}
-ModuleSP DynamicLoaderPOSIXDYLD::LoadModuleAtAddress(const FileSpec &file,
- addr_t link_map_addr,
- addr_t base_addr,
- bool base_addr_is_offset) {
+ModuleSP DynamicLoaderPOSIXDYLD::LoadModuleAtAddress(
+ const FileSpec &file, addr_t link_map_addr, addr_t base_addr,
+ bool base_addr_is_offset, bool defer_module_preload) {
if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
- file, link_map_addr, base_addr, base_addr_is_offset))
+ file, link_map_addr, base_addr, base_addr_is_offset,
+ defer_module_preload))
return module_sp;
// This works around an dynamic linker "bug" on android <= 23, where the
@@ -668,7 +673,7 @@ ModuleSP DynamicLoaderPOSIXDYLD::LoadModuleAtAddress(const FileSpec &file,
!(memory_info.GetName().IsEmpty())) {
if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
FileSpec(memory_info.GetName().GetStringRef()), link_map_addr,
- base_addr, base_addr_is_offset))
+ base_addr, base_addr_is_offset, defer_module_preload))
return module_sp;
}
}
@@ -704,9 +709,11 @@ void DynamicLoaderPOSIXDYLD::LoadAllCurrentModules() {
module_names, m_process->GetTarget().GetArchitecture().GetTriple());
auto load_module_fn = [this, &module_list,
- &log](const DYLDRendezvous::SOEntry &so_entry) {
- ModuleSP module_sp = LoadModuleAtAddress(
- so_entry.file_spec, so_entry.link_addr, so_entry.base_addr, true);
+ &log](const DYLDRendezvous::SOEntry &so_entry,
+ bool defer_module_preload) {
+ ModuleSP module_sp =
+ LoadModuleAtAddress(so_entry.file_spec, so_entry.link_addr,
+ so_entry.base_addr, true, defer_module_preload);
if (module_sp.get()) {
LLDB_LOG(log, "LoadAllCurrentModules loading module: {0}",
so_entry.file_spec.GetFilename());
@@ -723,11 +730,14 @@ void DynamicLoaderPOSIXDYLD::LoadAllCurrentModules() {
if (m_process->GetTarget().GetParallelModuleLoad()) {
llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I)
- task_group.async(load_module_fn, *I);
+ task_group.async(load_module_fn, *I, true /* defer_module_preload */);
task_group.wait();
+
+ if (m_process->GetTarget().GetPreloadSymbols())
+ module_list.PreloadSymbols();
} else {
for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I) {
- load_module_fn(*I);
+ load_module_fn(*I, false /* defer_module_preload */);
}
}
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
index 6efb92673a13c..2d9dba9bca9fa 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
@@ -55,10 +55,11 @@ class DynamicLoaderPOSIXDYLD : public lldb_private::DynamicLoader {
// PluginInterface protocol
llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
- lldb::ModuleSP LoadModuleAtAddress(const lldb_private::FileSpec &file,
- lldb::addr_t link_map_addr,
- lldb::addr_t base_addr,
- bool base_addr_is_offset) override;
+ lldb::ModuleSP
+ LoadModuleAtAddress(const lldb_private::FileSpec &file,
+ lldb::addr_t link_map_addr, lldb::addr_t base_addr,
+ bool base_addr_is_offset,
+ bool defer_module_preload = false) override;
void CalculateDynamicSaveCoreRanges(
lldb_private::Process &process,
diff --git a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
index d019415cb67a6..09cde464ccdc8 100644
--- a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
@@ -67,9 +67,11 @@ ThreadPlanSP DynamicLoaderWasmDYLD::GetStepThroughTrampolinePlan(Thread &thread,
lldb::ModuleSP DynamicLoaderWasmDYLD::LoadModuleAtAddress(
const lldb_private::FileSpec &file, lldb::addr_t link_map_addr,
- lldb::addr_t base_addr, bool base_addr_is_offset) {
+ lldb::addr_t base_addr, bool base_addr_is_offset,
+ bool defer_module_preload) {
if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
- file, link_map_addr, base_addr, base_addr_is_offset))
+ file, link_map_addr, base_addr, base_addr_is_offset,
+ defer_module_preload))
return module_sp;
if (ModuleSP module_sp = m_process->ReadModuleFromMemory(file, base_addr)) {
diff --git a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
index 5ed855395cca7..9c13bdff0279a 100644
--- a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
+++ b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
@@ -33,10 +33,11 @@ class DynamicLoaderWasmDYLD : public DynamicLoader {
Status CanLoadImage() override { return Status(); }
lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
bool stop) override;
- lldb::ModuleSP LoadModuleAtAddress(const lldb_private::FileSpec &file,
- lldb::addr_t link_map_addr,
- lldb::addr_t base_addr,
- bool base_addr_is_offset) override;
+ lldb::ModuleSP
+ LoadModuleAtAddress(const lldb_private::FileSpec &file,
+ lldb::addr_t link_map_addr, lldb::addr_t base_addr,
+ bool base_addr_is_offset,
+ bool defer_module_preload = false) override;
/// \}
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index d070c3d953d4a..ea685992043e7 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2342,7 +2342,8 @@ bool Target::ReadPointerFromMemory(const Address &addr, Status &error,
}
ModuleSP Target::GetOrCreateModule(const ModuleSpec &orig_module_spec,
- bool notify, Status *error_ptr) {
+ bool notify, Status *error_ptr,
+ bool defer_preload) {
ModuleSP module_sp;
Status error;
@@ -2406,7 +2407,7 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &orig_module_spec,
module_spec.GetFileSpec().GetDirectory(), transformed_dir)) {
transformed_spec.GetFileSpec().SetDirectory(transformed_dir);
transformed_spec.GetFileSpec().SetFilename(
- module_spec.GetFileSpec().GetFilename());
+ module_spec.GetFileSpec().GetFilename());
error = ModuleList::GetSharedModule(transformed_spec, module_sp,
&search_paths, &old_modules,
&did_create_module);
@@ -2510,8 +2511,9 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &orig_module_spec,
// Preload symbols outside of any lock, so hopefully we can do this for
// each library in parallel.
- if (GetPreloadSymbols())
+ if (GetPreloadSymbols() && !defer_preload)
module_sp->PreloadSymbols();
+
llvm::SmallVector<ModuleSP, 1> replaced_modules;
for (ModuleSP &old_module_sp : old_modules) {
if (m_images.GetIndexForModule(old_module_sp.get()) !=
``````````
</details>
https://github.com/llvm/llvm-project/pull/166480
More information about the lldb-commits
mailing list