[lldb] Consider all breakpoints in breakpoint detection

[kpdev42]

Summary

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

[DavidSpickett]

The intent makes sense. We should stop and report user breakpoints triggered while trying to execute some internal stepping plan, even if they overlap what lldb was planning to do in the first place.

Not totally sure how the change achieves that, this is quite the function. + @jingham who wrote the original changes.

Currently in some cases lldb reports stop reason as "step out" or "step over" (from thread plan completion) over "breakpoint"

This would be clearer if you said "(from thread plan completion) instead of "breakpoint"". Took me a while to work out that it wasn't over meaning step over a breakpoint.

I think the test naming could be clearer. breakpoint/step_out_breakpoint/TestStepOutBreakpoint.py implies it's just about stepping out. How about breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py ? Something that is clear we're testing the interaction of automatic internal stepping plans and breakpoints the user puts in.

Is it worth checking that an unconditional user breakpoint is also reported?

lldb/source/Target/StopInfo.cpp

2

I think the key here is the m_should_stop check (the rest looks equivalent to what is there already). What exactly does that achieve?

[kpdev42]

Renamed the test, added more tests for unconditional (enabled/disabled) breakpoints and breakpoints with callbacks

[kpdev42]

In D140368#4007234, @DavidSpickett wrote:

The intent makes sense. We should stop and report user breakpoints triggered while trying to execute some internal stepping plan, even if they overlap what lldb was planning to do in the first place.

Not totally sure how the change achieves that, this is quite the function. + @jingham who wrote the original changes.

Currently in some cases lldb reports stop reason as "step out" or "step over" (from thread plan completion) over "breakpoint"

This would be clearer if you said "(from thread plan completion) instead of "breakpoint"". Took me a while to work out that it wasn't over meaning step over a breakpoint.

I think the test naming could be clearer. breakpoint/step_out_breakpoint/TestStepOutBreakpoint.py implies it's just about stepping out. How about breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py ? Something that is clear we're testing the interaction of automatic internal stepping plans and breakpoints the user puts in.

Is it worth checking that an unconditional user breakpoint is also reported?

Fixed, please take a look

[kpdev42]

Fix test

[clayborg]

Jim Ingham really should chime in on this if possible as the thread plans and stop info are his area of expertise.

[kpdev42]

@jingham
May I kindly ask you to take a look at this patch?

[jingham]

I think this patch is correct, but could be clearer - mostly because the original code didn't choose good enough names.

What's going on here is that the "internal_breakpoint" variable is getting used in two ways. In the loop over breakpoint locations we're using it to cache the result of IsInternal() for the current breakpoint location. But then outside the loop we really want it to mean "were all the breakpoint locations that said we should stop internal breakpoints?", which gets used in:

if ((!m_should_stop || internal_breakpoint) &&
    thread_sp->CompletedPlanOverridesBreakpoint()) {

The two are equivalent when there aren't other active & matching breakpoints at the same site, but not otherwise, so you're right we need to fix that.

Your patch eliminates the first use and makes internal_breakpoint just follow the outer meaning. That's fine, but then internal_breakpoint ends up being a pretty confusing name - given we've just iterated over a bunch of breakpoints. Instead, it should have a name that indicates it is a value computed from ALL the breakpoint locations we looked at, something like all_stopping_locs_internal. That starts as true, and if you see any non-internal breakpoint say we should stop, then turning this to false will make total sense. And the outer check will also make more sense, it will be:

if ((!m_should_stop || all_stopping_locs_internal) &&
    thread_sp->CompletedPlanOverridesBreakpoint()) {

BTW, I'm not insisting on this particular name, if you can think of a clearer one, that's fine too. But internal_breakpoint is confusing.

I also had a couple of clean-up suggestions for the test. These should be pretty simple cleanups.

lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py

32

It's easier to use BreakpointCreateBySourceRegex for this, since you can do the search & set in one step.

35

Why do you only check one of the breakpoints?

40

You are making three breakpoints, but really the one in main is the "stop to start the test breakpoint" and the other two are the ones for the test. So this would all be more compact if you did:

(target, process, thread, _) = lldbutil.run_to_source_breakpoint(self, "breakpoint_0", lldb.SBFileSpec("main.cpp")

That will do all the jobs of making the target, setting the initial breakpoint, starting a process, and making sure that you hit the initial breakpoint. Then you can just make the other two breakpoints and continue on as you have done here.

76

Step out ALWAYS stops directly on returning to the caller frame. You need that so that if you have:

foo(bar(), baz());

and are stopped in bar below this code, then step-out followed by step-in will land you in baz. If step-out finished the source line, you would be past the call to baz before you got control back.

The only architecture dependency here is whether the compiler needed to emit more code for a given source line after the return from the callee.

[kpdev42]

Renaming and cleanup according to review

[jingham]

LGTM, thanks for figuring this out!

lldb/source/Target/StopInfo.cpp

2

The point is that once we've looked at all the locations that were in the site we stopped at, if all the breakpoints that said we should stop here were internal, then we must be implementing a thread plan and we should give the thread plan a chance to set the stop info. But if there were any non-internal breakpoints that say we should stop, then we should report the breakpoint stop reason instead.

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>
 
bool ShouldStop(Event *event_ptr) override {
     assert(m_should_stop_is_valid);
     return m_should_stop;
   }
 
   void PerformAction(Event *event_ptr) override {
     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());
 
     if (thread_sp) {
       Log *log = GetLog(LLDBLog::Breakpoints | LLDBLog::Step);
 
       if (!thread_sp->IsValid()) {
         // This shouldn't ever happen, but just in case, don't do more harm.
if (thread_sp) {
                           "Breakpoint %s hit on thread 0x%llx but it was not "
                           "for this thread, continuing.",
                           loc_desc.GetData(),
                           static_cast<unsigned long long>(thread_sp->GetID()));
               }
               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 =
                 precondition_breakpoints.insert(
                     bp_loc_sp->GetBreakpoint().GetID());
             if (!result.second)
               continue;
 
if (thread_sp) {
             bool auto_continue_says_stop = true;
             if (bp_loc_sp->IsAutoContinue())
             {
               LLDB_LOGF(log,
                         "Continuing breakpoint %s as AutoContinue was set.",
                         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;
             }
 
             bool callback_says_stop = true;
 
             // FIXME: For now the callbacks have to run in async mode - the
             // first time we restart we need
if (thread_sp) {
               debugger.SetAsyncExecution(old_async);
 
               if (callback_says_stop && auto_continue_says_stop)
                 m_should_stop = true;
               else
                 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 &&
                 bp_loc_sp->GetBreakpoint().IsOneShot()) {
               thread_sp->GetProcess()->GetTarget().RemoveBreakpointByID(
                   bp_loc_sp->GetBreakpoint().GetID());
             }
             // Also make sure that the callback hasn't continued the target. If
if (thread_sp) {
         Log *log_process(GetLog(LLDBLog::Process));
 
         LLDB_LOGF(log_process,
                   "Process::%s could not find breakpoint site id: %" PRId64
                   "...",
                   __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
         // additionally to the breakpoint
         m_should_stop = true;
 
         // We know we're stopping for a completed plan and we don't want to
         // show the breakpoint stop, so compute the public stop info immediately

lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/Makefile

+CXX_SOURCES := main.cpp
+
+ifneq (,$(findstring icc,$(CC)))
+    CXXFLAGS_EXTRAS := -debug inline-debug-info
+endif
+
+
+include Makefile.rules

lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py

+"""
+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)

lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/main.cpp

+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;
+}