Skip to content

Commit ab08de3

Browse files
committed
Fix a deadlock in ModuleList when starting a standalone lldb client
Summary: There was a deadlock was introduced by [PR llvm#146441](llvm#146441) which changed `CurrentThreadIsPrivateStateThread()` to `CurrentThreadPosesAsPrivateStateThread()`. This change caused the execution path in `ExecutionContextRef::SetTargetPtr()` to now enter a code block that was previously skipped, triggering `GetSelectedFrame()` which leads to a deadlock. In particular, one thread held `m_modules_mutex` and tried to acquire `m_language_runtimes_mutex` (via the notifier call chain), and another thread held `m_language_runtimes_mutex` and tried to acquire `m_modules_mutex` (via `ScanForGNUstepObjCLibraryCandidate`) This fixes the deadlock by adding a scoped block around the mutex lock before the call to the notifier, and moved the notifier call outside of the mutex-guarded section. Test Plan: Tested manually
1 parent 2c67718 commit ab08de3

File tree

1 file changed

+25
-21
lines changed

1 file changed

+25
-21
lines changed

lldb/source/Core/ModuleList.cpp

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -215,30 +215,34 @@ ModuleList::~ModuleList() = default;
215215

216216
void ModuleList::AppendImpl(const ModuleSP &module_sp, bool use_notifier) {
217217
if (module_sp) {
218-
std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
219-
// We are required to keep the first element of the Module List as the
220-
// executable module. So check here and if the first module is NOT an
221-
// but the new one is, we insert this module at the beginning, rather than
222-
// at the end.
223-
// We don't need to do any of this if the list is empty:
224-
if (m_modules.empty()) {
225-
m_modules.push_back(module_sp);
226-
} else {
227-
// Since producing the ObjectFile may take some work, first check the 0th
228-
// element, and only if that's NOT an executable look at the incoming
229-
// ObjectFile. That way in the normal case we only look at the element
230-
// 0 ObjectFile.
231-
const bool elem_zero_is_executable
232-
= m_modules[0]->GetObjectFile()->GetType()
233-
== ObjectFile::Type::eTypeExecutable;
234-
lldb_private::ObjectFile *obj = module_sp->GetObjectFile();
235-
if (!elem_zero_is_executable && obj
236-
&& obj->GetType() == ObjectFile::Type::eTypeExecutable) {
237-
m_modules.insert(m_modules.begin(), module_sp);
238-
} else {
218+
{
219+
std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
220+
// We are required to keep the first element of the Module List as the
221+
// executable module. So check here and if the first module is NOT an
222+
// but the new one is, we insert this module at the beginning, rather than
223+
// at the end.
224+
// We don't need to do any of this if the list is empty:
225+
if (m_modules.empty()) {
239226
m_modules.push_back(module_sp);
227+
} else {
228+
// Since producing the ObjectFile may take some work, first check the
229+
// 0th element, and only if that's NOT an executable look at the
230+
// incoming ObjectFile. That way in the normal case we only look at the
231+
// element 0 ObjectFile.
232+
const bool elem_zero_is_executable =
233+
m_modules[0]->GetObjectFile()->GetType() ==
234+
ObjectFile::Type::eTypeExecutable;
235+
lldb_private::ObjectFile *obj = module_sp->GetObjectFile();
236+
if (!elem_zero_is_executable && obj &&
237+
obj->GetType() == ObjectFile::Type::eTypeExecutable) {
238+
m_modules.insert(m_modules.begin(), module_sp);
239+
} else {
240+
m_modules.push_back(module_sp);
241+
}
240242
}
241243
}
244+
// Release the mutex before calling the notifier to avoid deadlock
245+
// NotifyModuleAdded should be thread-safe
242246
if (use_notifier && m_notifier)
243247
m_notifier->NotifyModuleAdded(*this, module_sp);
244248
}

0 commit comments

Comments
 (0)