https://github.com/zhyty created https://github.com/llvm/llvm-project/pull/166480
Another attempt at resolving the deadlock issue @hyubo 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 faster in this new implementation. # How does this solve the issue? As @hyubo 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. Thus, we deadlock. ## Why not read-write lock? # Other attempts Ideally, this fix would also improve the easy of using parallel module loading in the caller. Any future DYLD implementation would need to explicitly # TODO upstreaming at the same time. Landing internally to unblock ourselves # Fixing module load order In this change, I decided to go with the non-deterministic module loading order as with the previous implementation. TODO explain perf # Test Plan: >From ffd2e0c0cfcbe9c20deda9e95a08ebcc30dcb14e Mon Sep 17 00:00:00 2001 From: Tom Yang <[email protected]> Date: Tue, 14 Oct 2025 00:13:03 -0700 Subject: [PATCH] move preload out of GetOrCreateModules and parallelize Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D86004440 --- lldb/include/lldb/Core/ModuleList.h | 3 ++ lldb/include/lldb/Target/DynamicLoader.h | 17 +++++++- lldb/include/lldb/Target/Target.h | 10 ++++- lldb/source/Core/DynamicLoader.cpp | 12 ++++-- lldb/source/Core/ModuleList.cpp | 13 ++++++ .../POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp | 40 ++++++++++++------- .../POSIX-DYLD/DynamicLoaderPOSIXDYLD.h | 9 +++-- .../wasm-DYLD/DynamicLoaderWasmDYLD.cpp | 6 ++- .../wasm-DYLD/DynamicLoaderWasmDYLD.h | 9 +++-- lldb/source/Target/Target.cpp | 8 ++-- 10 files changed, 92 insertions(+), 35 deletions(-) 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..c8e917bb0da4a 100644 --- a/lldb/source/Core/ModuleList.cpp +++ b/lldb/source/Core/ModuleList.cpp @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include "lldb/Core/Debugger.h" #include "lldb/Core/ModuleList.h" #include "lldb/Core/Module.h" #include "lldb/Core/ModuleSpec.h" @@ -15,6 +16,7 @@ #include "lldb/Interpreter/OptionValueFileSpecList.h" #include "lldb/Interpreter/OptionValueProperties.h" #include "lldb/Interpreter/Property.h" +#include "llvm/Support/ThreadPool.h" #include "lldb/Symbol/ObjectFile.h" #include "lldb/Symbol/SymbolContext.h" #include "lldb/Symbol/TypeList.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()) != _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
