Re: read watchpoints don't work on targets that support read watchpoints

[Eli Zaretskii <eliz at gnu dot org>]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Sun, 21 Feb 2010 21:47:44 +0000
> 
> I think we're on the right track, but the logic can
> be simplified further:
> 
>  If we're watchpoint the memory for both reads and writes, then
>  apply the only-if-changed logic.  Otherwise, report the watchpoint
>  unconditionally.  The former can happen either when the user set an
>  access or write watchpoint at the same memory as the read
>  watchpoint, or, when GDB falls back to emulating read watchpoints
>  with access watchpoints.

Agreed.

> WDYT?

I like it, but I have a few comments and questions:

> +      /* If trying to set a read-watchpoint, and it turns out it's not
> +	 supported, try emulating one with an access watchpoint.  */
> +      if (val == 1 && bpt->watchpoint_type == hw_read)
> +	{
> +	  struct bp_location *loc, **loc_temp;
> +
> +	  /* But don't try to insert it, if there's already another
> +	     hw_access location that would be considered a duplicate
> +	     of this one.  */

What does the last comment above mean for the use-case where I set a
read watchpoint and an access watchpoint at the same address (but with
different conditions)?  I probably am missing something, because the
above seems to say this use will be impossible?

> +		  if (bl->watchpoint_type == hw_read)
> +		    {
> +		      CORE_ADDR addr;
> +		      int res;
> +
> +		      /* A real read watchpoint.  Check if we're also
> +			 trapping the same memory for writes.  */
> +
> +		      res = target_stopped_data_address (&current_target,
> +							 &addr);
> +		      /* We can only find a read watchpoint triggering
> +			 if we know the address that trapped.  We
> +			 shouldn't get here otherwise.  */
> +		      gdb_assert (res);
> +
> +		      ALL_BREAKPOINTS (b)
> +			if (breakpoint_enabled (b)
> +			    && (b->type == bp_hardware_watchpoint
> +				|| b->type == bp_access_watchpoint))

Why do you need to scan ALL_BREAKPOINTS HERE?  Can't you scan only the
list returned by watchpoints_triggered?

> +			    /* Exact match not required.  Within range
> +			       is sufficient.  */
> +			    for (loc = b->loc; loc; loc = loc->next)
> +			      if (target_watchpoint_addr_within_range
> +				  (&current_target,
> +				   addr,
> +				   loc->address,
> +				   loc->length))

And why are we checking watchpoints that couldn't have triggered,
because they watch a different, albeit close, address?  What would be
the use-case where such a watchpoint could be relevant to deciding
which watchpoint to announce?