[PATCH] D140368: [lldb] Consider all breakpoints in breakpoint detection

[Jim Ingham via Phabricator via llvm-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

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 says 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()) {

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



================
Comment at: lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:31
+        # Setup three breakpoints
+        self.lines = [line_number('main.cpp', "breakpoint_%i" % i) for i in range(3)]
+
----------------
It's easier to use BreakpointCreateBySourceRegex for this, since you can do the search & set in one step.


================
Comment at: lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:34
+        self.breakpoints = [self.target.BreakpointCreateByLocation(src, line) for line in self.lines]
+        self.assertTrue(
+            self.breakpoints[0] and self.breakpoints[0].GetNumLocations() == 1,
----------------
Why do you only check one of the breakpoints?


================
Comment at: lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:39
+        # Start debugging
+        self.process = self.target.LaunchSimple(
+            None, None, self.get_process_working_directory())
----------------
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.


================
Comment at: lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py:75
+        # Make sure we install the breakpoint at the right address:
+        # on some architectures (e.g, aarch64), step-out stops before the next source line
+        return_addr = self.thread.GetFrameAtIndex(1).GetPC()
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140368/new/

https://reviews.llvm.org/D140368