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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: jingham.
DavidSpickett added a subscriber: jingham.
DavidSpickett added a comment.

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?



================
Comment at: lldb/source/Target/StopInfo.cpp:540
 
+            if (m_should_stop && !bp_loc_sp->GetBreakpoint().IsInternal())
+              internal_breakpoint = false;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140368