[lldb] Fix ThreadList assignment race #98293
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThreadList 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. Full diff: https://github.com/llvm/llvm-project/pull/98293.diff 4 Files Affected:
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(
|
|
Previous attempt to fix this issue: https://reviews.llvm.org/D158034 |
JDevlieghere
approved these changes
Jul 10, 2024
There was a problem hiding this comment.
LGTM.
As a nice to have, maybe add a short explanation to the first mutex explaining that these operations are only allowed between thread lists belonging to the same process. Your current comment makes it sound as if that only applies if the processes are the same, but per the assertion, the requirement is stricter than that.
EDIT: Oh I see you had the comment in the header. That covers my concern, though I'd still add the same thing to the assert to quickly diagnose the issue if someone were to hit it.
augusto2112
reviewed
Jul 10, 2024
jimingham
approved these changes
Jul 10, 2024
There was a problem hiding this comment.
ThreadList differs from ThreadCollection in that it holds a process, and all the threads in the list must belong to that process. Things like having "ShouldStop" methods on ThreadList would make no sense for a list of threads from random processes. We don't ever reuse these as processes come and go in a target. So having the process seem optional or changeable in ThreadList was a mistake (probably mine).
This is more correct.
Elaborated the assert comment as well. |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.