Make ignore counts work as "after stop" modifiers so they play nicely with conditions

[jingham]

Summary

Previously ignore counts were checked when we stopped to do the sync callback in Breakpoint::ShouldStop. That meant we would do all the ignore count work even when there is also a condition says the breakpoint should not stop.

That's wrong, lldb treats breakpoint hits that fail the thread or condition checks as "not having hit the breakpoint". So the ignore count check should happen after the condition and thread checks in StopInfoBreakpoint::PerformAction.

The one side-effect of doing this is that if you have a breakpoint with a synchronous callback, it will run the synchronous callback before checking the ignore count. That is probably a good thing, since this was already true of the condition and thread checks, so this removes an odd asymmetry. And breakpoints with sync callbacks are all internal lldb breakpoints and there's not a really good reason why you would want one of these to use an ignore count (but not a condition or thread check...)

[JDevlieghere]

LGTM

lldb/include/lldb/Breakpoint/BreakpointLocation.h

300–305

lldb/include/lldb/Breakpoint/Breakpoint.h

Breakpoint(Target &target, lldb::SearchFilterSP &filter_sp,
              lldb::BreakpointResolverSP &resolver_sp, bool hardware,
              bool resolve_indirect_symbols = true);
 
   friend class BreakpointLocation; // To call the following two when determining
                                    // whether to stop.
 
   void DecrementIgnoreCount();
 
-  // BreakpointLocation::IgnoreCountShouldStop &
-  // Breakpoint::IgnoreCountShouldStop can only be called once per stop, and
-  // BreakpointLocation::IgnoreCountShouldStop should be tested first, and if
-  // it returns false we should continue, otherwise we should test
-  // Breakpoint::IgnoreCountShouldStop.
-
-  bool IgnoreCountShouldStop();
-
 private:
   // To call from CopyFromBreakpoint.
   Breakpoint(Target &new_target, const Breakpoint &bp_to_copy_from);
 
   // For Breakpoint only
   bool m_being_created;
   bool
       m_hardware; // If this breakpoint is required to use a hardware breakpoint

lldb/include/lldb/Breakpoint/BreakpointLocation.h

 //===-- BreakpointLocation.h ------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_BREAKPOINT_BREAKPOINTLOCATION_H
 #define LLDB_BREAKPOINT_BREAKPOINTLOCATION_H
 
 #include <memory>
 #include <mutex>
 
 #include "lldb/Breakpoint/BreakpointOptions.h"
 #include "lldb/Breakpoint/StoppointHitCounter.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 /// \class BreakpointLocation BreakpointLocation.h
protected:
   ///
   /// \return
   ///     \b true if we were successful at setting the breakpoint site,
   ///     \b false otherwise.
   bool SetBreakpointSite(lldb::BreakpointSiteSP &bp_site_sp);
 
   void DecrementIgnoreCount();
 
+  /// BreakpointLocation::IgnoreCountShouldStop  can only be called once
+  /// per stop.  This method checks first against the loc and then the owner.
+  /// It also takes care of decrementing the ignore counters.
+  /// If it returns false we should continue, otherwise stop.
+
   bool IgnoreCountShouldStop();
 
 private:
   void SwapLocation(lldb::BreakpointLocationSP swap_from);
 
   void BumpHitCount();
 
   void UndoBumpHitCount();
 

lldb/source/Breakpoint/Breakpoint.cpp

void Breakpoint::DecrementIgnoreCount() {
   if (ignore != 0)
     m_options_up->SetIgnoreCount(ignore - 1);
 }
 
 uint32_t Breakpoint::GetIgnoreCount() const {
   return m_options_up->GetIgnoreCount();
 }
 
-bool Breakpoint::IgnoreCountShouldStop() {
-  uint32_t ignore = GetIgnoreCount();
-  if (ignore != 0) {
-    // When we get here we know the location that caused the stop doesn't have
-    // an ignore count, since by contract we call it first...  So we don't have
-    // to find & decrement it, we only have to decrement our own ignore count.
-    DecrementIgnoreCount();
-    return false;
-  } else
-    return true;
-}
-
 uint32_t Breakpoint::GetHitCount() const { return m_hit_counter.GetValue(); }
 
 bool Breakpoint::IsOneShot() const { return m_options_up->IsOneShot(); }
 
 void Breakpoint::SetOneShot(bool one_shot) {
   m_options_up->SetOneShot(one_shot);
 }
 

lldb/source/Breakpoint/BreakpointLocation.cpp

 //===-- BreakpointLocation.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/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/BreakpointID.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/UserExpression.h"
