Simplify Target::RunStopHooks()

[yln]

Introduce StopHookResult::NoPreference and
simplify control flow in Target::RunStopHooks().

The algorithm is (in order):

1. "Auto continue" set on any hook -> continue
2. "Stop demanded" by any hook -> stop
3. "Continue requested" by any hook -> continue
4. No hooks, or "no preference" only (default
   stance) -> stop

The new NoPreference lets us keep the default
stance, distinguishing case 3. and 4.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Julian Lettner (yln)

Changes

Introduce StopHookResult::NoPreference and
simplify control flow in Target::RunStopHooks().

The algorithm is (in order):

1. "Auto continue" set on any hook -> continue
2. "Stop" demanded by any hook -> stop
3. "Continue" requested by any hook -> continue
4. Default is to stop

The new NoPreference lets us keep the default
distinguishing case 3. and 4.

---

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

2 Files Affected:

• (modified) lldb/include/lldb/Target/Target.h (+1)
• (modified) lldb/source/Target/Target.cpp (+23-54)

diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index f31ac381391b4..0bff7220952fb 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1317,6 +1317,7 @@ class Target : public std::enable_shared_from_this<Target>,
     enum class StopHookResult : uint32_t {
       KeepStopped = 0,
       RequestContinue,
+      NoPreference,
       AlreadyContinued
     };
 
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 550424720e095..5f5163f3b907d 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -69,6 +69,7 @@
 #include "llvm/ADT/SetVector.h"
 #include "llvm/Support/ThreadPool.h"
 
+#include <algorithm>
 #include <memory>
 #include <mutex>
 #include <optional>
@@ -3033,15 +3034,9 @@ bool Target::RunStopHooks() {
   if (m_stop_hooks.empty())
     return false;
 
-  // If there aren't any active stop hooks, don't bother either.
-  bool any_active_hooks = false;
-  for (auto hook : m_stop_hooks) {
-    if (hook.second->IsActive()) {
-      any_active_hooks = true;
-      break;
-    }
-  }
-  if (!any_active_hooks)
+  bool no_active = std::none_of(m_stop_hooks.cbegin(), m_stop_hooks.cend(),
+                                [](auto &p) { return p.second->IsActive(); });
+  if (no_active)
     return false;
 
   // Make sure we check that we are not stopped because of us running a user
@@ -3075,13 +3070,13 @@ bool Target::RunStopHooks() {
     return false;
 
   StreamSP output_sp = m_debugger.GetAsyncOutputStream();
+  auto on_exit = llvm::make_scope_exit([output_sp] { output_sp->Flush(); });
 
-  bool auto_continue = false;
-  bool hooks_ran = false;
   bool print_hook_header = (m_stop_hooks.size() != 1);
   bool print_thread_header = (num_exe_ctx != 1);
+  bool auto_continue = false;
   bool should_stop = false;
-  bool somebody_restarted = false;
+  bool requested_continue= false;
 
   for (auto stop_entry : m_stop_hooks) {
     StopHookSP cur_hook_sp = stop_entry.second;
@@ -3090,11 +3085,6 @@ bool Target::RunStopHooks() {
 
     bool any_thread_matched = false;
     for (auto exc_ctx : exc_ctx_with_reasons) {
-      // We detect somebody restarted in the stop-hook loop, and broke out of
-      // that loop back to here.  So break out of here too.
-      if (somebody_restarted)
-        break;
-
       if (!cur_hook_sp->ExecutionContextPasses(exc_ctx))
         continue;
 
@@ -3120,59 +3110,38 @@ bool Target::RunStopHooks() {
         output_sp->Printf("-- Thread %d\n",
                           exc_ctx.GetThreadPtr()->GetIndexID());
 
-      StopHook::StopHookResult this_result =
-          cur_hook_sp->HandleStop(exc_ctx, output_sp);
-      bool this_should_stop = true;
-
-      switch (this_result) {
+      auto result = cur_hook_sp->HandleStop(exc_ctx, output_sp);
+      switch (result) {
       case StopHook::StopHookResult::KeepStopped:
-        // If this hook is set to auto-continue that should override the
-        // HandleStop result...
-        if (cur_hook_sp->GetAutoContinue())
-          this_should_stop = false;
-        else
-          this_should_stop = true;
-
+        should_stop = true;
         break;
       case StopHook::StopHookResult::RequestContinue:
-        this_should_stop = false;
+        requested_continue = true;
+        break;
+      case StopHook::StopHookResult::NoPreference:
+        // Do nothing
         break;
       case StopHook::StopHookResult::AlreadyContinued:
         // We don't have a good way to prohibit people from restarting the
         // target willy nilly in a stop hook.  If the hook did so, give a
-        // gentle suggestion here and bag out if the hook processing.
+        // gentle suggestion here and back out of the hook processing.
         output_sp->Printf("\nAborting stop hooks, hook %" PRIu64
                           " set the program running.\n"
                           "  Consider using '-G true' to make "
                           "stop hooks auto-continue.\n",
                           cur_hook_sp->GetID());
-        somebody_restarted = true;
-        break;
+        // FIXME: if we are doing non-stop mode for real, we would have to
+        // check that OUR thread was restarted, otherwise we should keep
+        // processing stop hooks.
+        return true;
       }
-      // If we're already restarted, stop processing stop hooks.
-      // FIXME: if we are doing non-stop mode for real, we would have to
-      // check that OUR thread was restarted, otherwise we should keep
-      // processing stop hooks.
-      if (somebody_restarted)
-        break;
-
-      // If anybody wanted to stop, we should all stop.
-      if (!should_stop)
-        should_stop = this_should_stop;
     }
   }
 
-  output_sp->Flush();
-
-  // If one of the commands in the stop hook already restarted the target,
-  // report that fact.
-  if (somebody_restarted)
-    return true;
-
-  // Finally, if auto-continue was requested, do it now:
-  // We only compute should_stop against the hook results if a hook got to run
-  // which is why we have to do this conjoint test.
-  if ((hooks_ran && !should_stop) || auto_continue) {
+  // Resume iff:
+  // 1) At least one hook requested to continue and no hook asked to stop, or
+  // 2) at least one hook had auto continue on.
+  if ((requested_continue && !should_stop) || auto_continue) {
     Log *log = GetLog(LLDBLog::Process);
     Status error = m_process_sp->PrivateResume();
     if (error.Success()) {

[jimingham]

Looking at the revised version I realize that both versions treatment of auto_continue doesn't make much sense. This treats any stop-hook with --auto-continue set when it was made to override all the other stop hook's opinions about whether to stop or not.

That wasn't the intention of auto_continue. It was added because if you write a command-based stop hook there wasn't a way to request that the process continue, you had to emit an explicit continue in your stop hook. But that restarted the target too early, preventing other stop hooks from running.

I don't think it's a good idea to have a stop hook that can keep any others from actually stopping. That doesn't seem desirable. And I especially don't think it's a good idea that the only convenient way to make a command based stop hook auto-continue override the other stop hooks.

I know you didn't add this behavior, but since you're working in this code, it might be good to get that right.

I think what should happen is not to accumulate the auto_continue flag, but rather, if a particular stop-hook is set to auto-continue, then we should discard the result of its HandleStop, and force RequestContinue.

[yln]

@jimingham Just simplified what was there, documenting and preserving existing behavior. Do you want to fix the handling of auto_continue as a follow-up?

[jimingham]

Fine

[jimingham]

LGTM