Don't hold the Target's ModuleListLock over running LoadScriptingResourceInTarget

[jimingham]

That calls an unknown amount of Python code, and can do quite a bit of work - especially if people do things like launch scripted processes in this script affordance. Doing that while holding a major lock like the ModuleList lock is asking for trouble.

I tried to make a test that would actually stall without this, but I couldn't come up with anything that reliably failed. You always have to get pretty unlucky.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

That calls an unknown amount of Python code, and can do quite a bit of work - especially if people do things like launch scripted processes in this script affordance. Doing that while holding a major lock like the ModuleList lock is asking for trouble.

I tried to make a test that would actually stall without this, but I couldn't come up with anything that reliably failed. You always have to get pretty unlucky.

---

Full diff: https://github.com/llvm/llvm-project/pull/138216.diff

1 Files Affected:

• (modified) lldb/source/Core/ModuleList.cpp (+8-2)

diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index 6052cc151744d..9724039decf0d 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -1046,8 +1046,14 @@ bool ModuleList::LoadScriptingResourcesInTarget(Target *target,
                                                 bool continue_on_error) {
   if (!target)
     return false;
-  std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
-  for (auto module : m_modules) {
+  m_modules_mutex.lock();
+  // Don't hold the module list mutex while loading the scripting resources,
+  // The initializer might do any amount of work, and having that happen while
+  // the module list is held is asking for A/B locking problems.
+  const ModuleList tmp_module_list(*this);
+  m_modules_mutex.unlock();
+  
+  for (auto module : tmp_module_list.ModulesNoLocking()) {
     if (module) {
       Status error;
       if (!module->LoadScriptingResourceInTarget(target, error,

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- lldb/source/Core/ModuleList.cpp

View the diff from clang-format here.

diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index 9724039de..d5ddf6e84 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -1052,7 +1052,7 @@ bool ModuleList::LoadScriptingResourcesInTarget(Target *target,
   // the module list is held is asking for A/B locking problems.
   const ModuleList tmp_module_list(*this);
   m_modules_mutex.unlock();
-  
+
   for (auto module : tmp_module_list.ModulesNoLocking()) {
     if (module) {
       Status error;