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

[mohit.bhakkad]

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.

[clayborg]

I will defer to Jim Ingham on this one.

[mohit.bhakkad]

In D13296#260030, @clayborg wrote:

I will defer to Jim Ingham on this one.

@jingham could you please review this.

[jingham]

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.

[mohit.bhakkad]

In D13296#265608, @jingham wrote:

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]

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.

[mohit.bhakkad]

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.

[jingham]

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

[mohit.bhakkad]

Is it okay to commit now?

[jingham]

Yes.

lldb/trunk/source/Breakpoint/Watchpoint.cpp

 bool
 Watchpoint::ShouldStop (StoppointCallbackContext *context)
 {
     IncrementHitCount();
 
     if (!IsEnabled())
         return false;
 
-    if (GetHitCount() <= GetIgnoreCount())
-        return false;
-
     return true;
 }
 
 void
 Watchpoint::GetDescription (Stream *s, lldb::DescriptionLevel level)
 {
     DumpWithLevel(s, level);
 }

lldb/trunk/source/Target/StopInfo.cpp

PerformAction(Event *event_ptr) override
                     WatchpointSP wp_hit_sp = thread_sp->CalculateTarget()->GetWatchpointList().FindByAddress(m_watch_hit_addr);
                     if (!wp_hit_sp)
                     {
                         m_should_stop = false;
                         wp_sp->IncrementFalseAlarmsAndReviseHitCount();
                     }
                 }
 
+                // 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.
+                if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount())
+                    m_should_stop = false;
+
                 if (m_should_stop && wp_sp->GetConditionText() != NULL)
                 {
                     // We need to make sure the user sees any parse errors in their condition, so we'll hook the
                     // constructor errors up to the debugger's Async I/O.
                     ExpressionResults result_code;
                     EvaluateExpressionOptions expr_options;
                     expr_options.SetUnwindOnError(true);
                     expr_options.SetIgnoreBreakpoints(true);