remove defer_module_preload bool flag, remove preload from

Target::GetOrCreateModule

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

Author: Tom Yang

lldb/include/lldb/Core/ModuleList.h

@@ -512,7 +512,10 @@ class ModuleList {
   void Swap(ModuleList &other);
 
   /// For each module in this ModuleList, preload its symbols.
-  void PreloadSymbols() const;
+  ///
+  /// \param[in] parallelize
+  ///     If true, all modules will be preloaded in parallel.
+  void PreloadSymbols(bool parallelize) const;
 
 protected:
   // Class typedefs.

lldb/include/lldb/Target/DynamicLoader.h

@@ -208,16 +208,10 @@ class DynamicLoader : public PluginInterface {
   /// 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 defer_module_preload = false);
+                                             bool base_addr_is_offset);
 
   /// 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.
@@ -360,12 +354,7 @@ class DynamicLoader : public PluginInterface {
   // Utility methods for derived classes
 
   /// 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);
+  lldb::ModuleSP FindModuleViaTarget(const FileSpec &file);
 
   /// Checks to see if the target module has changed, updates the target
   /// accordingly and returns the target executable module.

lldb/include/lldb/Target/Target.h

@@ -649,20 +649,12 @@ 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,
-                                   bool defer_preload = false);
+                                   Status *error_ptr = nullptr);
 
   // Settings accessors
 

lldb/source/Core/DynamicLoader.cpp

@@ -154,8 +154,7 @@ DynamicLoader::GetSectionListFromModule(const ModuleSP module) const {
   return sections;
 }
 
-ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file,
-                                            bool defer_module_preload) {
+ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file) {
   Target &target = m_process->GetTarget();
   ModuleSpec module_spec(file, target.GetArchitecture());
   if (UUID uuid = m_process->FindModuleUUID(file.GetPath())) {
@@ -167,8 +166,7 @@ ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file,
     return module_sp;
 
   if (ModuleSP module_sp = target.GetOrCreateModule(
-          module_spec, false /* notify */, nullptr /* error_ptr */,
-          defer_module_preload))
+          module_spec, false /* notify */, nullptr /* error_ptr */))
     return module_sp;
 
   return nullptr;
@@ -177,9 +175,8 @@ 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,
-                                            bool defer_module_preload) {
-  if (ModuleSP module_sp = FindModuleViaTarget(file, defer_module_preload)) {
+                                            bool base_addr_is_offset) {
+  if (ModuleSP module_sp = FindModuleViaTarget(file)) {
     UpdateLoadedSections(module_sp, link_map_addr, base_addr,
                          base_addr_is_offset);
     return module_sp;

lldb/source/Core/ModuleList.cpp

@@ -1360,8 +1360,16 @@ void ModuleList::Swap(ModuleList &other) {
   m_modules.swap(other.m_modules);
 }
 
-void ModuleList::PreloadSymbols() const {
+void ModuleList::PreloadSymbols(bool parallelize) const {
   std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
+
+  if (!parallelize) {
+    for (const ModuleSP &module_sp : m_modules)
+      module_sp->PreloadSymbols();
+    return;
+  }
+
+  // parallelize
   llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
   for (const ModuleSP &module_sp : m_modules)
     task_group.async([module_sp] {

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

@@ -453,8 +453,7 @@ 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,
-                                    bool defer_module_preload) {
+         &interpreter_module_mutex](const DYLDRendezvous::SOEntry &so_entry) {
           // 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
@@ -471,7 +470,7 @@ void DynamicLoaderPOSIXDYLD::RefreshModules() {
 
           ModuleSP module_sp = LoadModuleAtAddress(
               so_entry.file_spec, so_entry.link_addr, so_entry.base_addr,
-              true /* base_addr_is_offset */, defer_module_preload);
+              true /* base_addr_is_offset */);
           if (!module_sp.get())
             return;
 
@@ -505,16 +504,17 @@ 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, true /* defer_module_preload */);
+        task_group.async(load_module_fn, *I);
       task_group.wait();
-
-      if (m_process->GetTarget().GetPreloadSymbols())
-        new_modules.PreloadSymbols();
     } else {
       for (; I != E; ++I)
-        load_module_fn(*I, false /* defer_module_preload */);
+        load_module_fn(*I);
     }
 
+    if (m_process->GetTarget().GetPreloadSymbols())
+      new_modules.PreloadSymbols(
+          m_process->GetTarget().GetParallelModuleLoad());
+
     m_process->GetTarget().ModulesDidLoad(new_modules);
   }
 
@@ -649,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, bool defer_module_preload) {
+ModuleSP DynamicLoaderPOSIXDYLD::LoadModuleAtAddress(const FileSpec &file,
+                                                     addr_t link_map_addr,
+                                                     addr_t base_addr,
+                                                     bool base_addr_is_offset) {
   if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
-          file, link_map_addr, base_addr, base_addr_is_offset,
-          defer_module_preload))
+          file, link_map_addr, base_addr, base_addr_is_offset))
     return module_sp;
 
   // This works around an dynamic linker "bug" on android <= 23, where the
@@ -673,7 +673,7 @@ ModuleSP DynamicLoaderPOSIXDYLD::LoadModuleAtAddress(
         !(memory_info.GetName().IsEmpty())) {
       if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
               FileSpec(memory_info.GetName().GetStringRef()), link_map_addr,
-              base_addr, base_addr_is_offset, defer_module_preload))
+              base_addr, base_addr_is_offset))
         return module_sp;
     }
   }
