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

[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) {
----------------
jimingham wrote:

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?

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