Skip to content

Commit e74f03d

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

File tree

1 file changed

+12
-2
lines changed

1 file changed

+12
-2
lines changed

lldb/source/Symbol/TypeSystem.cpp

+12-2
Original file line numberDiff line numberDiff line change
@@ -217,11 +217,21 @@ void TypeSystemMap::Clear() {
217217

218218
void TypeSystemMap::ForEach(
219219
std::function<bool(lldb::TypeSystemSP)> const &callback) {
220-
std::lock_guard<std::mutex> guard(m_mutex);
220+
221+
// The callback may call into this function again causing
222+
// us to lock m_mutex twice if we held it across the callback.
223+
// Since we just care about guarding access to 'm_map', make
224+
// a local copy and iterate over that instead.
225+
collection map_snapshot;
226+
{
227+
std::lock_guard<std::mutex> guard(m_mutex);
228+
map_snapshot = m_map;
229+
}
230+
221231
// Use a std::set so we only call the callback once for each unique
222232
// TypeSystem instance.
223233
llvm::DenseSet<TypeSystem *> visited;
224-
for (auto &pair : m_map) {
234+
for (auto &pair : map_snapshot) {
225235
TypeSystem *type_system = pair.second.get();
226236
if (!type_system || visited.count(type_system))
227237
continue;

0 commit comments

Comments
 (0)