[Lldb-commits] [lldb] Parallelize module loading in POSIX dyld code (PR #130912)

[Jonas Devlieghere via lldb-commits]

================
@@ -423,34 +493,66 @@ void DynamicLoaderPOSIXDYLD::RefreshModules() {
       E = m_rendezvous.end();
       m_initial_modules_added = true;
     }
-    for (; I != E; ++I) {
-      // 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 removed. That
-      // information is required for placing breakpoints on Arm/Thumb systems.
-      if ((m_interpreter_module.lock() != nullptr) &&
-          (I->base_addr == m_interpreter_base))
-        continue;
-
-      ModuleSP module_sp =
-          LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
-      if (!module_sp.get())
-        continue;
-
-      if (module_sp->GetObjectFile()->GetBaseAddress().GetLoadAddress(
-              &m_process->GetTarget()) == m_interpreter_base) {
-        ModuleSP interpreter_sp = m_interpreter_module.lock();
-        if (m_interpreter_module.lock() == nullptr) {
-          m_interpreter_module = module_sp;
-        } else if (module_sp == interpreter_sp) {
-          // Module already loaded.
-          continue;
-        }
-      }
 
-      loaded_modules.AppendIfNeeded(module_sp);
-      new_modules.Append(module_sp);
+    std::mutex interpreter_module_mutex;
+    // We should be able to take SOEntry as reference since the data
+    // 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) {
+          // 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
+          // removed. That information is required for placing breakpoints on
+          // Arm/Thumb systems.
+          {
+            // `m_interpreter_module` may be modified by another thread at the
+            // same time, so we guard the access here.
+            std::lock_guard<std::mutex> lock(interpreter_module_mutex);
+            if ((m_interpreter_module.lock() != nullptr) &&
+                (so_entry.base_addr == m_interpreter_base))
+              return;
+          }
+
+          ModuleSP module_sp = LoadModuleAtAddress(
+              so_entry.file_spec, so_entry.link_addr, so_entry.base_addr, true);
+          if (!module_sp.get())
+            return;
+
+          {
+            // `m_interpreter_module` may be modified by another thread at the
+            // same time, so we guard the access here.
+            std::lock_guard<std::mutex> lock(interpreter_module_mutex);
+            // Set the interpreter module, if this is the interpreter.
+            if (module_sp->GetObjectFile()->GetBaseAddress().GetLoadAddress(
+                    &m_process->GetTarget()) == m_interpreter_base) {
+              ModuleSP interpreter_sp = m_interpreter_module.lock();
+              if (m_interpreter_module.lock() == nullptr) {
+                m_interpreter_module = module_sp;
+              } else if (module_sp == interpreter_sp) {
+                // Module already loaded.
+                return;
+              }
+            }
+          }
+
+          loaded_modules.AppendIfNeeded(module_sp);
+          new_modules.Append(module_sp);
----------------
JDevlieghere wrote:

Could we ever get into trouble because this operation isn't atomic? Both `ModuleList`s are thread safe, but is there a situation where another thread could see a module in one but not the other? Given that we only append to it in the lambda, I suspect it's fine. Maybe it's worth adding a comment here, something along the lines of we don't need to synchronize the module list as it's thread safe and no two threads look at the two lists at the same time, or something. 

https://github.com/llvm/llvm-project/pull/130912