[lldb] Consider all breakpoints in breakpoint detection

Currently in some cases lldb reports stop reason as "step out" or "step over" (from thread plan completion) instead of "breakpoint", if the user breakpoint happens to be set on the same address.
The part of https://github.com/llvm/llvm-project/commit/f08f5c99262ff9eaa08956334accbb2614b0f7a2 seems to overwrite internal breakpoint detection logic, so that only the last breakpoint for the current stop address is considered.
Together with step-out plans not clearing its breakpoint until they are destrouyed, this creates a situation when there is a user breakpoint set for address, but internal breakpoint makes lldb report a plan completion stop reason instead of breakpoint.
This patch reverts that internal breakpoint detection logic to consider all breakpoints

Reviewed By: jingham

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

diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index 225234c..2c61216 100644 (file)
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -256,7 +256,7 @@ protected:
     if (!m_should_perform_action)
       return;
     m_should_perform_action = false;
-    bool internal_breakpoint = true;
+    bool all_stopping_locs_internal = true;
 
     ThreadSP thread_sp(m_thread_wp.lock());
 
@@ -421,8 +421,6 @@ protected:
               continue;
             }
 
-            internal_breakpoint = bp_loc_sp->GetBreakpoint().IsInternal();
-
             // First run the precondition, but since the precondition is per
             // breakpoint, only run it once per breakpoint.
             std::pair<std::unordered_set<break_id_t>::iterator, bool> result =
@@ -509,7 +507,7 @@ protected:
                         loc_desc.GetData());
               // We want this stop reported, so you will know we auto-continued
               // but only for external breakpoints:
-              if (!internal_breakpoint)
+              if (!bp_loc_sp->GetBreakpoint().IsInternal())
                 thread_sp->SetShouldReportStop(eVoteYes);
               auto_continue_says_stop = false;
             }
@@ -539,6 +537,9 @@ protected:
                 actually_said_continue = true;
             }
 
+            if (m_should_stop && !bp_loc_sp->GetBreakpoint().IsInternal())
+              all_stopping_locs_internal = false;
+
             // If we are going to stop for this breakpoint, then remove the
             // breakpoint.
             if (callback_says_stop && bp_loc_sp &&
@@ -576,7 +577,7 @@ protected:
                   __FUNCTION__, m_value);
       }
 