void BreakpointLocation::DecrementIgnoreCount() {
   if (m_options_up != nullptr) {
     uint32_t loc_ignore = m_options_up->GetIgnoreCount();
     if (loc_ignore != 0)
       m_options_up->SetIgnoreCount(loc_ignore - 1);
   }
 }
 
 bool BreakpointLocation::IgnoreCountShouldStop() {
-  if (m_options_up != nullptr) {
-    uint32_t loc_ignore = m_options_up->GetIgnoreCount();
-    if (loc_ignore != 0) {
-      m_owner.DecrementIgnoreCount();
-      DecrementIgnoreCount(); // Have to decrement our owners' ignore count,
-                              // since it won't get a
-                              // chance to.
-      return false;
-    }
-  }
+  uint32_t owner_ignore = GetBreakpoint().GetIgnoreCount();
+  uint32_t loc_ignore = 0;
+  if (m_options_up != nullptr)
+    loc_ignore = m_options_up->GetIgnoreCount();
+
+  if (loc_ignore != 0 || owner_ignore != 0) {
+                            // since it won't get a chance to.
   return true;
 }
 
 BreakpointOptions *BreakpointLocation::GetLocationOptions() {
   // If we make the copy we don't copy the callbacks because that is
   // potentially expensive and we don't want to do that for the simple case
   // where someone is just disabling the location.
   if (m_options_up == nullptr)
bool BreakpointLocation::ShouldStop(StoppointCallbackContext *context) {
   bool should_stop = true;
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
 
   // Do this first, if a location is disabled, it shouldn't increment its hit
   // count.
   if (!IsEnabled())
     return false;
 
-  if (!IgnoreCountShouldStop())
-    return false;
-
-  if (!m_owner.IgnoreCountShouldStop())
-    return false;
-
   // We only run synchronous callbacks in ShouldStop:
   context->is_synchronous = true;
   should_stop = InvokeCallback(context);
 
   if (log) {
     StreamString s;
     GetDescription(&s, lldb::eDescriptionLevelVerbose);
     LLDB_LOGF(log, "Hit breakpoint location: %s, %s.\n", s.GetData(),

lldb/source/Target/StopInfo.cpp

 //===-- StopInfo.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 <string>
 
 #include "lldb/Breakpoint/Breakpoint.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Breakpoint/Watchpoint.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StopInfo.h"
if (thread_sp) {
                   // the condition fails. We've already bumped it by the time
                   // we get here, so undo the bump:
                   bp_loc_sp->UndoBumpHitCount();
                   actually_said_continue = true;
                   continue;
                 }
               }
             }
             
+            // We've done all the checks whose failure means "we consider lldb
+            // not to have hit the breakpoint".  Now we're going to check for
+            // conditions that might continue after hitting.  Start with the
+            // ignore count:
+            if (!bp_loc_sp->IgnoreCountShouldStop()) {
+              actually_said_continue = true;
+              continue;
+            }
+
+
+
             // Check the auto-continue bit on the location, do this before the
             // callback since it may change this, but that would be for the
             // NEXT hit.  Note, you might think you could check auto-continue
             // before the condition, and not evaluate the condition if it says
             // to continue.  But failing the condition means the breakpoint was
             // effectively NOT HIT.  So these two states are different.
             bool auto_continue_says_stop = true;
             if (bp_loc_sp->IsAutoContinue())

lldb/test/API/functionalities/breakpoint/breakpoint_ignore_count/TestBreakpointIgnoreCount.py

class BreakpointIgnoreCountTestCase(TestBase):
 
     @add_test_categories(['pyapi'])
     @skipIfWindows # This test will hang on windows llvm.org/pr21753
     def test_with_python_api(self):
         """Use Python APIs to set breakpoint ignore count."""
         self.build()
         self.breakpoint_ignore_count_python()
 
+    @skipIfWindows # This test will hang on windows llvm.org/pr21753
+    def test_ignore_vrs_condition_bkpt(self):
+        self.build()
+        self.ignore_vrs_condition(False)
+        
+    @skipIfWindows # This test will hang on windows llvm.org/pr21753
+    def test_ignore_vrs_condition_loc(self):
+        self.build()
+        self.ignore_vrs_condition(True)
+        
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to of function 'c'.
+        self.stop_in_main = "Stop here at start of main"
         self.line1 = line_number(
             'main.c', '// Find the line number of function "c" here.')
         self.line2 = line_number(
             'main.c', '// b(2) -> c(2) Find the call site of b(2).')
         self.line3 = line_number(
             'main.c', '// a(3) -> c(3) Find the call site of c(3).')
         self.line4 = line_number(
             'main.c', '// a(3) -> c(3) Find the call site of a(3).')
def breakpoint_ignore_count(self):
         # in function name 'c'.  And frame #2 should point to main.c:45.
         self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT_IGNORE_COUNT,
                     #substrs = ["stop reason = breakpoint"],
                     patterns=["frame #0.*main.c:%d" % self.line1,
                               "frame #1.*main.c:%d" % self.line5])
 
     def breakpoint_ignore_count_python(self):
         """Use Python APIs to set breakpoint ignore count."""
-        target = self.createTestTarget()
-
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self,
+                                                                          self.stop_in_main,
+                                                                          lldb.SBFileSpec("main.c"))
         # Now create a breakpoint on main.c by name 'c'.
         breakpoint = target.BreakpointCreateByName('c', 'a.out')
         self.assertTrue(breakpoint and
                         breakpoint.GetNumLocations() == 1,
                         VALID_BREAKPOINT)
 
         # Get the breakpoint location from breakpoint after we verified that,
         # indeed, it has one location.
         location = breakpoint.GetLocationAtIndex(0)
         self.assertTrue(location and
                         location.IsEnabled(),
                         VALID_BREAKPOINT_LOCATION)
 
         # Set the ignore count on the breakpoint location.
         location.SetIgnoreCount(2)
         self.assertEqual(location.GetIgnoreCount(), 2,
                         "SetIgnoreCount() works correctly")
 
-        # Now launch the process, and do not stop at entry point.
-        process = target.LaunchSimple(
-            None, None, self.get_process_working_directory())
-        self.assertTrue(process, PROCESS_IS_VALID)
+        # Now continue and hit our breakpoint on c:
+        process.Continue()
 
         # Frame#0 should be on main.c:37, frame#1 should be on main.c:25, and
         # frame#2 should be on main.c:48.
         # lldbutil.print_stacktraces(process)
         from lldbsuite.test.lldbutil import get_stopped_thread
         thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
         self.assertTrue(
             thread.IsValid(),
             "There should be a thread stopped due to breakpoint")
         frame0 = thread.GetFrameAtIndex(0)
         frame1 = thread.GetFrameAtIndex(1)
         frame2 = thread.GetFrameAtIndex(2)
         self.assertTrue(frame0.GetLineEntry().GetLine() == self.line1 and
                         frame1.GetLineEntry().GetLine() == self.line3 and
                         frame2.GetLineEntry().GetLine() == self.line4,
                         STOPPED_DUE_TO_BREAKPOINT_IGNORE_COUNT)
 
         # The hit count for the breakpoint should be 3.
         self.assertEqual(breakpoint.GetHitCount(), 3)
 
-        process.Continue()
+    def ignore_vrs_condition(self, use_location):
+        main_spec = lldb.SBFileSpec("main.c")
+        target, process, _ , _ = lldbutil.run_to_source_breakpoint(self,
+                                                                   self.stop_in_main,
+                                                                   main_spec)
+        
+        # Now make a breakpoint on the loop, and set a condition and ignore count.
+        # Make sure that the condition fails don't count against the ignore count.
+        bkpt = target.BreakpointCreateBySourceRegex("Set a breakpoint here, with i", main_spec)
+        self.assertEqual(bkpt.GetNumLocations(), 1, "Wrong number of locations")
+
+        if use_location:
+            loc = bkpt.location[0]
+            self.assertTrue(loc.IsValid(), "Got a valid location")
+            loc.SetIgnoreCount(2)
+            loc.SetCondition("i >= 3")
+        else:
+            bkpt.SetIgnoreCount(2)
+            bkpt.SetCondition("i >= 3")
+
+        threads = lldbutil.continue_to_breakpoint(process, bkpt)
+        self.assertEqual(len(threads), 1, "Hit the breakpoint")
+        var = threads[0].frame[0].FindVariable("i")
+        self.assertTrue(var.IsValid(), "Didn't find the i variable")
+        val = var.GetValueAsUnsigned(10000)
+        self.assertNotEqual(val, 10000, "Got the fail value for i")
+        self.assertEqual(val, 5, "We didn't stop the right number of times")
+        self.assertEqual(bkpt.GetHitCount(), 3, "Hit count is not right")

lldb/test/API/functionalities/breakpoint/breakpoint_ignore_count/main.c

int b(int val)
     return c(val);
 }
 
 int c(int val)
 {
     return val + 3; // Find the line number of function "c" here.
 }
 
+void spin_a_bit () {
+  for (unsigned int i = 0; i < 10; i++) {
+    printf("Set a breakpoint here, with i = %d.\n", i);
+  }
+}
+
 int main (int argc, char const *argv[])
 {
-    int A1 = a(1);  // a(1) -> b(1) -> c(1)
+    int A1 = a(1);  // a(1) -> b(1) -> c(1) // Stop here at start of main
     printf("a(1) returns %d\n", A1);
     
     int B2 = b(2);  // b(2) -> c(2) Find the call site of b(2).
     printf("b(2) returns %d\n", B2);
     
     int A3 = a(3);  // a(3) -> c(3) Find the call site of a(3).
     printf("a(3) returns %d\n", A3);
     
     int C1 = c(5); // Find the call site of c in main.
     printf ("c(5) returns %d\n", C1);
+
+    spin_a_bit();
     return 0;
 }