Fix raciness in the StopHook check for "has the target run".

This was looking at the privateState, but it's possible that
the actual process has started up and then stopped again by the
time we get to the check, which would lead us to get out of running
the stop hooks too early.

Instead we need to track the intention of the stop hooks directly.

Differential Revision: https://reviews.llvm.org/D88753

Author: jimingham

lldb/include/lldb/Target/Target.h

@@ -1145,6 +1145,11 @@ class Target : public std::enable_shared_from_this<Target>,
     virtual ~StopHook() = default;
 
     enum class StopHookKind  : uint32_t { CommandBased = 0, ScriptBased };
+    enum class StopHookResult : uint32_t {
+      KeepStopped = 0,
+      RequestContinue,
+      AlreadyContinued
+    };
 
     lldb::TargetSP &GetTarget() { return m_target_sp; }
 
@@ -1160,8 +1165,8 @@ class Target : public std::enable_shared_from_this<Target>,
     // with a reason" thread.  It should add to the stream whatever text it
     // wants to show the user, and return False to indicate it wants the target
     // not to stop.
-    virtual bool HandleStop(ExecutionContext &exe_ctx,
-                            lldb::StreamSP output) = 0;
+    virtual StopHookResult HandleStop(ExecutionContext &exe_ctx,
+                                      lldb::StreamSP output) = 0;
 
     // Set the Thread Specifier.  The stop hook will own the thread specifier,
     // and is responsible for deleting it when we're done.
@@ -1201,8 +1206,8 @@ class Target : public std::enable_shared_from_this<Target>,
     void SetActionFromString(const std::string &strings);
     void SetActionFromStrings(const std::vector<std::string> &strings);
 
-    bool HandleStop(ExecutionContext &exc_ctx,
-                    lldb::StreamSP output_sp) override;
+    StopHookResult HandleStop(ExecutionContext &exc_ctx,
+                              lldb::StreamSP output_sp) override;
     void GetSubclassDescription(Stream *s,
                                 lldb::DescriptionLevel level) const override;
 
@@ -1219,7 +1224,8 @@ class Target : public std::enable_shared_from_this<Target>,
   class StopHookScripted : public StopHook {
   public:
     virtual ~StopHookScripted() = default;
-    bool HandleStop(ExecutionContext &exc_ctx, lldb::StreamSP output) override;
+    StopHookResult HandleStop(ExecutionContext &exc_ctx,
+                              lldb::StreamSP output) override;
 
     Status SetScriptCallback(std::string class_name,
                              StructuredData::ObjectSP extra_args_sp);
@@ -1254,7 +1260,9 @@ class Target : public std::enable_shared_from_this<Target>,
   /// remove the stop hook, as it will also reset the stop hook counter.
   void UndoCreateStopHook(lldb::user_id_t uid);
 
-  void RunStopHooks();
+  // Runs the stop hooks that have been registered for this target.
+  // Returns true if the stop hooks cause the target to resume.
+  bool RunStopHooks();
 
   size_t GetStopHookSize();
 

lldb/source/Target/Process.cpp

@@ -4178,8 +4178,7 @@ void Process::ProcessEventData::DoOnRemoval(Event *event_ptr) {
       // public (or SyncResume) broadcasters.  StopHooks are just for
       // real public stops.  They might also restart the target,
       // so watch for that.
-      process_sp->GetTarget().RunStopHooks();
-      if (process_sp->GetPrivateState() == eStateRunning)
+      if (process_sp->GetTarget().RunStopHooks())
         SetRestarted(true);
     }
   }

lldb/source/Target/Target.cpp

@@ -2541,25 +2541,26 @@ void Target::SetAllStopHooksActiveState(bool active_state) {
   }
 }
 
