Fix raciness in the check for whether a stop hook has run the target

[jingham]

Summary

This was originally done by checking the private state to see if it was "eStateRunning" or not. If it is running we need mark the stop event we're currently processing as "restarted" so the code like Process::ResumeSynchronous will know to keep waiting for a real stop.

But that is racy because between the time that the process gets restarted and the time we do the check, the process could have stopped again.

I used the private state because where the stop-hooks are run we are in the process of computing Public state so it won't show the effect of the restart yet.

Instead, this patch switches to just asking the stop hook whether it ran or not, and then marking the stop event as restarted or not based on the answer. The CommandLine stop hooks know this definitively - or at least they do provided the command return status is returned correctly. The Python stop hooks shouldn't be directly restarting the target, they should use the return values to request a restart.

I'll deal later on with the cases where (a) a command returns the wrong status and (b) somebody runs Continue/etc in a scripted stop hook even though they shouldn't by adding a mode to the Process where the high level functions that could run the target don't but instead mark an intention to continue the target. We will turn that on when we start processing stop actions, and then when we're done with all the stop actions, check whether everybody agrees we should continue.

That will also solve the unfairness problem in the stop actions, where the first stop action that runs a "continue" command, or an SB API that continues the target causes all the other stop actions don't get a chance to run. I've added auto-continue flags and for python return values to request running so that you CAN do the right thing. But it would be better to make the right thing happen automatically.

But I'm leaving that for a separate patch because it's a much bigger chunk of work, and this should be sufficient for now - only failing when people do things they shouldn't do...

[JDevlieghere]

LGTM

lldb/source/Target/Target.cpp

2648–2651

[labath]

This explains the things I saw in the log file. Thanks for fixing it.

lldb/include/lldb/Target/Target.h

public:
 
   // Target Stop Hooks
   class StopHook : public UserID {
   public:
     StopHook(const StopHook &rhs);
     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; }
 
     // Set the specifier.  The stop hook will own the specifier, and is
     // responsible for deleting it when we're done.
     void SetSpecifier(SymbolContextSpecifier *specifier);
 
     SymbolContextSpecifier *GetSpecifier() { return m_specifier_sp.get(); }
 
     bool ExecutionContextPasses(const ExecutionContext &exe_ctx);
 
     // Called on stop, this gets passed the ExecutionContext for each "stop
     // 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,
+    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.
     void SetThreadSpecifier(ThreadSpec *specifier);
 
     ThreadSpec *GetThreadSpecifier() { return m_thread_spec_up.get(); }
 
     bool IsActive() { return m_active; }
