[Lldb-commits] [lldb] Refactor `ThreadList::WillResume()` to prepare to support reverse execution (PR #120817)
Robert O'Callahan via lldb-commits
lldb-commits at lists.llvm.org
Mon Jan 13 21:06:01 PST 2025
https://github.com/rocallahan updated https://github.com/llvm/llvm-project/pull/120817
>From a3968690f394f15083839ea91e9908d6d7d1e886 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Tue, 17 Dec 2024 23:40:34 +0000
Subject: [PATCH 1/8] [lldb] Move thread-notification setup to happen later
This lets us check `run_me_only_list.GetSize()` instead of `wants_solo_run`.
---
lldb/source/Target/ThreadList.cpp | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 1a2d7dd61c778f..bfb5ba8cda3331 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -540,20 +540,6 @@ bool ThreadList::WillResume() {
}
}
- if (wants_solo_run) {
- Log *log = GetLog(LLDBLog::Step);
- if (log && log->GetVerbose())
- LLDB_LOGF(log, "Turning on notification of new threads while single "
- "stepping a thread.");
- 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();
- }
-
// Give all the threads that are likely to run a last chance to set up their
// state before we negotiate who is actually going to get a chance to run...
// Don't set to resume suspended threads, and if any thread wanted to stop
@@ -607,6 +593,20 @@ bool ThreadList::WillResume() {
}
}
+ if (run_me_only_list.GetSize(false) > 0) {
+ Log *log = GetLog(LLDBLog::Step);
+ if (log && log->GetVerbose())
+ LLDB_LOGF(log, "Turning on notification of new threads while single "
+ "stepping a thread.");
+ 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();
+ }
+
bool need_to_resume = true;
if (run_me_only_list.GetSize(false) == 0) {
>From 4cd680f53b1733a864a472c1c33cdf358ce469b9 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Tue, 17 Dec 2024 23:46:01 +0000
Subject: [PATCH 2/8] [lldb] Make `Thread::WillResume()` report whether it
pushed a `ThreadPlanStepOverBreakpoint`
We'll need this later to determine whether we need to account for
the newly-pushed `ThreadPlanStepOverBreakpoint`.
---
lldb/include/lldb/Target/Thread.h | 13 ++++++++-----
lldb/source/Target/Thread.cpp | 6 ++++--
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 38b65b2bc58490..ef66fa11574db9 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -200,11 +200,14 @@ class Thread : public std::enable_shared_from_this<Thread>,
/// The User resume state for this thread.
lldb::StateType GetResumeState() const { return m_resume_state; }
- // This function is called on all the threads before "ShouldResume" and
- // "WillResume" in case a thread needs to change its state before the
- // ThreadList polls all the threads to figure out which ones actually will
- // get to run and how.
- void SetupForResume();
+ /// This function is called on all the threads before "ShouldResume" and
+ /// "WillResume" in case a thread needs to change its state before the
+ /// ThreadList polls all the threads to figure out which ones actually will
+ /// get to run and how.
+ ///
+ /// \return
+ /// True if we pushed a ThreadPlanStepOverBreakpoint
+ bool SetupForResume();
// Do not override this function, it is for thread plan logic only
bool ShouldResume(lldb::StateType resume_state);
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index a6130f6b925bbf..b5261310970611 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -617,7 +617,7 @@ void Thread::WillStop() {
current_plan->WillStop();
}
-void Thread::SetupForResume() {
+bool Thread::SetupForResume() {
if (GetResumeState() != eStateSuspended) {
// First check whether this thread is going to "actually" resume at all.
// For instance, if we're stepping from one level to the next of an
@@ -625,7 +625,7 @@ void Thread::SetupForResume() {
// without actually running this thread. In that case, for this thread we
// shouldn't push a step over breakpoint plan or do that work.
if (GetCurrentPlan()->IsVirtualStep())
- return;
+ return false;
// If we're at a breakpoint push the step-over breakpoint plan. Do this
// before telling the current plan it will resume, since we might change
@@ -663,11 +663,13 @@ void Thread::SetupForResume() {
step_bp_plan->SetAutoContinue(true);
}
QueueThreadPlan(step_bp_plan_sp, false);
+ return true;
}
}
}
}
}
+ return false;
}
bool Thread::ShouldResume(StateType resume_state) {
>From a3913231cd99e8309ef3411c5fc107552789f585 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Tue, 17 Dec 2024 23:47:54 +0000
Subject: [PATCH 3/8] [lldb] Use `thread_sp` in more places in
`ThreadList::WillResume()`
This makes the code prettier and more consistent.
---
lldb/source/Target/ThreadList.cpp | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index bfb5ba8cda3331..3474a70d4dc617 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -528,12 +528,13 @@ bool ThreadList::WillResume() {
bool wants_solo_run = false;
for (pos = m_threads.begin(); pos != end; ++pos) {
- lldbassert((*pos)->GetCurrentPlan() &&
+ ThreadSP thread_sp(*pos);
+ lldbassert(thread_sp->GetCurrentPlan() &&
"thread should not have null thread plan");
- if ((*pos)->GetResumeState() != eStateSuspended &&
- (*pos)->GetCurrentPlan()->StopOthers()) {
- if ((*pos)->IsOperatingSystemPluginThread() &&
- !(*pos)->GetBackingThread())
+ if (thread_sp->GetResumeState() != eStateSuspended &&
+ thread_sp->GetCurrentPlan()->StopOthers()) {
+ if (thread_sp->IsOperatingSystemPluginThread() &&
+ !thread_sp->GetBackingThread())
continue;
wants_solo_run = true;
break;
@@ -546,12 +547,13 @@ bool ThreadList::WillResume() {
// others, only call setup on the threads that request StopOthers...
for (pos = m_threads.begin(); pos != end; ++pos) {
- if ((*pos)->GetResumeState() != eStateSuspended &&
- (!wants_solo_run || (*pos)->GetCurrentPlan()->StopOthers())) {
- if ((*pos)->IsOperatingSystemPluginThread() &&
- !(*pos)->GetBackingThread())
+ ThreadSP thread_sp(*pos);
+ if (thread_sp->GetResumeState() != eStateSuspended &&
+ (!wants_solo_run || thread_sp->GetCurrentPlan()->StopOthers())) {
+ if (thread_sp->IsOperatingSystemPluginThread() &&
+ !thread_sp->GetBackingThread())
continue;
- (*pos)->SetupForResume();
+ thread_sp->SetupForResume();
}
}
@@ -574,8 +576,8 @@ bool ThreadList::WillResume() {
ThreadSP thread_sp(*pos);
if (thread_sp->GetResumeState() != eStateSuspended &&
thread_sp->GetCurrentPlan()->StopOthers()) {
- if ((*pos)->IsOperatingSystemPluginThread() &&
- !(*pos)->GetBackingThread())
+ if (thread_sp->IsOperatingSystemPluginThread() &&
+ !thread_sp->GetBackingThread())
continue;
// You can't say "stop others" and also want yourself to be suspended.
>From ee110d5e96d8a00d53c906664081abaaf403e4a6 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Tue, 17 Dec 2024 23:55:19 +0000
Subject: [PATCH 4/8] [lldb] Move `SetupForResume()` calls to happen after the
first pass populating `run_me_only_list`
Eventually we will need to determine the execution direction after populating
`run_me_only_list` and before calling `SetupForResume()`.
---
lldb/source/Target/ThreadList.cpp | 46 ++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 3474a70d4dc617..c9d68773a8860a 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -541,22 +541,6 @@ bool ThreadList::WillResume() {
}
}
- // Give all the threads that are likely to run a last chance to set up their
- // state before we negotiate who is actually going to get a chance to run...
- // Don't set to resume suspended threads, and if any thread wanted to stop
- // others, only call setup on the threads that request StopOthers...
-
- for (pos = m_threads.begin(); pos != end; ++pos) {
- ThreadSP thread_sp(*pos);
- if (thread_sp->GetResumeState() != eStateSuspended &&
- (!wants_solo_run || thread_sp->GetCurrentPlan()->StopOthers())) {
- if (thread_sp->IsOperatingSystemPluginThread() &&
- !thread_sp->GetBackingThread())
- continue;
- thread_sp->SetupForResume();
- }
- }
-
// Now go through the threads and see if any thread wants to run just itself.
// if so then pick one and run it.
@@ -595,6 +579,36 @@ bool ThreadList::WillResume() {
}
}
+ // Give all the threads that are likely to run a last chance to set up their
+ // state before we negotiate who is actually going to get a chance to run...
+ // Don't set to resume suspended threads, and if any thread wanted to stop
+ // others, only call setup on the threads that request StopOthers...
+ for (pos = m_threads.begin(); pos != end; ++pos) {
+ ThreadSP thread_sp(*pos);
+ if (thread_sp->GetResumeState() != eStateSuspended &&
+ (!wants_solo_run || thread_sp->GetCurrentPlan()->StopOthers())) {
+ if (thread_sp->IsOperatingSystemPluginThread() &&
+ !thread_sp->GetBackingThread())
+ continue;
+ if (thread_sp->SetupForResume()) {
+ // You can't say "stop others" and also want yourself to be suspended.
+ assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
+ run_me_only_list.AddThread(thread_sp);
+
+ if (!(stop_others_thread_sp && stop_others_thread_sp->ShouldRunBeforePublicStop())) {
+ if (thread_sp == GetSelectedThread())
+ stop_others_thread_sp = thread_sp;
+
+ if (thread_sp->ShouldRunBeforePublicStop()) {
+ // This takes precedence, so if we find one of these, service it:
+ stop_others_thread_sp = thread_sp;
+ break;
+ }
+ }
+ }
+ }
+ }
+
if (run_me_only_list.GetSize(false) > 0) {
Log *log = GetLog(LLDBLog::Step);
if (log && log->GetVerbose())
>From 2f468fc48f21e620a358749ed327764f419d87e8 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Wed, 18 Dec 2024 00:00:27 +0000
Subject: [PATCH 5/8] [lldb] Set `wants_solo_run` based on whether
`run_me_only_list` is empty
We no longer need a dedicated loop to set `wants_solo_run`.
---
lldb/source/Target/ThreadList.cpp | 32 ++++++++-----------------------
1 file changed, 8 insertions(+), 24 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index c9d68773a8860a..40b7ff3a2f5666 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -518,30 +518,7 @@ bool ThreadList::WillResume() {
collection::iterator pos, end = m_threads.end();
- // See if any thread wants to run stopping others. If it does, then we won't
- // setup the other threads for resume, since they aren't going to get a
- // chance to run. This is necessary because the SetupForResume might add
- // "StopOthers" plans which would then get to be part of the who-gets-to-run
- // negotiation, but they're coming in after the fact, and the threads that
- // are already set up should take priority.
-
- bool wants_solo_run = false;
-
- for (pos = m_threads.begin(); pos != end; ++pos) {
- ThreadSP thread_sp(*pos);
- lldbassert(thread_sp->GetCurrentPlan() &&
- "thread should not have null thread plan");
- if (thread_sp->GetResumeState() != eStateSuspended &&
- thread_sp->GetCurrentPlan()->StopOthers()) {
- if (thread_sp->IsOperatingSystemPluginThread() &&
- !thread_sp->GetBackingThread())
- continue;
- wants_solo_run = true;
- break;
- }
- }
-
- // Now go through the threads and see if any thread wants to run just itself.
+ // Go through the threads and see if any thread wants to run just itself.
// if so then pick one and run it.
ThreadList run_me_only_list(m_process);
@@ -583,8 +560,15 @@ bool ThreadList::WillResume() {
// state before we negotiate who is actually going to get a chance to run...
// Don't set to resume suspended threads, and if any thread wanted to stop
// others, only call setup on the threads that request StopOthers...
+ bool wants_solo_run = run_me_only_list.GetSize(false) > 0;
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
+ // See if any thread wants to run stopping others. If it does, then we
+ // won't setup the other threads for resume, since they aren't going to get
+ // a chance to run. This is necessary because the SetupForResume might add
+ // "StopOthers" plans which would then get to be part of the who-gets-to-run
+ // negotiation, but they're coming in after the fact, and the threads that
+ // are already set up should take priority.
if (thread_sp->GetResumeState() != eStateSuspended &&
(!wants_solo_run || thread_sp->GetCurrentPlan()->StopOthers())) {
if (thread_sp->IsOperatingSystemPluginThread() &&
>From 046a3bbe64c7c204945f6395a9ce124339699b25 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Wed, 18 Dec 2024 01:16:50 +0000
Subject: [PATCH 6/8] [lldb] Move `thread_to_run` selection to happen before
`SetupForResume()` is called
Later, we'll need to know which thread will run before we determine the
execution direction, which has to happen before we call `SetupForResume()`.
We fix up `thread_to_run` after calling `SetupForResume()`, if required.
---
lldb/source/Target/ThreadList.cpp | 59 +++++++++++++++----------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 40b7ff3a2f5666..cd1b466d182e8c 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -556,36 +556,46 @@ bool ThreadList::WillResume() {
}
}
+ ThreadSP thread_to_run;
+ if (run_me_only_list.GetSize(false) > 0) {
+ if (stop_others_thread_sp) {
+ thread_to_run = stop_others_thread_sp;
+ } else if (run_me_only_list.GetSize(false) == 1) {
+ thread_to_run = run_me_only_list.GetThreadAtIndex(0);
+ } else {
+ int random_thread =
+ (int)((run_me_only_list.GetSize(false) * (double)rand()) /
+ (RAND_MAX + 1.0));
+ thread_to_run = run_me_only_list.GetThreadAtIndex(random_thread);
+ }
+ }
+
// Give all the threads that are likely to run a last chance to set up their
// state before we negotiate who is actually going to get a chance to run...
// Don't set to resume suspended threads, and if any thread wanted to stop
// others, only call setup on the threads that request StopOthers...
- bool wants_solo_run = run_me_only_list.GetSize(false) > 0;
- for (pos = m_threads.begin(); pos != end; ++pos) {
- ThreadSP thread_sp(*pos);
+ if (thread_to_run != nullptr) {
// See if any thread wants to run stopping others. If it does, then we
// won't setup the other threads for resume, since they aren't going to get
// a chance to run. This is necessary because the SetupForResume might add
// "StopOthers" plans which would then get to be part of the who-gets-to-run
// negotiation, but they're coming in after the fact, and the threads that
// are already set up should take priority.
- if (thread_sp->GetResumeState() != eStateSuspended &&
- (!wants_solo_run || thread_sp->GetCurrentPlan()->StopOthers())) {
- if (thread_sp->IsOperatingSystemPluginThread() &&
- !thread_sp->GetBackingThread())
- continue;
- if (thread_sp->SetupForResume()) {
- // You can't say "stop others" and also want yourself to be suspended.
- assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
- run_me_only_list.AddThread(thread_sp);
-
- if (!(stop_others_thread_sp && stop_others_thread_sp->ShouldRunBeforePublicStop())) {
- if (thread_sp == GetSelectedThread())
- stop_others_thread_sp = thread_sp;
-
+ thread_to_run->SetupForResume();
+ } else {
+ for (pos = m_threads.begin(); pos != end; ++pos) {
+ ThreadSP thread_sp(*pos);
+ if (thread_sp->GetResumeState() != eStateSuspended) {
+ if (thread_sp->IsOperatingSystemPluginThread() &&
+ !thread_sp->GetBackingThread())
+ continue;
+ if (thread_sp->SetupForResume()) {
+ // You can't say "stop others" and also want yourself to be suspended.
+ assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
+ run_me_only_list.AddThread(thread_sp);
+ thread_to_run = thread_sp;
if (thread_sp->ShouldRunBeforePublicStop()) {
// This takes precedence, so if we find one of these, service it:
- stop_others_thread_sp = thread_sp;
break;
}
}
@@ -622,19 +632,6 @@ bool ThreadList::WillResume() {
need_to_resume = false;
}
} else {
- ThreadSP thread_to_run;
-
- if (stop_others_thread_sp) {
- thread_to_run = stop_others_thread_sp;
- } else if (run_me_only_list.GetSize(false) == 1) {
- thread_to_run = run_me_only_list.GetThreadAtIndex(0);
- } else {
- int random_thread =
- (int)((run_me_only_list.GetSize(false) * (double)rand()) /
- (RAND_MAX + 1.0));
- thread_to_run = run_me_only_list.GetThreadAtIndex(random_thread);
- }
-
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
if (thread_sp == thread_to_run) {
>From f1daf015441fb2bb98a6f60807208379e699c328 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Wed, 18 Dec 2024 01:24:23 +0000
Subject: [PATCH 7/8] [lldb] Replace `stop_others_thread_sp` with
`thread_to_run`
We don't need `stop_others_thread_sp` to be a separate variable anymore,
we can just use `thread_to_run` from the start.
---
lldb/source/Target/ThreadList.cpp | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index cd1b466d182e8c..ff1863c916cc2b 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -531,8 +531,7 @@ bool ThreadList::WillResume() {
// There are two special kinds of thread that have priority for "StopOthers":
// a "ShouldRunBeforePublicStop thread, or the currently selected thread. If
// we find one satisfying that critereon, put it here.
- ThreadSP stop_others_thread_sp;
-
+ ThreadSP thread_to_run;
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
if (thread_sp->GetResumeState() != eStateSuspended &&
@@ -546,21 +545,18 @@ bool ThreadList::WillResume() {
run_me_only_list.AddThread(thread_sp);
if (thread_sp == GetSelectedThread())
- stop_others_thread_sp = thread_sp;
-
+ thread_to_run = thread_sp;
+
if (thread_sp->ShouldRunBeforePublicStop()) {
// This takes precedence, so if we find one of these, service it:
- stop_others_thread_sp = thread_sp;
+ thread_to_run = thread_sp;
break;
}
}
}
- ThreadSP thread_to_run;
- if (run_me_only_list.GetSize(false) > 0) {
- if (stop_others_thread_sp) {
- thread_to_run = stop_others_thread_sp;
- } else if (run_me_only_list.GetSize(false) == 1) {
+ if (run_me_only_list.GetSize(false) > 0 && !thread_to_run) {
+ if (run_me_only_list.GetSize(false) == 1) {
thread_to_run = run_me_only_list.GetThreadAtIndex(0);
} else {
int random_thread =
>From 398c5046353e84d8d140ac73a6fa80c8146a11fa Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <rocallahan at google.com>
Date: Tue, 14 Jan 2025 04:33:17 +0000
Subject: [PATCH 8/8] [lldb] Replace check of `run_me_only_list.GetSize()` with
test of `thread_to_run` against null
This is clearer and it means we don't have to update `run_me_only_list` when `SetupForResume()`
returns true, which was confusing.
---
lldb/source/Target/ThreadList.cpp | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index ff1863c916cc2b..6cbef330bf4888 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -588,7 +588,6 @@ bool ThreadList::WillResume() {
if (thread_sp->SetupForResume()) {
// You can't say "stop others" and also want yourself to be suspended.
assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
- run_me_only_list.AddThread(thread_sp);
thread_to_run = thread_sp;
if (thread_sp->ShouldRunBeforePublicStop()) {
// This takes precedence, so if we find one of these, service it:
@@ -599,7 +598,7 @@ bool ThreadList::WillResume() {
}
}
- if (run_me_only_list.GetSize(false) > 0) {
+ if (thread_to_run != nullptr) {
Log *log = GetLog(LLDBLog::Step);
if (log && log->GetVerbose())
LLDB_LOGF(log, "Turning on notification of new threads while single "
@@ -615,7 +614,7 @@ bool ThreadList::WillResume() {
bool need_to_resume = true;
- if (run_me_only_list.GetSize(false) == 0) {
+ if (thread_to_run == nullptr) {
// Everybody runs as they wish:
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
More information about the lldb-commits
mailing list