[lldb] Add deleted line in NextRangeBreakpointExplainsStop

[augusto2112]

Commit f838fa8 refactored large pointers of the thread plan code. In that refactor, the call to ClearNextBranchBreakpoint in
ThreadPlanStepRange::NextRangeBreakpointExplainsStop was deleted, I believe by mistake.

This patch simply re-adds that call.

rdar://159675204

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Augusto Noronha (augusto2112)

Changes

Commit f838fa8 refactored large pointers of the thread plan code. In that refactor, the call to ClearNextBranchBreakpoint in
ThreadPlanStepRange::NextRangeBreakpointExplainsStop was deleted, I believe by mistake.

This patch simply re-adds that call.

rdar://159675204

---

Full diff: https://github.com/llvm/llvm-project/pull/161597.diff

1 Files Affected:

• (modified) lldb/source/Target/ThreadPlanStepRange.cpp (+1)

diff --git a/lldb/source/Target/ThreadPlanStepRange.cpp b/lldb/source/Target/ThreadPlanStepRange.cpp index dca96cc74ba46..ee574b7a9864e 100644 --- a/lldb/source/Target/ThreadPlanStepRange.cpp +++ b/lldb/source/Target/ThreadPlanStepRange.cpp @@ -487,6 +487,7 @@ bool ThreadPlanStepRange::NextRangeBreakpointExplainsStop( "next range breakpoint which has %" PRIu64 " constituents - explains stop: %u.", (uint64_t)num_constituents, explains_stop); + ClearNextBranchBreakpoint(); return explains_stop; } 

[augusto2112]

@jeffreytan81, do you remember if the deletion of this line (https://github.com/llvm/llvm-project/pull/90930/files#diff-46aa8e984a6ebc1d4032efa38c8fba130ab4a8851fba18842e36c37daef32e50L421) was accidental or if there was a reason behind it? I ran the test suite locally and no tests fail if I re-introduce it.

[augusto2112]

By the way, the comment on this line strongly implies that the breakpoint should be cleared:

 // If we've hit the next branch breakpoint, then clear it. 

[jimingham]

It makes sense to clear the "next branch" breakpoint there. Either the thread plan's next branch breakpoint was the only one we hit, in which case that part of the step succeeded and we should clear that breakpoint, or there was a user breakpoint there and we're going to give control back to the user, in which case, again, clearing the breakpoint is a good idea.

I think not doing this would be pretty hard to notice. We will delete the breakpoint when the thread plan is destroyed, so it won't outlive the stepping sequence. Did you actually have a case where this fails, or was this just code inspection?

Anyway, it's worth waiting a bit for a reply by the patch author, but this LGTM.

[jeffreytan81]

@augusto2112, sorry, our company does not use email as primary work communication channel so this PR got missed in overwhelming number of other auto-notification emails.

I am pretty sure the deletion of ClearNextBranchBreakpoint is not accidental. The new design is making NextRangeBreakpointExplainsStop() a side-effect free checking -- only check if the stop_info_sp is an internal "next range breakpoint" stop or not without removing it. And this internal "next range breakpoint" clean up has been moved into ClearNextBranchBreakpointExplainedStop() during each ThreadPlan's ShouldStop(). The comment can be left-over that should be removed.

I remember this change is important because in original PR's ThreadPlanSingleThreadTimeout workflow NextRangeBreakpointExplainsStop can be called multiple times or some ordering, which can cause later checking to fail? There is a test for ThreadPlanSingleThreadTimeout but it's unfortunate it did not fail with ClearNextBranchBreakpoint() adding back. I have no idea why though.

Do you have a scenario that requires ClearNextBranchBreakpoint otherwise it is failing?

[augusto2112]

@jeffreytan81 thanks. It's for a downstream thread plan (which I plan to upstream). I added a call to ClearNextBranchBreakpointExplainedStop() from my thread plan's ShouldStop() as you suggested and that works.