Fix the logic so stop-hooks get run after a breakpoint that ran an expression

Code was added to Target::RunStopHook to make sure that we don't run stop hooks when
you stop after an expression evaluation. But the way it was done was to check that we
hadn't run an expression since the last natural stop. That failed in the case where you
stopped for a breakpoint which had run an expression, because the stop-hooks get run
after the breakpoint actions, and so by the time we got to running the stop-hooks,
we had already run a user expression.

I fixed this by adding a target ivar tracking the last natural stop ID at which we had
run a stop-hook. Then we keep track of this and make sure we run the stop-hooks only
once per natural stop.

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

lldb/include/lldb/Target/Target.h

@@ -1429,8 +1429,10 @@ protected:
   typedef std::map<lldb::user_id_t, StopHookSP> StopHookCollection;
   StopHookCollection m_stop_hooks;
   lldb::user_id_t m_stop_hook_next_id;
+  uint32_t m_latest_stop_hook_id; /// This records the last natural stop at
+                                  /// which we ran a stop-hook.
   bool m_valid;
-  bool m_suppress_stop_hooks;
+  bool m_suppress_stop_hooks; /// Used to not run stop hooks for expressions
   bool m_is_dummy_target;
   unsigned m_next_persistent_variable_index = 0;
   /// An optional \a lldb_private::Trace object containing processor trace

lldb/source/Target/Target.cpp

@@ -95,6 +95,7 @@ Target::Target(Debugger &debugger, const ArchSpec &target_arch,
       m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
       m_image_search_paths(ImageSearchPathsChanged, this),
       m_source_manager_up(), m_stop_hooks(), m_stop_hook_next_id(0),
+      m_latest_stop_hook_id(0),
       m_valid(true), m_suppress_stop_hooks(false),
       m_is_dummy_target(is_dummy_target),
       m_frame_recognizer_manager_up(
@@ -181,6 +182,7 @@ void Target::CleanupProcess() {
   DisableAllWatchpoints(false);
   ClearAllWatchpointHitCounts();
   ClearAllWatchpointHistoricValues();
+  m_latest_stop_hook_id = 0;
 }
 
 void Target::DeleteCurrentProcess() {
@@ -2609,12 +2611,6 @@ bool Target::RunStopHooks() {
   if (m_process_sp->GetState() != eStateStopped)
     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 false;
-
   if (m_stop_hooks.empty())
     return false;
 
@@ -2629,6 +2625,18 @@ bool Target::RunStopHooks() {
   if (!any_active_hooks)
     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.  Note, you can't just check whether the last stop
+  // was for a User Expression, because breakpoint commands get run before
+  // stop hooks, and one of them might have run an expression.  You have
+  // to ensure you run the stop hooks once per natural stop.
+  uint32_t last_natural_stop = m_process_sp->GetModIDRef().GetLastNaturalStopID();
+  if (last_natural_stop != 0 && m_latest_stop_hook_id == last_natural_stop)
+    return false;
+
+  m_latest_stop_hook_id = last_natural_stop;
+
   std::vector<ExecutionContext> exc_ctx_with_reasons;
 
   ThreadList &cur_threadlist = m_process_sp->GetThreadList();

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

@@ -96,12 +96,12 @@ class TestStopHooks(TestBase):
         # Now set the breakpoint on step_out_of_me, and make sure we run the
         # expression, then continue back to main.
         bkpt = target.BreakpointCreateBySourceRegex("Set a breakpoint here and step out", self.main_source_file)
-        self.assertTrue(bkpt.GetNumLocations() > 0, "Got breakpoints in step_out_of_me")
+        self.assertNotEqual(bkpt.GetNumLocations(), 0, "Got breakpoints in step_out_of_me")
         process.Continue()
 
         var = target.FindFirstGlobalVariable("g_var")
         self.assertTrue(var.IsValid())
-        self.assertEqual(var.GetValueAsUnsigned(), 5, "Updated g_var")
+        self.assertEqual(var.GetValueAsUnsigned(), 6, "Updated g_var")
         
         func_name = process.GetSelectedThread().frames[0].GetFunctionName()
         self.assertEqual("main", func_name, "Didn't stop at the expected function.")

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

@@ -29,6 +29,11 @@ class TestStopHooks(TestBase):
         """Test that stop hooks fire on step-out."""
         self.step_out_test()
 
+    def test_stop_hooks_after_expr(self):
+        """Test that a stop hook fires when hitting a breakpoint
+           that runs an expression"""
+        self.after_expr_test()
+
     def step_out_test(self):
         (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
                                    "Set a breakpoint here", self.main_source_file)
@@ -42,3 +47,44 @@ class TestStopHooks(TestBase):
         self.assertTrue(var.IsValid())
         self.assertEqual(var.GetValueAsUnsigned(), 1, "Updated g_var")
 
+    def after_expr_test(self):
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+        interp.HandleCommand("target stop-hook add -o 'expr g_var++'", result)
+        self.assertTrue(result.Succeeded, "Set the target stop hook")
+
+        (target, process, thread, first_bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                   "Set a breakpoint here", self.main_source_file)
+
+        var = target.FindFirstGlobalVariable("g_var")
+        self.assertTrue(var.IsValid())
+        self.assertEqual(var.GetValueAsUnsigned(), 1, "Updated g_var")
+
+        bkpt = target.BreakpointCreateBySourceRegex("Continue to here", self.main_source_file)
+        self.assertNotEqual(bkpt.GetNumLocations(), 0, "Set the second breakpoint")
+        commands = lldb.SBStringList()
+        commands.AppendString("expr increment_gvar()")
+        bkpt.SetCommandLineCommands(commands);
+        
+        threads = lldbutil.continue_to_breakpoint(process, bkpt)
+        self.assertEqual(len(threads), 1, "Hit my breakpoint")
+        
+        self.assertTrue(var.IsValid())
+        self.assertEqual(var.GetValueAsUnsigned(), 3, "Updated g_var")
+
+        # Make sure running an expression does NOT run the stop hook.
+        # Our expression will increment it by one, but the stop shouldn't
+        # have gotten it to 5.
+        threads[0].frames[0].EvaluateExpression("increment_gvar()")
+        self.assertTrue(var.IsValid())
+        self.assertEqual(var.GetValueAsUnsigned(), 4, "Updated g_var")
+        
+
+        # Make sure a rerun doesn't upset the state we've set up:
+        process.Kill()
+        lldbutil.run_to_breakpoint_do_run(self, target, first_bkpt)
+        var = target.FindFirstGlobalVariable("g_var")
+        self.assertTrue(var.IsValid())
+        self.assertEqual(var.GetValueAsUnsigned(), 1, "Updated g_var")
+        
+        

lldb/test/API/commands/target/stop-hooks/main.c

@@ -7,9 +7,15 @@ int step_out_of_me()
   return g_var; // Set a breakpoint here and step out.
 }
 
+void
+increment_gvar() {
+  g_var++;
+}
+
 int
 main()
 {
   int result = step_out_of_me(); // Stop here first
+  increment_gvar(); // Continue to here
   return result;
 }