[lldb] Iterate over a copy of the ModuleList in SearchFilter

[JDevlieghere]

Avoid a potential deadlock caused by the search filter callback acquiring the target's module lock by iterating over a copy of the list.

Fixes #188766

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Avoid a potential deadlock caused by the search filter callback acquiring the target's module lock by iterating over a copy of the list.

Fixes #188766

---

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

3 Files Affected:

• (modified) lldb/source/Core/SearchFilter.cpp (+14-13)
• (added) lldb/test/Shell/Breakpoint/Inputs/main.c (+1)
• (added) lldb/test/Shell/Breakpoint/source-regex-missing-source.test (+11)

diff --git a/lldb/source/Core/SearchFilter.cpp b/lldb/source/Core/SearchFilter.cpp
index 6cd00af64870a..e14beba1fe40c 100644
--- a/lldb/source/Core/SearchFilter.cpp
+++ b/lldb/source/Core/SearchFilter.cpp
@@ -149,7 +149,7 @@ bool SearchFilter::CompUnitPasses(FileSpec &fileSpec) { return true; }
 bool SearchFilter::CompUnitPasses(CompileUnit &compUnit) { return true; }
 
 bool SearchFilter::FunctionPasses(Function &function) {
-  // This is a slightly cheesy job, but since we don't have finer grained 
+  // This is a slightly cheesy job, but since we don't have finer grained
   // filters yet, just checking that the start address passes is probably
   // good enough for the base class behavior.
   Address addr = function.GetAddress();
@@ -262,7 +262,9 @@ SearchFilter::DoModuleIteration(const SymbolContext &context,
     return Searcher::eCallbackReturnContinue;
   }
 
-  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
+  ModuleList module_list = m_target_sp->GetImages();
+  // Since we're iterating over a copy, no need to do any locking.
+  for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
     // If this is the last level supplied, then call the callback directly,
     // otherwise descend.
     if (!ModulePasses(module_sp))
@@ -422,14 +424,9 @@ void SearchFilterByModule::Search(Searcher &searcher) {
     searcher.SearchCallback(*this, empty_sc, nullptr);
   }
 
-  // If the module file spec is a full path, then we can just find the one
-  // filespec that passes.  Otherwise, we need to go through all modules and
-  // find the ones that match the file name.
-
-  const ModuleList &target_modules = m_target_sp->GetImages();
-  std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-
-  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
+  ModuleList module_list = m_target_sp->GetImages();
+  // Since we're iterating over a copy, no need to do any locking.
+  for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
     if (FileSpec::Match(m_module_spec, module_sp->GetFileSpec())) {
       SymbolContext matchingContext(m_target_sp, module_sp);
       Searcher::CallbackReturn shouldContinue;
@@ -545,7 +542,9 @@ void SearchFilterByModuleList::Search(Searcher &searcher) {
   // If the module file spec is a full path, then we can just find the one
   // filespec that passes.  Otherwise, we need to go through all modules and
   // find the ones that match the file name.
-  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
+  ModuleList module_list = m_target_sp->GetImages();
+  // Since we're iterating over a copy, no need to do any locking.
+  for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
     if (m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) ==
         UINT32_MAX)
       continue;
@@ -707,7 +706,7 @@ bool SearchFilterByModuleListAndCU::AddressPasses(Address &address) {
     cu_spec = sym_ctx.comp_unit->GetPrimaryFile();
   if (m_cu_spec_list.FindFileIndex(0, cu_spec, false) == UINT32_MAX)
     return false; // Fails the file check
-  return SearchFilterByModuleList::ModulePasses(sym_ctx.module_sp); 
+  return SearchFilterByModuleList::ModulePasses(sym_ctx.module_sp);
 }
 
 bool SearchFilterByModuleListAndCU::CompUnitPasses(FileSpec &fileSpec) {
@@ -744,7 +743,9 @@ void SearchFilterByModuleListAndCU::Search(Searcher &searcher) {
   ModuleList matching_modules;
 
   bool no_modules_in_filter = m_module_spec_list.GetSize() == 0;
-  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
+  ModuleList module_list = m_target_sp->GetImages();
+  // Since we're iterating over a copy, no need to do any locking.
+  for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
     if (!no_modules_in_filter &&
         m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) ==
             UINT32_MAX)
diff --git a/lldb/test/Shell/Breakpoint/Inputs/main.c b/lldb/test/Shell/Breakpoint/Inputs/main.c
new file mode 100644
index 0000000000000..78f2de106c92b
--- /dev/null
+++ b/lldb/test/Shell/Breakpoint/Inputs/main.c
@@ -0,0 +1 @@
+int main(void) { return 0; }
diff --git a/lldb/test/Shell/Breakpoint/source-regex-missing-source.test b/lldb/test/Shell/Breakpoint/source-regex-missing-source.test
new file mode 100644
index 0000000000000..15f3d2dfc7c2b
--- /dev/null
+++ b/lldb/test/Shell/Breakpoint/source-regex-missing-source.test
@@ -0,0 +1,11 @@
+# Test that setting a source regex breakpoint doesn't crash when the source
+# file has been deleted after compilation.
+
+# RUN: cp %p/Inputs/main.c %t.c
+# RUN: %clang_host -g -o %t %t.c
+# RUN: rm %t.c
+# RUN: %lldb -b -o "br set -p any" %t 2>&1 | FileCheck %s
+
+# CHECK: Breakpoint 1: no locations (pending).
+# CHECK-NOT: error
+# CHECK-NOT: assert

[igorkudrin]

LGTM. Thanks for the quick fix.

[igorkudrin]

BTW, this should be ported to LLVM-22.

[JDevlieghere]

/cherry-pick ce1b12e

[llvmbot]

/pull-request #190277