Skip to content

[lldb] Fix ThreadList assignment race #98293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 11, 2024
Merged

[lldb] Fix ThreadList assignment race #98293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a594fe
[lldb] Fix ThreadList assignment race
labath Jul 10, 2024
6f74426
Merge remote-tracking branch 'origin/main' into threadlist
labath Jul 11, 2024
5b0b444
fixups
labath Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lldb/include/lldb/Target/ThreadList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
14 changes: 6 additions & 8 deletions lldb/source/Target/Process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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)) {
Expand Down
61 changes: 31 additions & 30 deletions lldb/source/Target/ThreadList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}

Expand All @@ -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;
Expand Down Expand Up @@ -84,15 +83,15 @@ 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();
}

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())
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
Loading