-void Target::RunStopHooks() {
+bool Target::RunStopHooks() {
   if (m_suppress_stop_hooks)
-    return;
+    return false;
 
   if (!m_process_sp)
-    return;
+    return false;
 
   // Somebody might have restarted the process:
+  // Still return false, the return value is about US restarting the target.
   if (m_process_sp->GetState() != eStateStopped)
-    return;
+    return false;
 
   // <rdar://problem/12027563> make sure we check that we are not stopped
   // because of us running a user expression since in that case we do not want
   // to run the stop-hooks
   if (m_process_sp->GetModIDRef().IsLastResumeForUserExpression())
-    return;
+    return false;
 
   if (m_stop_hooks.empty())
-    return;
+    return false;
 
   // If there aren't any active stop hooks, don't bother either.
   bool any_active_hooks = false;
@@ -2570,7 +2571,7 @@ void Target::RunStopHooks() {
     }
   }
   if (!any_active_hooks)
-    return;
+    return false;
 
   std::vector<ExecutionContext> exc_ctx_with_reasons;
 
@@ -2588,7 +2589,7 @@ void Target::RunStopHooks() {
   // If no threads stopped for a reason, don't run the stop-hooks.
   size_t num_exe_ctx = exc_ctx_with_reasons.size();
   if (num_exe_ctx == 0)
-    return;
+    return false;
 
   StreamSP output_sp = m_debugger.GetAsyncOutputStream();
 
@@ -2636,22 +2637,27 @@ void Target::RunStopHooks() {
         output_sp->Printf("-- Thread %d\n",
                           exc_ctx.GetThreadPtr()->GetIndexID());
 
-      bool this_should_stop = cur_hook_sp->HandleStop(exc_ctx, output_sp);
-      // If this hook is set to auto-continue that should override the
-      // HandleStop result...
-      if (cur_hook_sp->GetAutoContinue())
-        this_should_stop = false;
+      StopHook::StopHookResult this_result =
+          cur_hook_sp->HandleStop(exc_ctx, output_sp);
+      bool this_should_stop = true;
 
-      // If anybody wanted to stop, we should all stop.
-      if (!should_stop)
-        should_stop = this_should_stop;
+      switch (this_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;
 
-      // We don't have a good way to prohibit people from restarting the target
-      // willy nilly in a stop hook.  So see if the private state is running
-      // here and bag out if it is.
-      // FIXME: when we are doing non-stop mode for realz we'll have to instead
-      // track each thread, and only bag out if a thread is set running.
-      if (m_process_sp->GetPrivateState() != eStateStopped) {
+        break;
+      case StopHook::StopHookResult::RequestContinue:
+        this_should_stop = false;
+        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.
         output_sp->Printf("\nAborting stop hooks, hook %" PRIu64
                           " set the program running.\n"
                           "  Consider using '-G true' to make "
@@ -2660,16 +2666,42 @@ void Target::RunStopHooks() {
         somebody_restarted = true;
         break;
       }
+      // 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 (!somebody_restarted && ((hooks_ran && !should_stop) || auto_continue))
-    m_process_sp->PrivateResume();
+  if ((hooks_ran && !should_stop) || auto_continue) {
+    Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+    Status error = m_process_sp->PrivateResume();
+    if (error.Success()) {
+      LLDB_LOG(log, "Resuming from RunStopHooks");
+      return true;
+    } else {
+      LLDB_LOG(log, "Resuming from RunStopHooks failed: {0}", error);
+      return false;
+    }
+  }
+
+  return false;
 }
 
 const TargetPropertiesSP &Target::GetGlobalProperties() {
@@ -3235,13 +3267,14 @@ void Target::StopHookCommandLine::SetActionFromStrings(
     GetCommands().AppendString(string.c_str());
 }
 
-bool Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
-                                             StreamSP output_sp) {
+Target::StopHook::StopHookResult
+Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
+                                        StreamSP output_sp) {
   assert(exc_ctx.GetTargetPtr() && "Can't call PerformAction on a context "
                                    "with no target");
 
   if (!m_commands.GetSize())
-    return true;
+    return StopHookResult::KeepStopped;
 
   CommandReturnObject result(false);
   result.SetImmediateOutputStream(output_sp);
@@ -3260,8 +3293,11 @@ bool Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
   debugger.GetCommandInterpreter().HandleCommands(GetCommands(), &exc_ctx,
                                                   options, result);
   debugger.SetAsyncExecution(old_async);
-
-  return true;
+  lldb::ReturnStatus status = result.GetStatus();
+  if (status == eReturnStatusSuccessContinuingNoResult ||
+      status == eReturnStatusSuccessContinuingResult)
+    return StopHookResult::AlreadyContinued;
+  return StopHookResult::KeepStopped;
 }
 
 // Target::StopHookScripted
@@ -3289,20 +3325,22 @@ Status Target::StopHookScripted::SetScriptCallback(
   return error;
 }
 
-bool Target::StopHookScripted::HandleStop(ExecutionContext &exc_ctx,
-                                          StreamSP output_sp) {
+Target::StopHook::StopHookResult
+Target::StopHookScripted::HandleStop(ExecutionContext &exc_ctx,
+                                     StreamSP output_sp) {
   assert(exc_ctx.GetTargetPtr() && "Can't call HandleStop on a context "
                                    "with no target");
 
   ScriptInterpreter *script_interp =
       GetTarget()->GetDebugger().GetScriptInterpreter();
   if (!script_interp)
-    return true;
+    return StopHookResult::KeepStopped;
 
   bool should_stop = script_interp->ScriptedStopHookHandleStop(
       m_implementation_sp, exc_ctx, output_sp);
 
-  return should_stop;
+  return should_stop ? StopHookResult::KeepStopped
+                     : StopHookResult::RequestContinue;
 }
 
 void Target::StopHookScripted::GetSubclassDescription(

lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py

@@ -71,8 +71,6 @@ def test_stop_hooks_scripted_return_false(self):
         """Test that the returning False from a stop hook works"""
         self.do_test_auto_continue(True)
 
-    # Test is flakey on Linux.
-    @skipIfLinux
     def do_test_auto_continue(self, return_true):
         """Test that auto-continue works."""
         # We set auto-continue to 1 but the stop hook only applies to step_out_of_me,