Page MenuHomePhabricator

Fix a cornercase in breakpoint reporting
ClosedPublic

Authored by labath on Apr 1 2016, 5:27 AM.

Details

Summary

This resolves a similar problem as D16720 (which handled the case when we single-step onto a
breakpoint), but this one deals with involutary stops: when we stop a thread (e.g. because
another thread has hit a breakpont and we are doing a full stop), we can end up stopping it right
before it executes a breakpoint instruction. In this case, the stop reason will be empty, but we
will still step over the breakpoint when do the next resume, thereby missing a breakpoint hit.

I have observed this happening in TestConcurrentEvents, but I have no idea how to reproduce this
behavior more reliably.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 52351.Apr 1 2016, 5:27 AM
labath retitled this revision from to Fix a cornercase in breakpoint reporting.
labath updated this object.
labath added a reviewer: clayborg.
labath added a subscriber: lldb-commits.
labath updated this revision to Diff 52357.Apr 1 2016, 6:17 AM

I've realized that some foreign stubs may not send the "reason" field, so I limit the scope of this change to cases where "signal" is zero as well.

clayborg accepted this revision.Apr 1 2016, 9:39 AM
clayborg edited edge metadata.

Looks fine. We should probably put the code that checks for a breakpoint at the thread PC into a function since it is done about 5 times in this function.

This revision is now accepted and ready to land.Apr 1 2016, 9:39 AM

The only worry I have about this is

  • thread A is stopped at a breakpoint, and then we stop on thread B instead, then we report a breakpoint, great!
  • The user steps thread B, which we do by suspending thread B and stepping A.
  • Then A stops, and we report ANOTHER hit on thread B.

Your fix looks better than what was there previously. If it is easy to check "last stop reason was this breakpoint hit, and this thread's temporary resume state is suspended, then don't report the hit, that would be more accurate.

labath added a comment.Apr 5 2016, 9:50 AM

Looks fine. We should probably put the code that checks for a breakpoint at the thread PC into a function since it is done about 5 times in this function.

I'll prepare a follow-up to deduplicate the code here.

The only worry I have about this is

  • thread A is stopped at a breakpoint, and then we stop on thread B instead, then we report a breakpoint, great!
  • The user steps thread B, which we do by suspending thread B and stepping A.
  • Then A stops, and we report ANOTHER hit on thread B.

Your fix looks better than what was there previously. If it is easy to check "last stop reason was this breakpoint hit, and this thread's temporary resume state is suspended, then don't report the hit, that would be more accurate.

I tried to do the following:

  • have two threads: A and B
  • hit a breakpoint on thread A
  • switch to thread B and resume it
  • hit a breakpoint on thread B

After this, thread A still reports as being stopped at a breakpoint, but:

  • the breakpoint is not counted as hit twice
  • the code I am changing is not even executed, as we back out of this function early due to thread_sp->StopInfoIsUpToDate() being true

So, while I could easily add the check for eStateSuspended, it seems that this situation is already handled elsewhere (probably by Thread::IsStillAtLastBreakpointHit, although I'm not that familiar with this code). Which makes sense as this situation could happen even for normal breakpoint hits, not just the ones I "simulate" here.

Does that make sense?

jingham accepted this revision.Apr 5 2016, 10:35 AM
jingham added a reviewer: jingham.

Yes, that sounds right.

This revision was automatically updated to reflect the committed changes.