[Lldb-commits] [lldb] e1bd337 - [lldb] Fix ThreadList assignment race (#98293)
via lldb-commits
lldb-commits at lists.llvm.org
Thu Jul 11 05:04:24 PDT 2024
Author: Pavel Labath
Date: 2024-07-11T14:04:19+02:00
New Revision: e1bd337865fca9f455225ba37b76595d37bad213
URL: https://github.com/llvm/llvm-project/commit/e1bd337865fca9f455225ba37b76595d37bad213
DIFF: https://github.com/llvm/llvm-project/commit/e1bd337865fca9f455225ba37b76595d37bad213.diff
LOG: [lldb] Fix ThreadList assignment race (#98293)
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.
Added:
Modified:
lldb/include/lldb/Target/ThreadList.h
lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
lldb/source/Target/Process.cpp
lldb/source/Target/ThreadList.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/ThreadList.h b/lldb/include/lldb/Target/ThreadList.h
index 6af04f8ffc376..f931bb83a8cea 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..1a2d7dd61c778 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,13 @@ 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;
+ // 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() == &rhs.GetMutex());
+ 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 +83,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 +91,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 +103,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 +121,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 +139,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 +159,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 +209,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 +240,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 +376,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 +424,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 +437,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 +485,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 +514,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 +545,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 +574,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 +735,13 @@ 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());
+ // 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() == &rhs.GetMutex());
+ 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 +785,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(
More information about the lldb-commits
mailing list