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: Thu, 18 Feb 2010 01:11:31 +0000
> 
> The problem is that bpstat_check_watchpoint logic for read watchpoints
> is bogus for targets that do support read watchpoints properly:

I'd say "bogus" is a little exaggerated here, to say the least.  For
x86, it's the best we can do, and works reasonably well even under
contrived conditions (see below for one such example).  And at the
time this was written, the number of targets that did support
read-only watchpoints was very small.

>  	    case WP_VALUE_CHANGED:
> 	      if (b->type == bp_read_watchpoint)
> 		{
> 		  /* Don't stop: read watchpoints shouldn't fire if
> 		     the value has changed.  This is for targets
> 		     which cannot set read-only watchpoints.  */
> 		  bs->print_it = print_it_noop;
> 		  bs->stop = 0;
> 		}
> 
> If we don't get a memory write trap, then when the next read traps,
> we'll see that the watched value changes, and consequently we just
> ignore the watchpoint!

Right, but I could argue that this situation is rarely of practical
importance: why didn't the user use a data-write watchpoint for the
program you've shown?

> As Jeremy says on the PR: "over-enthusiastic reporting is less
> of an issue than under-enthusiastic reporting".

I don't agree with this philosophy, but I don't want to start an
academic argument, either.  What worries me is this: what will happen
on x86 under your suggested change if the same address is watched with
2 different watchpoints: a read one and a write one?  (If you wonder
why would that be useful, then the answer is that each one could use a
different condition.)  Before the changes which added this logic, the
results were a total mess.

> The logic of not reporting read watchpoints if the memory
> changes could be reinstated, if conditionalized on the target
> telling the core that it can't do read watchpoints, but it
> can do access watchpoints instead.  Or the target itself could
> do that (filtering read-write traps from gdb if the memory
> watched doesn't change), which is more efficient, transparent
> and flexible.  Or such targets should just refuse
> to install read watchpoints, and GDB should try to fallback
> to trying access watchpoints, and knowing it should apply
> the "only-if-not-changed" logic then.  This is never perfect,
> because we'll report reads when the program writes the same value
> as the memory already had, but then again, this is what already
> happens today.  The latter option is a bit problematic with
> the current interfaces, as we don't know _why_ is the target
> refusing a read watchpoint.  In any case, I think that
> targets allowing read watchpoints to be inserted, and
> then trapping on writes should get what they deserve.
> 
> I think we should apply this, and work from here on if needed.
> Any objection or better ideas?

I would suggest to fix the problem in a more fundamental manner,
instead of degrading the watchpoint support on one target for the sake
of another.

I like this suggestion the best:

  The logic of not reporting read watchpoints if the memory
  changes could be reinstated, if conditionalized on the target
  telling the core that it can't do read watchpoints, but it
  can do access watchpoints instead.

With this setup, we can properly support both x86 and targets that
support real read-only watchpoints.