Re: read watchpoints broken with remote targets?

[Daniel Jacobowitz <drow@false.org>]

On Fri, Nov 11, 2005 at 07:39:08PM +0200, Eli Zaretskii wrote:
> > 1. handle_inferior_event (infrun.c) is called.
> > 
> > 2. It contains:
> >    int stopped_by_watchpoint = -1;
> > 
> > 3. The following code is executed:
> > 
> >    if (HAVE_CONTINUABLE_WATCHPOINT)
> >     stopped_by_watchpoint = STOPPED_BY_WATCHPOINT (ecs->ws);
> > 
> >    For remote targets, and for pretty much all other targets,
> >    HAVE_CONTINUABLE_WATCHPOINT is 0, so value of stopped_by_watchpoint
> >    is still -1
> > 
> > 4. Function bpstat_stop_status (breakpoint.c) is called, and
> >    stopped_by_watchpoint is passed to it (the value is still -1).
> > 
> > 5. bpstat_stop_status tries to create a list of stop reasons, by iterating
> >    over all breakpoints and trying to check if that's breakpoint that's
> >    fired. For read wathcpoints we arrive at this:
> > 
> >       if ((b->type == bp_hardware_watchpoint
> > 	   || b->type == bp_read_watchpoint
> >    	   || b->type == bp_access_watchpoint)
> >            && !stopped_by_watchpoint)
> >         continue;
> > 
> >    since stepped_by_watchpoint is -1 we continue with the loop body, and
> >    arrive at this:
> > 
> >       bs = bpstat_alloc (b, bs);	/* Alloc a bpstat to explain stop */
> > 
> >    this adds a new element to the list of stop reasons.
> > 
> > 6. We execute this code:
> > 
> >    	if (!target_stopped_data_address (&current_target, &addr))
> > 	  continue;
> > 
> >     since there were no watchpoint, "continue" is executed. But the stop
> >     reasons list still has a new element corresponding to read
> >     watchpoint. 
> > 
> > 7.  We return to handle_inhefiour_event, which notices the stop reasons list 
> >     and stops stepping.
> 
> Thanks for the detailed report.
> 
> I think this should be fixed by explicitly testing for
> stopped_by_watchpoint being -1, and treating it as if the value were
> zero.  Or perhaps bpstat_stop_status should change its value to zero
> if it's -1.
> 
> Can you see if one of these methods solves the problem?

A little archeology is in order here.

The stopped_by_watchpoint check was added only last year, by Ulrich
Weigand.  At that point it was 0 or 1.

date: 2004/05/13 16:39:10;  author: uweigand;  state: Exp;  lines: +4 -3
        * breakpoint.c (bpstat_stop_status): Add new argument
        STOPPED_BY_WATCHPOINT.  Use it instead of testing
        target_stopped_data_address agaist 0 to check whether
        or not we stopped due to a hardware watchpoint.
        * breakpoint.h (bpstat_stop_status): Adapt prototype.
        * infrun.c (handle_inferior_event): Call bpstat_stop_status
        with new argument.

The initialization to -1 was added later:

2004-06-22  Jeff Johnston  <jjohnstn@redhat.com>

        * infrun.c (handle_inferior_event): Initialize stopped_by_watchpoint
        to -1.
        * breakpoint.c (bpstat_stop_status): Move check for ignoring
        untriggered watchpoints to a separate if clause.  Update function
        comment regarding STOPPED_BY_WATCHPOINT argument.

+    /* Continuable hardware watchpoints are treated as non-existent if the
+       reason we stopped wasn't a hardware watchpoint (we didn't stop on
+       some data address).  Otherwise gdb won't stop on a break instruction
+       in the code (not from a breakpoint) when a hardware watchpoint has
+       been defined.  */

So at the time, Jeff wanted to change this for only continuable
watchpoints.  Which are actually supported for i386, sparc-solaris,
s390, and QNX, not just i386.  He was trying to fix a bug on a platform
without continuable watchpoints, ia64:

  http://sourceware.org/ml/gdb-patches/2004-06/msg00481.html

It looks like this happens because we step over nonsteppable
watchpoints before reporting them, thus preventing us from using
STOPPED_BY_WATCHPOINT to know that we hit a watchpoint.  Which doesn't
really make any sense to me.  Since i386 and sparc-solaris are probably
the oldest supported targets with watchpoints, this may have been the
path of least resistance when !HAVE_CONTINUABLE_WATCHPOINT support was
added.  I don't want to try to rearchitect those bits right now,
though.  So for the moment...

Jeff fixed this problem for non-read watchpoints, but read watchpoints
are still broken.

> > I've fixed this by replacing code in (6) with:
> > 
> > 	if (!target_stopped_data_address (&current_target, &addr))
> >         {
> >           bs->print_it = print_it_noop;
> >    	  bs->stop = 0;
> > 	  continue;
> >         }
> > 
> > Could somebody comment if this is the right fix?
> 
> I don't think this is the right fix.  In effect, you say that if
> stopped_by_watchpoint is non-zero, but target_stopped_data_address
> says there was no watchpoint, let's ignore stopped_by_watchpoint.
> That's not clean, IMHO.

Actually, I think it is the right solution.  This is a read or access
watchpoint; we can't report it if we don't have
target_stopped_data_address.  The code already tried to do that. 
Here's what was there:

    /* Come here if it's a watchpoint, or if the break address matches */

    bs = bpstat_alloc (b, bs);  /* Alloc a bpstat to explain stop */

    /* Watchpoints may change this, if not found to have triggered. */
    bs->stop = 1;
    bs->print = 1;

    if (b->type == bp_watchpoint ||
        b->type == bp_hardware_watchpoint)
      {
         ....

    else if (b->type == bp_read_watchpoint || 
             b->type == bp_access_watchpoint)
      {
        CORE_ADDR addr;
        struct value *v;
        int found = 0;

        if (!target_stopped_data_address (&current_target, &addr))
          continue;

So a watchpoint was found not to have triggered, but failed to change
bs->stop, so it was bogusly reported.

-- 
Daniel Jacobowitz
CodeSourcery, LLC