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

[jingham]

Summary

Code was added to Target::RunStopHook to make sure that we don't run stop hooks when you stop for 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.

[JDevlieghere]

Small nit, but otherwise this LGTM.

lldb/include/lldb/Target/Target.h

1432–1433

Please make these /// Doxygen comments above the variable.

1435

Same here + period.

lldb/include/lldb/Target/Target.h

 #define LLDB_TARGET_TARGET_H
 
 #include <list>
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "lldb/Breakpoint/BreakpointList.h"
 #include "lldb/Breakpoint/BreakpointName.h"
 #include "lldb/Breakpoint/WatchpointList.h"
 #include "lldb/Core/Architecture.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/UserSettingsController.h"
 #include "lldb/Expression/Expression.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
protected:
   typedef std::map<lldb::LanguageType, lldb::REPLSP> REPLMap;
   REPLMap m_repl_map;
 
   lldb::SourceManagerUP m_source_manager_up;
 
   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 stop running 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
   /// information of this target.
   lldb::TraceSP m_trace_sp;
   /// Stores the frame recognizers of this target.
   lldb::StackFrameRecognizerManagerUP m_frame_recognizer_manager_up;
 

lldb/source/Target/Target.cpp

 //===-- Target.cpp --------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/Target.h"
 #include "lldb/Breakpoint/BreakpointIDList.h"
 #include "lldb/Breakpoint/BreakpointPrecondition.h"
 #include "lldb/Breakpoint/BreakpointResolver.h"
 #include "lldb/Breakpoint/BreakpointResolverAddress.h"
 #include "lldb/Breakpoint/BreakpointResolverFileLine.h"
 #include "lldb/Breakpoint/BreakpointResolverFileRegex.h"
 #include "lldb/Breakpoint/BreakpointResolverName.h"
 #include "lldb/Breakpoint/BreakpointResolverScripted.h"
: TargetProperties(this),
       Broadcaster(debugger.GetBroadcasterManager(),
                   Target::GetStaticBroadcasterClass().AsCString()),
       ExecutionContextScope(), m_debugger(debugger), m_platform_sp(platform_sp),
       m_mutex(), m_arch(target_arch), m_images(this), m_section_load_history(),
       m_breakpoint_list(false), m_internal_breakpoint_list(true),
       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(
           std::make_unique<StackFrameRecognizerManager>()),
       m_stats_storage(static_cast<int>(StatisticKind::StatisticMax))
 
 {
   SetEventName(eBroadcastBitBreakpointChanged, "breakpoint-changed");
void Target::CleanupProcess() {
   m_breakpoint_list.ClearAllBreakpointSites();
   m_internal_breakpoint_list.ClearAllBreakpointSites();
   // Disable watchpoints just on the debugger side.
   std::unique_lock<std::recursive_mutex> lock;
   this->GetWatchpointList().GetListMutex(lock);
   DisableAllWatchpoints(false);
   ClearAllWatchpointHitCounts();
   ClearAllWatchpointHistoricValues();
+  m_latest_stop_hook_id = 0;
 }
 
 void Target::DeleteCurrentProcess() {
   if (m_process_sp) {
     m_section_load_history.Clear();
     if (m_process_sp->IsAlive())
       m_process_sp->Destroy(false);
 
bool Target::RunStopHooks() {
   if (!m_process_sp)
     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 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;
 
   // 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 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();
   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);

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

def setUp(self):
         self.main_source_file = lldb.SBFileSpec("main.c")
         full_path = os.path.join(self.getSourceDir(), "main.c")
         self.main_start_line = line_number(full_path, "main()")
         
     def test_stop_hooks_step_out(self):
         """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)
 
         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")
         thread.StepOut()
         var = target.FindFirstGlobalVariable("g_var")
         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

 #include <stdio.h>
 
 static int g_var = 0;
 
 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;
 }