-      if ((!m_should_stop || internal_breakpoint) &&
+      if ((!m_should_stop || all_stopping_locs_internal) &&
           thread_sp->CompletedPlanOverridesBreakpoint()) {
 
         // Override should_stop decision when we have completed step plan

diff --git a/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/Makefile b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/Makefile
new file mode 100644 (file)
index 0000000..2c00681
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/Makefile
@@ -0,0 +1,8 @@
+CXX_SOURCES := main.cpp
+
+ifneq (,$(findstring icc,$(CC)))
+    CXXFLAGS_EXTRAS := -debug inline-debug-info
+endif
+
+
+include Makefile.rules

diff --git a/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
new file mode 100644 (file)
index 0000000..aaecdff
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
@@ -0,0 +1,121 @@
+"""
+Test that breakpoints (reason = breakpoint) have more priority than
+plan completion (reason = step in/out/over) when reporting stop reason after step,
+in particular 'step out' and 'step over', and in addition 'step in'.
+Check for correct StopReason when stepping to the line with breakpoint,
+which should be eStopReasonBreakpoint in general,
+and eStopReasonPlanComplete when breakpoint's condition fails or it is disabled.
+"""
+
+
+import unittest2
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class ThreadPlanUserBreakpointsTestCase(TestBase):
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+        # Build and run to starting breakpoint
+        self.build()
+        src = lldb.SBFileSpec('main.cpp')
+        (self.target, self.process, self.thread, _) = \
+            lldbutil.run_to_source_breakpoint(self, '// Start from here', src)
+
+        # Setup two more breakpoints
+        self.breakpoints = [self.target.BreakpointCreateBySourceRegex('breakpoint_%i' % i, src)
+            for i in range(2)]
+        self.assertTrue(
+            all(bp and bp.GetNumLocations() == 1 for bp in self.breakpoints),
+            VALID_BREAKPOINT)
+
+    def check_correct_stop_reason(self, breakpoint_idx, condition):
+        self.assertState(self.process.GetState(), lldb.eStateStopped)
+        if condition:
+            # All breakpoints active, stop reason is breakpoint
+            thread1 = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, self.breakpoints[breakpoint_idx])
+            self.assertEquals(self.thread, thread1, "Didn't stop at breakpoint %i." % breakpoint_idx)
+        else:
+            # Breakpoints are inactive, stop reason is plan complete
+            self.assertEquals(self.thread.GetStopReason(), lldb.eStopReasonPlanComplete,
+                'Expected stop reason to be step into/over/out for inactive breakpoint %i line.' % breakpoint_idx)
+
+    def change_breakpoints(self, action):
+        for bp in self.breakpoints:
+            action(bp)
+
+    def check_thread_plan_user_breakpoint(self, condition, set_up_breakpoint_func):
+        # Make breakpoints active/inactive in different ways
+        self.change_breakpoints(lambda bp: set_up_breakpoint_func(condition, bp))
+
+        self.thread.StepInto()
+        # We should be stopped at the breakpoint_0 line with the correct stop reason
+        self.check_correct_stop_reason(0, condition)
+
+        # This step-over creates a step-out from `func_1` plan
+        self.thread.StepOver()
+        # We should be stopped at the breakpoint_1 line with the correct stop reason
+        self.check_correct_stop_reason(1, condition)
+
+        # Check explicit step-out
+        # Make sure we install the breakpoint at the right address:
+        # step-out might stop on different lines, if the compiler
+        # did or did not emit more instructions after the return
+        return_addr = self.thread.GetFrameAtIndex(1).GetPC()
+        step_out_breakpoint = self.target.BreakpointCreateByAddress(return_addr)
+        self.assertTrue(step_out_breakpoint, VALID_BREAKPOINT)
+        set_up_breakpoint_func(condition, step_out_breakpoint)
+        self.breakpoints.append(step_out_breakpoint)
+        self.thread.StepOut()
+        # We should be stopped somewhere in the main frame with the correct stop reason
+        self.check_correct_stop_reason(2, condition)
+
+        # Run the process until termination
+        self.process.Continue()
+        self.assertState(self.process.GetState(), lldb.eStateExited)
+
+    def set_up_breakpoints_condition(self, condition, bp):
+        # Set breakpoint condition to true/false
+        conditionStr = 'true' if condition else 'false'
+        bp.SetCondition(conditionStr)
+
+    def set_up_breakpoints_enable(self, condition, bp):
+        # Enable/disable breakpoint
+        bp.SetEnabled(condition)
+
+    def set_up_breakpoints_callback(self, condition, bp):
+        # Set breakpoint callback to return True/False
+        bp.SetScriptCallbackBody('return %s' % condition)
+
+    def test_thread_plan_user_breakpoint_conditional_active(self):
+        # Test with breakpoints having true condition
+        self.check_thread_plan_user_breakpoint(condition=True,
+                                               set_up_breakpoint_func=self.set_up_breakpoints_condition)
+
+    def test_thread_plan_user_breakpoint_conditional_inactive(self):
+        # Test with breakpoints having false condition
+        self.check_thread_plan_user_breakpoint(condition=False,
+                                               set_up_breakpoint_func=self.set_up_breakpoints_condition)
+
+    def test_thread_plan_user_breakpoint_unconditional_active(self):
+        # Test with breakpoints enabled unconditionally
+        self.check_thread_plan_user_breakpoint(condition=True,
+                                               set_up_breakpoint_func=self.set_up_breakpoints_enable)
+
+    def test_thread_plan_user_breakpoint_unconditional_inactive(self):
+        # Test with breakpoints disabled unconditionally
+        self.check_thread_plan_user_breakpoint(condition=False,
+                                               set_up_breakpoint_func=self.set_up_breakpoints_enable)
+
+    def test_thread_plan_user_breakpoint_callback_active(self):
+        # Test with breakpoints with callback that returns 'True'
+        self.check_thread_plan_user_breakpoint(condition=True,
+                                               set_up_breakpoint_func=self.set_up_breakpoints_callback)
+
+    def test_thread_plan_user_breakpoint_callback_inactive(self):
+        # Test with breakpoints with callback that returns 'False'
+        self.check_thread_plan_user_breakpoint(condition=False,
+                                               set_up_breakpoint_func=self.set_up_breakpoints_callback)

diff --git a/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/main.cpp b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/main.cpp
new file mode 100644 (file)
index 0000000..2d7e06e
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/main.cpp
@@ -0,0 +1,11 @@
+int func_1() { return 1; }
+
+int func_2() {
+  func_1();             // breakpoint_0
+  return 1 + func_1();  // breakpoint_1
+}
+
+int main(int argc, char const *argv[]) {
+  func_2();  // Start from here
+  return 0;
+}