Skip to content

Commit

Permalink
[lldb][TypeSystem] ForEach: Don't hold the TypeSystemMap lock across …
Browse files Browse the repository at this point in the history
…callback

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

Differential Revision: https://reviews.llvm.org/D149949

(cherry picked from commit dda3a6a)
  • Loading branch information
Michael137 committed May 5, 2023
1 parent 4471e44 commit ac6d5b1
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions lldb/source/Symbol/TypeSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,21 @@ void TypeSystemMap::Clear() {

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;
for (auto &pair : m_map) {
for (auto &pair : map_snapshot) {
TypeSystem *type_system = pair.second.get();
if (!type_system || visited.count(type_system))
continue;
Expand Down

0 comments on commit ac6d5b1

Please sign in to comment.