Refactor ThreadList::WillResume() to prepare to support reverse execution

[rocallahan]

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 ThreadPlanStepOverBreakpoints. And honestly I think they make the existing code a bit clearer.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Robert O'Callahan (rocallahan)

Changes

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 ThreadPlanStepOverBreakpoints. And honestly I think they make the existing code a bit clearer.

---

Full diff: https://github.com/llvm/llvm-project/pull/120817.diff

3 Files Affected:

• (modified) lldb/include/lldb/Target/Thread.h (+8-5)
• (modified) lldb/source/Target/Thread.cpp (+4-2)
• (modified) lldb/source/Target/ThreadList.cpp (+68-71)

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) {
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 1a2d7dd61c778f..2e62e46f886f4c 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -518,58 +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) {
-    lldbassert((*pos)->GetCurrentPlan() &&
-               "thread should not have null thread plan");
-    if ((*pos)->GetResumeState() != eStateSuspended &&
-        (*pos)->GetCurrentPlan()->StopOthers()) {
-      if ((*pos)->IsOperatingSystemPluginThread() &&
-          !(*pos)->GetBackingThread())
-        continue;
-      wants_solo_run = true;
-      break;
-    }
-  }
-
-  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
-  // 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())
-        continue;
-      (*pos)->SetupForResume();
-    }
-  }
-
-  // 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);
@@ -582,14 +531,13 @@ 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 &&
         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.
@@ -597,16 +545,78 @@ 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;
       }
     }
   }
 
+  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 =
+          (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);
+    // 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() &&
+          !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 (!(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:
+            thread_to_run = thread_sp;
+            break;
+          }
+        }
+      }
+    }
+  }
+
+  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) {
@@ -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) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jimingham]

I must admit I find this version harder to reason about than the original. Particularly, I find this second loop confusing. You have already figured out whether you are going to run solo and which thread you are going to run, by the time you get to this point. Then it looks like you do that computation a second time in a slightly different way, overwriting the decisions you made above in a fairly head-scratching way.

I would have expected at this point that if thread_to_run is not empty, you could just call SetupForResume on that one thread, and then just that thread will run. It's not at all clear to me what the purpose of redoing the computation is.

There's one difference between the two computations. In the first loop, you give every thread that wants to run solo a chance to be the one to do that and pick among them equally. Then in this loop if the thread you chose to run doesn't return true from SetupForResume (because it isn't at a breakpoint) but there's another wants to run solo thread that is at a breakpoint, we switch to that one.

You are changing behavior here because you prioritize threads that need to step over breakpoints in a way that we didn't before, but the logic seems unclear to me.

Can you say in words what behaviors you are intending to impose with this extra "am I stepping over a breakpoint" check?

[rocallahan]

I would have expected at this point that if thread_to_run is not empty, you could just call SetupForResume on that one thread, and then just that thread will run.

Yes, that's a good idea. I've done that in the new revision. It definitely helps.

You are changing behavior here because you prioritize threads that need to step over breakpoints in a way that we didn't before, but the logic seems unclear to me.

Putting the SetupForResume() loop behind a check that thread_to_run is non-empty addresses this.

Can you say in words what behaviors you are intending to impose with this extra "am I stepping over a breakpoint" check?

The goal is to decide which thread to run before we call SetupForResume(). I'm trying to change behavior as little as possible while achieving that.

Note that with the new code, there are only at most two loops over all threads, whereas the existing code before this PR always does three. And now, if the first loop identifies a thread to run because some thread returns true for StopOthers(), we don't do the second loop.

[rocallahan]

FYI the new code does change behavior --- when some threads return true for StopOthers(), the old code calls SetupForResume() on all threads that return true for StopOthers(), but now we only call SetupForResume() on the one thread we have chosen to run. I assume this is an improvement.

Also, when no threads return true for StopOthers() but multiple threads have breakpoints that need to be skipped, the old code would choose randomly between those threads, but the new code picks the last one we encounter looping over m_threads (if none return true for ShouldRunBeforePublicStop()). I assume this is innocuous...

[jimingham]

That looks better. The one functional change - not telling a bunch of threads to set themselves up for running when we know they aren't going to get to run - is better behavior, and I can't see how it would cause problems.

[rocallahan]

Great, thanks! Hopefully someone can merge this?

[jimingham]

Done.

[rocallahan]

Thanks!!