[Lldb-commits] [lldb] Refactor `ThreadList::WillResume()` to prepare to support reverse execution (PR #120817)

[Robert O'Callahan via lldb-commits]

================
@@ -582,31 +531,92 @@ 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.
       assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
       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) {
----------------
rocallahan wrote:

> 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 need the second loop.

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