[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
Fri Dec 20 18:44:23 PST 2024
https://github.com/rocallahan created https://github.com/llvm/llvm-project/pull/120817
These changes are designed to not change any behavior, but to make it easy to add code to choose the direction of execution after we've identified which thread(s) to run but before we add any `ThreadPlanStepOverBreakpoint`s. And honestly I think they make the existing code a bit clearer.
>From b021a56268c74010eae017d2d1169ab8b79978b3 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/7] [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..b44e53e0673b47 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 5164b418ea8e606729b24d0705f70e040aed8757 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/7] [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 0fe7e983f0c194dddb22059c4dd054dcb55d98a6 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/7] [lldb] Use `thread_sp` in more places in
`ThreadList::WillResume()`
This makes the code prettier and more consistent.
---
lldb/source/Target/ThreadList.cpp | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index b44e53e0673b47..676596978cf8cf 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) {
+ ThreadSP thread_sp(*pos);
lldbassert((*pos)->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 &&
+ ThreadSP thread_sp(*pos);
+ if (thread_sp->GetResumeState() != eStateSuspended &&
(!wants_solo_run || (*pos)->GetCurrentPlan()->StopOthers())) {
- if ((*pos)->IsOperatingSystemPluginThread() &&
- !(*pos)->GetBackingThread())
+ 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 070a2561e5ee3e7b083926ac1b1ff7fb3c2483ca 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/7] [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 676596978cf8cf..8edbd13c64a8b8 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 || (*pos)->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 || (*pos)->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 e366e6e655d9f74d70b873ae9c13cb788633689d 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/7] [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 8edbd13c64a8b8..6d5fadd360f5fb 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((*pos)->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 || (*pos)->GetCurrentPlan()->StopOthers())) {
if (thread_sp->IsOperatingSystemPluginThread() &&
>From 123c34662697dd13d43f4682dc9fcd07c3970e81 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/7] [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 | 32 +++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 6d5fadd360f5fb..df96ac59e63eab 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -556,6 +556,20 @@ 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
@@ -580,12 +594,15 @@ bool ThreadList::WillResume() {
run_me_only_list.AddThread(thread_sp);
if (!(stop_others_thread_sp && stop_others_thread_sp->ShouldRunBeforePublicStop())) {
- if (thread_sp == GetSelectedThread())
+ 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;
}
}
@@ -622,19 +639,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 68b8c34095430e65ce9d69e8fb4c7e70132b8f0c 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/7] [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 | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index df96ac59e63eab..2e62e46f886f4c 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 =
@@ -593,15 +589,12 @@ bool ThreadList::WillResume() {
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_to_run && thread_to_run->ShouldRunBeforePublicStop())) {
+ if (thread_sp == GetSelectedThread())
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;
}
More information about the lldb-commits
mailing list