[Lldb-commits] [PATCH] D149949: [lldb][TypeSystem] ForEach: Don't hold the TypeSystemMap lock across callback

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added reviewers: aprantl, jingham.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The `TypeSystemMap::m_mutex` guards against concurrent modifications
of members of `TypeSystemMap`. In particular, `m_map`.

`TypeSystemMap::ForEach` iterates through the entire `m_map` calling
a user-specified callback for each entry. This is all done while
`m_mutex` is locked. However, there's nothing that guarantees that
the callback itself won't call back into `TypeSystemMap` APIs on the
same thread. This lead to double-locking `m_mutex`, which is undefined
behaviour. We've seen this cause a deadlock in the swift plugin with
following backtrace:

  int main() {
      std::unique_ptr<int> up = std::make_unique<int>(5);
  
      volatile int val = *up;
      return val;
  }
  
  clang++ -std=c++2a -g -O1 main.cpp
  
  ./bin/lldb -o “br se -p return” -o run -o “v *up” -o “expr *up” -b



  frame #4: std::lock_guard<std::mutex>::lock_guard
  frame #5: lldb_private::TypeSystemMap::GetTypeSystemForLanguage <<<< Lock #2
  frame #6: lldb_private::TypeSystemMap::GetTypeSystemForLanguage
  frame #7: lldb_private::Target::GetScratchTypeSystemForLanguage
  ...
  frame #26: lldb_private::SwiftASTContext::LoadLibraryUsingPaths
  frame #27: lldb_private::SwiftASTContext::LoadModule
  frame #30: swift::ModuleDecl::collectLinkLibraries
  frame #31: lldb_private::SwiftASTContext::LoadModule
  frame #34: lldb_private::SwiftASTContext::GetCompileUnitImportsImpl
  frame #35: lldb_private::SwiftASTContext::PerformCompileUnitImports
  frame #36: lldb_private::TypeSystemSwiftTypeRefForExpressions::GetSwiftASTContext
  frame #37: lldb_private::TypeSystemSwiftTypeRefForExpressions::GetPersistentExpressionState
  frame #38: lldb_private::Target::GetPersistentSymbol
  frame #41: lldb_private::TypeSystemMap::ForEach                 <<<< Lock #1
  frame #42: lldb_private::Target::GetPersistentSymbol
  frame #43: lldb_private::IRExecutionUnit::FindInUserDefinedSymbols
  frame #44: lldb_private::IRExecutionUnit::FindSymbol
  frame #45: lldb_private::IRExecutionUnit::MemoryManager::GetSymbolAddressAndPresence
  frame #46: lldb_private::IRExecutionUnit::MemoryManager::findSymbol
  frame #47: non-virtual thunk to lldb_private::IRExecutionUnit::MemoryManager::findSymbol
  frame #48: llvm::LinkingSymbolResolver::findSymbol
  frame #49: llvm::LegacyJITSymbolResolver::lookup
  frame #50: llvm::RuntimeDyldImpl::resolveExternalSymbols
  frame #51: llvm::RuntimeDyldImpl::resolveRelocations
  frame #52: llvm::MCJIT::finalizeLoadedModules
  frame #53: llvm::MCJIT::finalizeObject
  frame #54: lldb_private::IRExecutionUnit::ReportAllocations
  frame #55: lldb_private::IRExecutionUnit::GetRunnableInfo
  frame #56: lldb_private::ClangExpressionParser::PrepareForExecution
  frame #57: lldb_private::ClangUserExpression::TryParse
  frame #58: lldb_private::ClangUserExpression::Parse

Our solution is to simply iterate over a local copy of `m_map`.

**Testing**

- Confirmed on manual reproducer (would reproduce 100% of the time before the patch)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149949

Files:
  lldb/source/Symbol/TypeSystem.cpp


Index: lldb/source/Symbol/TypeSystem.cpp
===================================================================
--- lldb/source/Symbol/TypeSystem.cpp
+++ lldb/source/Symbol/TypeSystem.cpp
@@ -217,7 +217,17 @@
 
 void TypeSystemMap::ForEach(
     std::function<bool(lldb::TypeSystemSP)> const &callback) {
-  std::lock_guard<std::mutex> guard(m_mutex);
+
+  // The callback may call into this function again causing
+  // us to lock m_mutex twice if we held it across the callback.
+  // Since we just care about guarding access to 'm_map', make
+  // a local copy and iterate over that instead.
+  collection map_snapshot;
+  {
+      std::lock_guard<std::mutex> guard(m_mutex);
+      map_snapshot = m_map;
+  }
+
   // Use a std::set so we only call the callback once for each unique
   // TypeSystem instance.
   llvm::DenseSet<TypeSystem *> visited;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D149949.519802.patch
Type: text/x-patch
Size: 851 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20230505/9a30cbe2/attachment.bin>