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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
