Page MenuHomePhabricator

[LLDB] Fix watchpoint ignore feature for architectures with watchpoint_exceptions_received=before
ClosedPublic

Authored by mohit.bhakkad on Sep 30 2015, 9:32 AM.

Details

Summary

For archs like MIPS, where watchpoints are triggered before associated instruction runs,
we need to disable watchpoint, single step and re-enable watchpoint, so that process
resumes without reporting same watchpoint. This is provided by code at Target/StopInfo.cpp:719

But if we check for ignore_count condition before this functionality, it reports same watchpoint
again and again and gets ignored before ignore condition gets false.

So shifting this condition to appropriate location.
This solves TestWatchpointIgnoreCount.py testcase for MIPS.

Diff Detail

Repository
rL LLVM

Event Timeline

mohit.bhakkad retitled this revision from to [LLDB] Fix watchpoint ignore feature for architectures with watchpoint_exceptions_received=before.
mohit.bhakkad updated this object.
mohit.bhakkad added reviewers: clayborg, jingham.
mohit.bhakkad set the repository for this revision to rL LLVM.
mohit.bhakkad added subscribers: jaydeep, bhushan, slthakur and 2 others.
clayborg resigned from this revision.Oct 5 2015, 12:56 PM
clayborg removed a reviewer: clayborg.

I will defer to Jim Ingham on this one.

I will defer to Jim Ingham on this one.

@jingham could you please review this.

jingham requested changes to this revision.Oct 12 2015, 5:14 PM
jingham edited edge metadata.

This change alters the timing for the handling of ignore counts for watchpoints. The original implementation (and the way ignore counts work for breakpoints) is that the breakpoint's ignore count gets checked during the "synchronous" phase of breakpoint stop evaluation. So it happens when the private stop event is received, and if the ignore count is not satisfied then a public event is not generated.

Your version will move the decision making to the async ShouldStop, i.e. when an event is getting pulled from the public event queue. If these were breakpoints it would mean that the synchronous callbacks would still fire even if they fail their ignore count test. Watchpoints don't currently have synchronous callbacks - and I'm not sure we would ever need to support that. If you moved the adjustment code you do in PerformAction to the synchronous callback (Watchpoint::ShouldStop) that would keep breakpoints & watchpoints behaving symmetrically. It's worth trying that, I am pretty sure that will be a safe place to do your little dance. But if it isn't, then I don't mind having it in PerformAction provided you put a note to that effect somewhere obvious in Watchpoint.h.

Note, if you end up going with the current patch, isn't quite right, however. You need to move the check for the watchpoint ignore count up before printing out the old & new values. An ignored watchpoint shouldn't print anything, it should be as if it never stopped. Where you have the check, the old & new values will get printed even if the watchpoint is ignored.

This revision now requires changes to proceed.Oct 12 2015, 5:14 PM

This change alters the timing for the handling of ignore counts for watchpoints. The original implementation (and the way ignore counts work for breakpoints) is that the breakpoint's ignore count gets checked during the "synchronous" phase of breakpoint stop evaluation. So it happens when the private stop event is received, and if the ignore count is not satisfied then a public event is not generated.

Your version will move the decision making to the async ShouldStop, i.e. when an event is getting pulled from the public event queue. If these were breakpoints it would mean that the synchronous callbacks would still fire even if they fail their ignore count test. Watchpoints don't currently have synchronous callbacks - and I'm not sure we would ever need to support that. If you moved the adjustment code you do in PerformAction to the synchronous callback (Watchpoint::ShouldStop) that would keep breakpoints & watchpoints behaving symmetrically. It's worth trying that, I am pretty sure that will be a safe place to do your little dance. But if it isn't, then I don't mind having it in PerformAction provided you put a note to that effect somewhere obvious in Watchpoint.h.

Hi @jingham, I tried to move adjustment code to ShouldStopSynchronous before we call ShouldStop, but its not getting stepped to next PC. Looks like threadplan are not active in that phase of code. Could you suggest anything wrong I may be doing or should I resubmit this patch by putting condition code before we print anything.

Note, if you end up going with the current patch, isn't quite right, however. You need to move the check for the watchpoint ignore count up before printing out the old & new values. An ignored watchpoint shouldn't print anything, it should be as if it never stopped. Where you have the check, the old & new values will get printed even if the watchpoint is ignored.

mohit.bhakkad requested a review of this revision.Oct 30 2015, 4:20 AM
mohit.bhakkad edited edge metadata.

Note, if you end up going with the current patch, isn't quite right, however. You need to move the check for the watchpoint ignore count up before printing out the old & new values. An ignored watchpoint shouldn't print anything, it should be as if it never stopped. Where you have the check, the old & new values will get printed even if the watchpoint is ignored.

Hi @jingham, I re-checked it today, we are not printing anything when we ignore a watchpoint.

I forgot about note part, is it okay to add a TODO before ignore count condition in source/Target/StopInfo.cpp:

TODO: This condition should be checked in synchronous part of watchpoint code (Watchpoint::ShouldStop), so that
we avoid pulling an event even if watchpoint fails ignore count condition. It is moved here temporarily, because for
archs with watchpoint_exceptions_received=before, there is an adjustment code in above lines, which takes control
of inferior to next PC. We have to check ignore count condition after this is done, otherwise we will get same watchpoint
multiple times untill we pass ignore condition, and we won't be ignoring them actually.

The substance is fine. I fixed up the grammar a little, maybe something like:

TODO: This condition should be checked in the synchronous part of the watchpoint code (Watchpoint::ShouldStop), so that
we avoid pulling an event even if the watchpoint fails the ignore count condition. It is moved here temporarily, because for
archs with watchpoint_exceptions_received=before, the code in the previous lines takes care of moving the inferior to next PC.
We have to check the ignore count condition after this is done, otherwise we will hit same watchpoint
multiple times until we pass ignore condition, but we won't actually be ignoring them.

Jim

Is it okay to commit now?

jingham accepted this revision.Nov 2 2015, 11:55 AM
jingham edited edge metadata.

Yes.

This revision is now accepted and ready to land.Nov 2 2015, 11:55 AM
This revision was automatically updated to reflect the committed changes.