move preload out of GetOrCreateModules and parallelize

Tom Yang · Tom Yang · commit ffd2e0c0cfcb · 2025-11-04T15:44:36.000-08:00
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D86004440
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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()) !=