@@ -709,11 +709,9 @@ void DynamicLoaderPOSIXDYLD::LoadAllCurrentModules() {
       module_names, m_process->GetTarget().GetArchitecture().GetTriple());
 
   auto load_module_fn = [this, &module_list,
-                         &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);
+                         &log](const DYLDRendezvous::SOEntry &so_entry) {
+    ModuleSP module_sp = LoadModuleAtAddress(
+        so_entry.file_spec, so_entry.link_addr, so_entry.base_addr, true);
     if (module_sp.get()) {
       LLDB_LOG(log, "LoadAllCurrentModules loading module: {0}",
                so_entry.file_spec.GetFilename());
@@ -730,16 +728,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, true /* defer_module_preload */);
+      task_group.async(load_module_fn, *I);
     task_group.wait();
+  } else
+    for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I)
+      load_module_fn(*I);
 
-    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, false /* defer_module_preload */);
-    }
-  }
+  if (m_process->GetTarget().GetPreloadSymbols())
+    module_list.PreloadSymbols(m_process->GetTarget().GetParallelModuleLoad());
 
   m_process->GetTarget().ModulesDidLoad(module_list);
   m_initial_modules_added = true;

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h

@@ -55,11 +55,10 @@ 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,
-                      bool defer_module_preload = false) 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;
 
   void CalculateDynamicSaveCoreRanges(
       lldb_private::Process &process,

lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp

@@ -67,12 +67,13 @@ 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,
-    bool defer_module_preload) {
+    lldb::addr_t base_addr, bool base_addr_is_offset) {
   if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
-          file, link_map_addr, base_addr, base_addr_is_offset,
-          defer_module_preload))
+          file, link_map_addr, base_addr, base_addr_is_offset)) {
+    if (m_process->GetTarget().GetPreloadSymbols())
+      module_sp->PreloadSymbols();
     return module_sp;
+  }
 
   if (ModuleSP module_sp = m_process->ReadModuleFromMemory(file, base_addr)) {
     UpdateLoadedSections(module_sp, link_map_addr, base_addr, false);

lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h

@@ -33,11 +33,10 @@ 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,
-                      bool defer_module_preload = false) 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/source/Target/Target.cpp

@@ -2342,8 +2342,7 @@ bool Target::ReadPointerFromMemory(const Address &addr, Status &error,
 }
 
 ModuleSP Target::GetOrCreateModule(const ModuleSpec &orig_module_spec,
-                                   bool notify, Status *error_ptr,
-                                   bool defer_preload) {
+                                   bool notify, Status *error_ptr) {
   ModuleSP module_sp;
 
   Status error;
@@ -2509,11 +2508,6 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &orig_module_spec,
         if (symbol_file_spec)
           module_sp->SetSymbolFileFileSpec(symbol_file_spec);
 
-        // Preload symbols outside of any lock, so hopefully we can do this for
-        // each library in parallel.
-        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()) !=