[Lldb-commits] [lldb] [lldb] Fix ThreadList assignment race (PR #98293)
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Jul 11 05:00:43 PDT 2024
https://github.com/labath updated https://github.com/llvm/llvm-project/pull/98293
>From 0a594fea0a5db3ebc8de74edbcd051cbac911697 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Wed, 10 Jul 2024 04:06:42 +0200
Subject: [PATCH 1/2] [lldb] Fix ThreadList assignment race
ThreadList uses the Process mutex to guard its state. This means its not
possible to safely modify its process member, as the member is required
to lock the mutex.
Fortunately for us, we never actually need to change the process member
(we always just juggle different kinds of thread lists belonging to the
same process).
This patch replaces the process member assignment (which is technically
a race even when it assigns the same value) with an assertion.
Since all this means that the class can never change its process member
value (and it also must be non-null at all times), I've also changed the
member type to a reference.
---
lldb/include/lldb/Target/ThreadList.h | 6 +-
.../Python/OperatingSystemPython.cpp | 2 +-
lldb/source/Target/Process.cpp | 14 ++---
lldb/source/Target/ThreadList.cpp | 59 +++++++++----------
4 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/lldb/include/lldb/Target/ThreadList.h b/lldb/include/lldb/Target/ThreadList.h
index 6af04f8ffc376..91c4317cb8571 100644
--- a/lldb/include/lldb/Target/ThreadList.h
+++ b/lldb/include/lldb/Target/ThreadList.h
@@ -27,12 +27,13 @@ class ThreadList : public ThreadCollection {
friend class Process;
public:
- ThreadList(Process *process);
+ ThreadList(Process &process);
ThreadList(const ThreadList &rhs);
~ThreadList() override;
+ // Precondition: both thread lists must be belong to the same process.
const ThreadList &operator=(const ThreadList &rhs);
uint32_t GetSize(bool can_update = true);
@@ -135,6 +136,7 @@ class ThreadList : public ThreadCollection {
std::recursive_mutex &GetMutex() const override;
+ // Precondition: both thread lists must be belong to the same process.
void Update(ThreadList &rhs);
protected:
@@ -143,7 +145,7 @@ class ThreadList : public ThreadCollection {
void NotifySelectedThreadChanged(lldb::tid_t tid);
// Classes that inherit from Process can see and modify these
- Process *m_process; ///< The process that manages this thread list.
+ Process &m_process; ///< The process that manages this thread list.
uint32_t
m_stop_id; ///< The process stop ID that this thread list is valid for.
lldb::tid_t
diff --git a/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp b/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
index 81ee7e328b6c0..e026ffefd645e 100644
--- a/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ b/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -372,7 +372,7 @@ lldb::ThreadSP OperatingSystemPython::CreateThread(lldb::tid_t tid,
std::vector<bool> core_used_map;
if (thread_info_dict) {
- ThreadList core_threads(m_process);
+ ThreadList core_threads(*m_process);
ThreadList &thread_list = m_process->GetThreadList();
bool did_create = false;
ThreadSP thread_sp(
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index dc7f6c9e86a47..b91446e1c0e49 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -460,12 +460,10 @@ Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp,
m_private_state_listener_sp(
Listener::MakeListener("lldb.process.internal_state_listener")),
m_mod_id(), m_process_unique_id(0), m_thread_index_id(0),
- m_thread_id_to_index_id_map(), m_exit_status(-1), m_exit_string(),
- m_exit_status_mutex(), m_thread_mutex(), m_thread_list_real(this),
- m_thread_list(this), m_thread_plans(*this), m_extended_thread_list(this),
- m_extended_thread_stop_id(0), m_queue_list(this), m_queue_list_stop_id(0),
- m_watchpoint_resource_list(), m_notifications(), m_image_tokens(),
- m_breakpoint_site_list(), m_dynamic_checkers_up(),
+ m_thread_id_to_index_id_map(), m_exit_status(-1),
+ m_thread_list_real(*this), m_thread_list(*this), m_thread_plans(*this),
+ m_extended_thread_list(*this), m_extended_thread_stop_id(0),
+ m_queue_list(this), m_queue_list_stop_id(0),
m_unix_signals_sp(unix_signals_sp), m_abi_sp(), m_process_input_reader(),
m_stdio_communication("process.stdio"), m_stdio_communication_mutex(),
m_stdin_forward(false), m_stdout_data(), m_stderr_data(),
@@ -1183,8 +1181,8 @@ void Process::UpdateThreadListIfNeeded() {
// mutex between the call to UpdateThreadList(...) and the
// os->UpdateThreadList(...) so it doesn't change on us
ThreadList &old_thread_list = m_thread_list;
- ThreadList real_thread_list(this);
- ThreadList new_thread_list(this);
+ ThreadList real_thread_list(*this);
+ ThreadList new_thread_list(*this);
// Always update the thread list with the protocol specific thread list,
// but only update if "true" is returned
if (UpdateThreadList(m_thread_list_real, real_thread_list)) {
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 03e8daedff129..448e2c299d7d4 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -23,7 +23,7 @@
using namespace lldb;
using namespace lldb_private;
-ThreadList::ThreadList(Process *process)
+ThreadList::ThreadList(Process &process)
: ThreadCollection(), m_process(process), m_stop_id(0),
m_selected_tid(LLDB_INVALID_THREAD_ID) {}
@@ -36,14 +36,12 @@ ThreadList::ThreadList(const ThreadList &rhs)
const ThreadList &ThreadList::operator=(const ThreadList &rhs) {
if (this != &rhs) {
- // Lock both mutexes to make sure neither side changes anyone on us while
- // the assignment occurs
- std::lock(GetMutex(), rhs.GetMutex());
- std::lock_guard<std::recursive_mutex> guard(GetMutex(), std::adopt_lock);
- std::lock_guard<std::recursive_mutex> rhs_guard(rhs.GetMutex(),
- std::adopt_lock);
-
- m_process = rhs.m_process;
+ // Same process implies same mutex...
+ assert(&m_process == &rhs.m_process);
+ assert(&GetMutex() == &GetMutex());
+ // .. which means it's enough to lock one of them.
+ std::lock_guard<std::recursive_mutex> guard(GetMutex());
+
m_stop_id = rhs.m_stop_id;
m_threads = rhs.m_threads;
m_selected_tid = rhs.m_selected_tid;
@@ -84,7 +82,7 @@ uint32_t ThreadList::GetSize(bool can_update) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
if (can_update)
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
return m_threads.size();
}
@@ -92,7 +90,7 @@ ThreadSP ThreadList::GetThreadAtIndex(uint32_t idx, bool can_update) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
if (can_update)
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
ThreadSP thread_sp;
if (idx < m_threads.size())
@@ -104,7 +102,7 @@ ThreadSP ThreadList::FindThreadByID(lldb::tid_t tid, bool can_update) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
if (can_update)
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
ThreadSP thread_sp;
uint32_t idx = 0;
@@ -122,7 +120,7 @@ ThreadSP ThreadList::FindThreadByProtocolID(lldb::tid_t tid, bool can_update) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
if (can_update)
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
ThreadSP thread_sp;
uint32_t idx = 0;
@@ -140,7 +138,7 @@ ThreadSP ThreadList::RemoveThreadByID(lldb::tid_t tid, bool can_update) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
if (can_update)
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
ThreadSP thread_sp;
uint32_t idx = 0;
@@ -160,7 +158,7 @@ ThreadSP ThreadList::RemoveThreadByProtocolID(lldb::tid_t tid,
std::lock_guard<std::recursive_mutex> guard(GetMutex());
if (can_update)
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
ThreadSP thread_sp;
uint32_t idx = 0;
@@ -210,7 +208,7 @@ ThreadSP ThreadList::FindThreadByIndexID(uint32_t index_id, bool can_update) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
if (can_update)
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
ThreadSP thread_sp;
const uint32_t num_threads = m_threads.size();
@@ -241,7 +239,7 @@ bool ThreadList::ShouldStop(Event *event_ptr) {
// Scope for locker
std::lock_guard<std::recursive_mutex> guard(GetMutex());
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
for (lldb::ThreadSP thread_sp : m_threads) {
// This is an optimization... If we didn't let a thread run in between
// the previous stop and this one, we shouldn't have to consult it for
@@ -377,7 +375,7 @@ Vote ThreadList::ShouldReportStop(Event *event_ptr) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
Vote result = eVoteNoOpinion;
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
collection::iterator pos, end = m_threads.end();
Log *log = GetLog(LLDBLog::Step);
@@ -425,7 +423,7 @@ Vote ThreadList::ShouldReportStop(Event *event_ptr) {
void ThreadList::SetShouldReportStop(Vote vote) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
collection::iterator pos, end = m_threads.end();
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
@@ -438,7 +436,7 @@ Vote ThreadList::ShouldReportRun(Event *event_ptr) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
Vote result = eVoteNoOpinion;
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
collection::iterator pos, end = m_threads.end();
// Run through the threads and ask whether we should report this event. The
@@ -486,7 +484,7 @@ void ThreadList::Destroy() {
void ThreadList::RefreshStateAfterStop() {
std::lock_guard<std::recursive_mutex> guard(GetMutex());
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
Log *log = GetLog(LLDBLog::Step);
if (log && log->GetVerbose())
@@ -515,7 +513,7 @@ bool ThreadList::WillResume() {
// they are.
std::lock_guard<std::recursive_mutex> guard(GetMutex());
- m_process->UpdateThreadListIfNeeded();
+ m_process.UpdateThreadListIfNeeded();
collection::iterator pos, end = m_threads.end();
@@ -546,13 +544,13 @@ bool ThreadList::WillResume() {
if (log && log->GetVerbose())
LLDB_LOGF(log, "Turning on notification of new threads while single "
"stepping a thread.");
- m_process->StartNoticingNewThreads();
+ m_process.StartNoticingNewThreads();
} else {
Log *log = GetLog(LLDBLog::Step);
if (log && log->GetVerbose())
LLDB_LOGF(log, "Turning off notification of new threads while single "
"stepping a thread.");
- m_process->StopNoticingNewThreads();
+ m_process.StopNoticingNewThreads();
}
// Give all the threads that are likely to run a last chance to set up their
@@ -575,7 +573,7 @@ bool ThreadList::WillResume() {
ThreadList run_me_only_list(m_process);
- run_me_only_list.SetStopID(m_process->GetStopID());
+ run_me_only_list.SetStopID(m_process.GetStopID());
// One or more threads might want to "Stop Others". We want to handle all
// those requests first. And if there is a thread that wanted to "resume
@@ -736,11 +734,12 @@ void ThreadList::NotifySelectedThreadChanged(lldb::tid_t tid) {
void ThreadList::Update(ThreadList &rhs) {
if (this != &rhs) {
- // Lock both mutexes to make sure neither side changes anyone on us while
- // the assignment occurs
- std::scoped_lock<std::recursive_mutex, std::recursive_mutex> guard(GetMutex(), rhs.GetMutex());
+ // Same process implies same mutex...
+ assert(&m_process == &rhs.m_process);
+ assert(&GetMutex() == &GetMutex());
+ // .. which means it's enough to lock one of them.
+ std::lock_guard<std::recursive_mutex> guard(GetMutex());
- m_process = rhs.m_process;
m_stop_id = rhs.m_stop_id;
m_threads.swap(rhs.m_threads);
m_selected_tid = rhs.m_selected_tid;
@@ -784,7 +783,7 @@ void ThreadList::Flush() {
}
std::recursive_mutex &ThreadList::GetMutex() const {
- return m_process->m_thread_mutex;
+ return m_process.m_thread_mutex;
}
ThreadList::ExpressionExecutionThreadPusher::ExpressionExecutionThreadPusher(
>From 5b0b44435bdfd85b61167cad160089e3890a9416 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Thu, 11 Jul 2024 12:00:17 +0000
Subject: [PATCH 2/2] fixups
---
lldb/include/lldb/Target/ThreadList.h | 4 ++--
lldb/source/Target/ThreadList.cpp | 14 ++++++++------
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/lldb/include/lldb/Target/ThreadList.h b/lldb/include/lldb/Target/ThreadList.h
index 91c4317cb8571..f931bb83a8cea 100644
--- a/lldb/include/lldb/Target/ThreadList.h
+++ b/lldb/include/lldb/Target/ThreadList.h
@@ -33,7 +33,7 @@ class ThreadList : public ThreadCollection {
~ThreadList() override;
- // Precondition: both thread lists must be belong to the same process.
+ /// Precondition: both thread lists must be belong to the same process.
const ThreadList &operator=(const ThreadList &rhs);
uint32_t GetSize(bool can_update = true);
@@ -136,7 +136,7 @@ class ThreadList : public ThreadCollection {
std::recursive_mutex &GetMutex() const override;
- // Precondition: both thread lists must be belong to the same process.
+ /// Precondition: both thread lists must be belong to the same process.
void Update(ThreadList &rhs);
protected:
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 448e2c299d7d4..1a2d7dd61c778 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -36,10 +36,11 @@ ThreadList::ThreadList(const ThreadList &rhs)
const ThreadList &ThreadList::operator=(const ThreadList &rhs) {
if (this != &rhs) {
- // Same process implies same mutex...
+ // We only allow assignments between thread lists describing the same
+ // process. Same process implies same mutex, which means it's enough to lock
+ // just the current object.
assert(&m_process == &rhs.m_process);
- assert(&GetMutex() == &GetMutex());
- // .. which means it's enough to lock one of them.
+ assert(&GetMutex() == &rhs.GetMutex());
std::lock_guard<std::recursive_mutex> guard(GetMutex());
m_stop_id = rhs.m_stop_id;
@@ -734,10 +735,11 @@ void ThreadList::NotifySelectedThreadChanged(lldb::tid_t tid) {
void ThreadList::Update(ThreadList &rhs) {
if (this != &rhs) {
- // Same process implies same mutex...
+ // We only allow assignments between thread lists describing the same
+ // process. Same process implies same mutex, which means it's enough to lock
+ // just the current object.
assert(&m_process == &rhs.m_process);
- assert(&GetMutex() == &GetMutex());
- // .. which means it's enough to lock one of them.
+ assert(&GetMutex() == &rhs.GetMutex());
std::lock_guard<std::recursive_mutex> guard(GetMutex());
m_stop_id = rhs.m_stop_id;
More information about the lldb-commits
mailing list