read watchpoints broken with remote targets?

read watchpoints broken with remote targets?

[Vladimir Prus]

Hi,
are read watchpoints supposed to work with remote targets?
When I try to set read watchpoint with my custom remote server, even "next" 
does not work. Gdb sends "make a single asm step" command to remote server,
server responds with "S.." packet (stopped), but gdb thinks it's read 
watchpoint that fired, turns off single stepping and reports read watchpoint.

Here's exactly what happens:

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.

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?

BTW, HAVE_CONTINUABLE_WATCHPOINT is only set for x86, it seems, so read 
watchpoint might be broken for more targets then just remote.

- Volodya

Re: read watchpoints broken with remote targets?

[Eli Zaretskii]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Fri, 11 Nov 2005 16:21:02 +0300
> 
> are read watchpoints supposed to work with remote targets?

Yes, assuming that the remote stub supports hardware watchpoints.

> 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?

> 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.

> BTW, HAVE_CONTINUABLE_WATCHPOINT is only set for x86, it seems, so read 
> watchpoint might be broken for more targets then just remote.

Most targets don't support read watchpoints (see my other mail in
response to your other message).

Re: read watchpoints broken with remote targets?

[Daniel Jacobowitz]

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

Re: read watchpoints broken with remote targets?

[Johan Rydberg]

Daniel Jacobowitz wrote:

> Actually, I think it is the right solution.  [...]
> So a watchpoint was found not to have triggered, but failed to change
> bs->stop, so it was bogusly reported.

This is exactly the change I have done in my private repo, and it seem
to work just fine for our purposes.


--- breakpoint.c        22 Aug 2005 15:45:46 -0000      1.1
+++ breakpoint.c        20 Oct 2005 16:17:59 -0000      1.2
@@ -2624,7 +2624,7 @@
      if ((b->type == bp_hardware_watchpoint
          || b->type == bp_read_watchpoint
          || b->type == bp_access_watchpoint)
-       && !stopped_by_watchpoint)
+       && stopped_by_watchpoint == 0)
        continue;

      if (b->type == bp_hardware_breakpoint)
@@ -2741,8 +2741,13 @@
         struct value *v;
         int found = 0;

-       if (!target_stopped_data_address (&current_target, &addr))
-         continue;
+       if (!target_stopped_data_address (&current_target, &addr))
+          {
+           /* Don't stop.  */
+           bs->print_it = print_it_noop;
+           bs->stop = 0;
+            continue;
+          }
         for (v = b->val_chain; v; v = v->next)
           {
             if (VALUE_LVAL (v) == lval_memory

Re: read watchpoints broken with remote targets?

[Vladimir Prus]

On Monday 14 November 2005 09:23, Johan Rydberg wrote:
> Daniel Jacobowitz wrote:
> > Actually, I think it is the right solution.  [...]
> > So a watchpoint was found not to have triggered, but failed to change
> > bs->stop, so it was bogusly reported.
>
> This is exactly the change I have done in my private repo, and it seem
> to work just fine for our purposes.

Good to know you've arrived to the same fix.

Any chance this will be applied? Should Johan, or I, send the patch to 
gdb-patches?

- Volodya