https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/184259
>From 11cd02efa61a3039225bc2c109f9ec9ee2934d6b Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Mon, 2 Mar 2026 15:06:51 -0800 Subject: [PATCH 1/2] [lldb] Fix 10 year old leak of g_debugger_list_ptr Roughly 10 years ago, in aacb80853a46bd544fa76a945667302be1de706c, Greg deleted the call to delete g_debugger_list_ptr because of a race condition: ``` Fixed a threading race condition where we could crash after calling Debugger::Terminate(). The issue was we have two global variables: one that contains a DebuggerList pointer and one that contains a std::mutex pointer. These get initialized in Debugger::Initialize(), and everywhere that uses these does: if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); // do work while mutex is locked } Debugger::Terminate() was deleting and nulling out g_debugger_list_ptr which meant we had a race condition where someone might do the if statement and it evaluates to true, then another thread calls Debugger::Terminate() and deletes and nulls out g_debugger_list_ptr while holding the mutex, and another thread then locks the mutex and tries to use g_debugger_list_ptr. The fix is to just not delete and null out the g_debugger_list_ptr variable. ``` However, this isn't necessary as long as we persist ("leak") the mutex and always check it first. That's exactly what this patch does. Without it, the assert in Debugger::Initialize is incorrect. --- lldb/source/Core/Debugger.cpp | 160 ++++++++++++++++++---------------- 1 file changed, 84 insertions(+), 76 deletions(-) diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index 70a2d73066390..9e88a25462653 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -105,12 +105,12 @@ using namespace lldb_private; static lldb::user_id_t g_unique_id = 1; static size_t g_debugger_event_thread_stack_bytes = 8 * 1024 * 1024; -#pragma mark Static Functions +static std::recursive_mutex &GetDebuggerListMutex() { + static std::recursive_mutex g_mutex; + return g_mutex; +} -static std::recursive_mutex *g_debugger_list_mutex_ptr = - nullptr; // NOTE: intentional leak to avoid issues with C++ destructor chain -static Debugger::DebuggerList *g_debugger_list_ptr = - nullptr; // NOTE: intentional leak to avoid issues with C++ destructor chain +static Debugger::DebuggerList *g_debugger_list_ptr = nullptr; static llvm::DefaultThreadPool *g_thread_pool = nullptr; static constexpr OptionEnumValueElement g_show_disassembly_enum_values[] = { @@ -751,7 +751,7 @@ bool Debugger::SetShowInlineDiagnostics(bool b) { void Debugger::Initialize(LoadPluginCallbackType load_plugin_callback) { assert(g_debugger_list_ptr == nullptr && "Debugger::Initialize called more than once!"); - g_debugger_list_mutex_ptr = new std::recursive_mutex(); + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); g_debugger_list_ptr = new DebuggerList(); g_thread_pool = new llvm::DefaultThreadPool(llvm::optimal_concurrency()); g_load_plugin_callback = load_plugin_callback; @@ -761,10 +761,11 @@ void Debugger::Terminate() { assert(g_debugger_list_ptr && "Debugger::Terminate called without a matching Debugger::Initialize!"); - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); - for (const auto &debugger : *g_debugger_list_ptr) - debugger->HandleDestroyCallback(); + { + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (g_debugger_list_ptr) + for (const auto &debugger : *g_debugger_list_ptr) + debugger->HandleDestroyCallback(); } if (g_thread_pool) { @@ -772,13 +773,15 @@ void Debugger::Terminate() { delete g_thread_pool; } - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - // Clear our global list of debugger objects - { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + { + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (g_debugger_list_ptr) { for (const auto &debugger : *g_debugger_list_ptr) debugger->Clear(); g_debugger_list_ptr->clear(); + + delete g_debugger_list_ptr; + g_debugger_list_ptr = nullptr; } } } @@ -884,10 +887,10 @@ DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback, }); DebuggerSP debugger_sp(new Debugger(log_callback, baton)); helper.SetDebugger(debugger_sp.get()); - - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); - g_debugger_list_ptr->push_back(debugger_sp); + { + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (g_debugger_list_ptr) + g_debugger_list_ptr->push_back(debugger_sp); } debugger_sp->InstanceInitialize(); return debugger_sp; @@ -938,8 +941,8 @@ void Debugger::Destroy(DebuggerSP &debugger_sp) { debugger_sp->Clear(); - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (g_debugger_list_ptr) { DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) { if ((*pos).get() == debugger_sp.get()) { @@ -952,10 +955,10 @@ void Debugger::Destroy(DebuggerSP &debugger_sp) { DebuggerSP Debugger::FindDebuggerWithInstanceName(llvm::StringRef instance_name) { - if (!g_debugger_list_ptr || !g_debugger_list_mutex_ptr) - return DebuggerSP(); + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (!g_debugger_list_ptr) + return nullptr; - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); for (const DebuggerSP &debugger_sp : *g_debugger_list_ptr) { if (!debugger_sp) continue; @@ -963,35 +966,38 @@ Debugger::FindDebuggerWithInstanceName(llvm::StringRef instance_name) { if (llvm::StringRef(debugger_sp->GetInstanceName()) == instance_name) return debugger_sp; } - return DebuggerSP(); + + return nullptr; } TargetSP Debugger::FindTargetWithProcessID(lldb::pid_t pid) { - TargetSP target_sp; - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); - DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); - for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) { - target_sp = (*pos)->GetTargetList().FindTargetWithProcessID(pid); - if (target_sp) - break; - } + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (!g_debugger_list_ptr) + return nullptr; + + DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); + for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) { + if (TargetSP target_sp = + (*pos)->GetTargetList().FindTargetWithProcessID(pid)) + return target_sp; } - return target_sp; + + return nullptr; } TargetSP Debugger::FindTargetWithProcess(Process *process) { - TargetSP target_sp; - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); - DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); - for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) { - target_sp = (*pos)->GetTargetList().FindTargetWithProcess(process); - if (target_sp) - break; - } + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (!g_debugger_list_ptr) + return nullptr; + + DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); + for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) { + if (TargetSP target_sp = + (*pos)->GetTargetList().FindTargetWithProcess(process)) + return target_sp; } - return target_sp; + + return nullptr; } llvm::StringRef Debugger::GetStaticBroadcasterClass() { @@ -1555,8 +1561,8 @@ void Debugger::ReportInterruption(const InterruptionReport &report) { Debugger::DebuggerList Debugger::DebuggersRequestingInterruption() { DebuggerList result; - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (g_debugger_list_ptr) { for (auto debugger_sp : *g_debugger_list_ptr) { if (debugger_sp->InterruptRequested()) result.push_back(debugger_sp); @@ -1566,39 +1572,40 @@ Debugger::DebuggerList Debugger::DebuggersRequestingInterruption() { } size_t Debugger::GetNumDebuggers() { - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); - return g_debugger_list_ptr->size(); - } - return 0; + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (!g_debugger_list_ptr) + return 0; + + return g_debugger_list_ptr->size(); } lldb::DebuggerSP Debugger::GetDebuggerAtIndex(size_t index) { - DebuggerSP debugger_sp; + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (!g_debugger_list_ptr) + return nullptr; - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + if (g_debugger_list_ptr) { if (index < g_debugger_list_ptr->size()) - debugger_sp = g_debugger_list_ptr->at(index); + return g_debugger_list_ptr->at(index); } - return debugger_sp; + return nullptr; } DebuggerSP Debugger::FindDebuggerWithID(lldb::user_id_t id) { - DebuggerSP debugger_sp; + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (!g_debugger_list_ptr) + return nullptr; - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + if (g_debugger_list_ptr) { DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) { - if ((*pos)->GetID() == id) { - debugger_sp = *pos; - break; - } + if ((*pos)->GetID() == id) + return *pos; } } - return debugger_sp; + + return nullptr; } bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format, @@ -1727,8 +1734,8 @@ void Debugger::ReportProgress(uint64_t progress_id, std::string title, } // The progress event is not debugger specific, iterate over all debuggers // and deliver a progress event to each one. - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (g_debugger_list_ptr) { DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) PrivateReportProgress(*(*pos), progress_id, title, details, completed, @@ -1796,8 +1803,8 @@ void Debugger::ReportDiagnosticImpl(Severity severity, std::string message, } // The diagnostic event is not debugger specific, iterate over all debuggers // and deliver a diagnostic event to each one. - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (g_debugger_list_ptr) { for (const auto &debugger : *g_debugger_list_ptr) PrivateReportDiagnostic(*debugger, severity, message, false); } @@ -1828,14 +1835,15 @@ void Debugger::ReportInfo(std::string message, } void Debugger::ReportSymbolChange(const ModuleSpec &module_spec) { - if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { - std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); - for (DebuggerSP debugger_sp : *g_debugger_list_ptr) { - EventSP event_sp = std::make_shared<Event>( - lldb::eBroadcastSymbolChange, - new SymbolChangeEventData(debugger_sp, module_spec)); - debugger_sp->GetBroadcaster().BroadcastEvent(event_sp); - } + std::lock_guard<std::recursive_mutex> guard(GetDebuggerListMutex()); + if (!g_debugger_list_ptr) + return; + + for (DebuggerSP debugger_sp : *g_debugger_list_ptr) { + EventSP event_sp = std::make_shared<Event>( + lldb::eBroadcastSymbolChange, + new SymbolChangeEventData(debugger_sp, module_spec)); + debugger_sp->GetBroadcaster().BroadcastEvent(event_sp); } } >From 229776fbabd47307ca667fb5f4235411e1af3f33 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Tue, 3 Mar 2026 11:09:15 -0800 Subject: [PATCH 2/2] Remove redundant g_debugger_list_ptr check --- lldb/source/Core/Debugger.cpp | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index 9e88a25462653..5ae097ad68e57 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -1584,10 +1584,8 @@ lldb::DebuggerSP Debugger::GetDebuggerAtIndex(size_t index) { if (!g_debugger_list_ptr) return nullptr; - if (g_debugger_list_ptr) { - if (index < g_debugger_list_ptr->size()) - return g_debugger_list_ptr->at(index); - } + if (index < g_debugger_list_ptr->size()) + return g_debugger_list_ptr->at(index); return nullptr; } @@ -1597,12 +1595,9 @@ DebuggerSP Debugger::FindDebuggerWithID(lldb::user_id_t id) { if (!g_debugger_list_ptr) return nullptr; - if (g_debugger_list_ptr) { - DebuggerList::iterator pos, end = g_debugger_list_ptr->end(); - for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) { - if ((*pos)->GetID() == id) - return *pos; - } + for (const DebuggerSP &debugger_sp : *g_debugger_list_ptr) { + if (debugger_sp->GetID() == id) + return debugger_sp; } return nullptr; _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
