[LLDB] Fix deadlock in module callback when running in parallel

[Jlalond]

When the target is being created, the target list acquires the mutex for the duration of the target creation process. However if a module callback is enabled and is being called in parallel there exists an opportunity to deadlock if the callback calls into targetlist. I've created a minimum repro here.

command script import dead-lock-example (from above gist)
...
target create a.out
[hangs]

This looks like a straight forward fix, where CreateTargetInternal doesn't access any state directly, and instead calls methods which they themselves are thread-safe. So I've moved the lock to when we update the list with the created target. I'm not sure if this is a comprehensive fix, but it does fix my above example and in my (albeit limited) testing, doesn't cause any strange change in behavior.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

When the target is being created, the target list acquires the mutex for the duration of the target creation process. However if a module callback is enabled and is being called in parallel there exists an opportunity to deadlock if the callback calls into targetlist. I've created a minimum repro here.

This looks like a straight forward fix, where CreateTargetInternal doesn't access any state directly, and instead calls methods which they themselves are thread-safe. So I've moved the lock to when we update the list with the created target. I'm not sure if this is a comprehensive fix, but it does fix my above example and in my (albeit limited) testing, doesn't cause any strange change in behavior.

---

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

1 Files Affected:

• (modified) lldb/source/Target/TargetList.cpp (+12-2)

diff --git a/lldb/source/Target/TargetList.cpp b/lldb/source/Target/TargetList.cpp
index 2e03bc1e38ea0..af0d24d7b1d0a 100644
--- a/lldb/source/Target/TargetList.cpp
+++ b/lldb/source/Target/TargetList.cpp
@@ -48,11 +48,16 @@ Status TargetList::CreateTarget(Debugger &debugger,
                                 LoadDependentFiles load_dependent_files,
                                 const OptionGroupPlatform *platform_options,
                                 TargetSP &target_sp) {
-  std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
+  // Create Target Internal does not modify and state
+  // directly and instead calls into methods which
+  // they themselves are thread-safe. We do this so
+  // the load module call back doesn't cause a re-entry
+  // dead-lock when creating the target.
   auto result = TargetList::CreateTargetInternal(
       debugger, user_exe_path, triple_str, load_dependent_files,
       platform_options, target_sp);
 
+  std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
   if (target_sp && result.Success())
     AddTargetInternal(target_sp, /*do_select*/ true);
   return result;
@@ -63,11 +68,16 @@ Status TargetList::CreateTarget(Debugger &debugger,
                                 const ArchSpec &specified_arch,
                                 LoadDependentFiles load_dependent_files,
                                 PlatformSP &platform_sp, TargetSP &target_sp) {
-  std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
+  // Create Target Internal does not modify and state
+  // directly and instead calls into methods which
+  // they themselves are thread-safe. We do this so
+  // the load module call back doesn't cause a re-entry
+  // dead-lock when creating the target.
   auto result = TargetList::CreateTargetInternal(
       debugger, user_exe_path, specified_arch, load_dependent_files,
       platform_sp, target_sp);
 
+  std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
   if (target_sp && result.Success())
     AddTargetInternal(target_sp, /*do_select*/ true);
   return result;

[github-actions]

🐧 Linux x64 Test Results

• 33126 tests passed
• 494 tests skipped

[clayborg]

Seems ok to to. m_target_list_mutex is really there to protect the target list itself. We might want to move the mutex lock into TargetList::AddTargetInternal to keep things cleaner?