public:
   class StopHookCommandLine : public StopHook {
   public:
     virtual ~StopHookCommandLine() = default;
 
     StringList &GetCommands() { return m_commands; }
     void SetActionFromString(const std::string &strings);
     void SetActionFromStrings(const std::vector<std::string> &strings);
 
-    bool HandleStop(ExecutionContext &exc_ctx,
+    StopHookResult HandleStop(ExecutionContext &exc_ctx,
                               lldb::StreamSP output_sp) override;
     void GetSubclassDescription(Stream *s,
                                 lldb::DescriptionLevel level) const override;
 
   private:
     StringList m_commands;
     // Use CreateStopHook to make a new empty stop hook. The GetCommandPointer
     // and fill it with commands, and SetSpecifier to set the specifier shared
     // pointer (can be null, that will match anything.)
     StopHookCommandLine(lldb::TargetSP target_sp, lldb::user_id_t uid)
         : StopHook(target_sp, uid) {}
     friend class 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);
 
     void GetSubclassDescription(Stream *s,
                                 lldb::DescriptionLevel level) const override;
 
   private:
public:
   /// Add an empty stop hook to the Target's stop hook list, and returns a
   /// shared pointer to it in new_hook. Returns the id of the new hook.
   StopHookSP CreateStopHook(StopHook::StopHookKind kind);
 
   /// If you tried to create a stop hook, and that failed, call this to
   /// 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();
 
   bool SetSuppresStopHooks(bool suppress) {
     bool old_value = m_suppress_stop_hooks;
     m_suppress_stop_hooks = suppress;
     return old_value;
   }

lldb/source/Target/Process.cpp

bool hijacked = process_sp->IsHijackedForEvent(eBroadcastBitStateChanged) &&
                     !process_sp->StateChangedIsHijackedForSynchronousResume();
 
     if (!hijacked) {
       // If we didn't restart, run the Stop Hooks here.
       // Don't do that if state changed events aren't hooked up to the
       // 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);
     }
   }
 }
 
 void Process::ProcessEventData::Dump(Stream *s) const {
   ProcessSP process_sp(m_process_wp.lock());
 

lldb/source/Target/Target.cpp

 
 void Target::SetAllStopHooksActiveState(bool active_state) {
   StopHookCollection::iterator pos, end = m_stop_hooks.end();
   for (pos = m_stop_hooks.begin(); pos != end; pos++) {
     (*pos).second->SetIsActive(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;
   for (auto hook : m_stop_hooks) {
     if (hook.second->IsActive()) {
       any_active_hooks = true;
       break;
     }
   }
   if (!any_active_hooks)
-    return;
+    return false;
 
   std::vector<ExecutionContext> exc_ctx_with_reasons;
 
   ThreadList &cur_threadlist = m_process_sp->GetThreadList();
   size_t num_threads = cur_threadlist.GetSize();
   for (size_t i = 0; i < num_threads; i++) {
     lldb::ThreadSP cur_thread_sp = cur_threadlist.GetThreadAtIndex(i);
     if (cur_thread_sp->ThreadStoppedForAReason()) {
       lldb::StackFrameSP cur_frame_sp = cur_thread_sp->GetStackFrameAtIndex(0);
       exc_ctx_with_reasons.emplace_back(m_process_sp.get(), cur_thread_sp.get(),
                                         cur_frame_sp.get());
     }
   }
 
   // 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();
 
   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 should_stop = false;
for (auto exc_ctx : exc_ctx_with_reasons) {
           output_sp->Printf("\n- Hook %" PRIu64 "\n", cur_hook_sp->GetID());
         any_thread_matched = true;
       }
 
       if (print_thread_header)
         output_sp->Printf("-- Thread %d\n",
                           exc_ctx.GetThreadPtr()->GetIndexID());
 
-      bool this_should_stop = cur_hook_sp->HandleStop(exc_ctx, output_sp);
+      StopHook::StopHookResult this_result =
+          cur_hook_sp->HandleStop(exc_ctx, output_sp);
+      bool this_should_stop = true;
+
+      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;
 
-      // If anybody wanted to stop, we should all stop.
-      if (!should_stop)
-        should_stop = this_should_stop;
-
-      // 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 "
                           "stop hooks auto-continue.\n",
                           cur_hook_sp->GetID());
         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() {
   // NOTE: intentional leak so we don't crash if global destructor chain gets
   // called as other threads still use the result of this function
   static TargetPropertiesSP *g_settings_sp_ptr =
       new TargetPropertiesSP(new TargetProperties(nullptr));
   return *g_settings_sp_ptr;

 }
 
 void Target::StopHookCommandLine::SetActionFromStrings(
     const std::vector<std::string> &strings) {
   for (auto string : strings)
     GetCommands().AppendString(string.c_str());
 }
 
-bool Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
-                                             StreamSP output_sp) {
+Target::StopHook::StopHookResult
+Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
   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);
   Debugger &debugger = exc_ctx.GetTargetPtr()->GetDebugger();
   CommandInterpreterRunOptions options;
   options.SetStopOnContinue(true);
   options.SetStopOnError(true);
   options.SetEchoCommands(false);
   options.SetPrintResults(true);
   options.SetPrintErrors(true);
   options.SetAddToHistory(false);
 
   // Force Async:
   bool old_async = debugger.GetAsyncExecution();
   debugger.SetAsyncExecution(true);
   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
 Status Target::StopHookScripted::SetScriptCallback(
     std::string class_name, StructuredData::ObjectSP extra_args_sp) {
   Status error;
 
   ScriptInterpreter *script_interp =
if (extra_args_sp)
     m_extra_args->SetObjectSP(extra_args_sp);
 
   m_implementation_sp = script_interp->CreateScriptedStopHook(
       GetTarget(), m_class_name.c_str(), m_extra_args, error);
 
   return error;
 }
 
-bool Target::StopHookScripted::HandleStop(ExecutionContext &exc_ctx,
-                                          StreamSP output_sp) {
+Target::StopHook::StopHookResult
+Target::StopHookScripted::HandleStop(ExecutionContext &exc_ctx,
   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(
     Stream *s, lldb::DescriptionLevel level) const {
   if (level == eDescriptionLevelBrief) {
     s->PutCString(m_class_name);
     return;
   }

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

class TestStopHooks(TestBase):
     def test_stop_hooks_scripted_auto_continue(self):
         """Test that the --auto-continue flag works"""
         self.do_test_auto_continue(False)
 
     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,
         # so we should end up stopped in main, having run the expression only once.
         self.script_setup()
         
         result = lldb.SBCommandReturnObject()