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