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

[Pedro Alves <pedro at codesourcery dot com>]

We got a bug report that read watchpoints weren't working
as expected against an ARM target.

The target was reporting the read watchpoint hits correctly,
but GDB was just always ignoring them like so:

...
&"infrun: TARGET_WAITKIND_STOPPED\n"
&"infrun: stop_pc = 0xce\n"
&"infrun: stopped by watchpoint\n"
&"infrun: stopped data address = 0x20007fdc\n"
&"infrun: no stepping, continue\n"
&"infrun: resume (step=0, signal=0), trap_expected=0\n"
...

The reproducer is just something like:

void main()
{
	int a = 3;
	
	while (1)
	{
		a++;
	}
}

and rwatching `a'.  This compiled as:

~"Dump of assembler code for function main:\n"
~"0x000000c0 <main+0>:\tpush\t{r7}\n"
~"0x000000c2 <main+2>:\tsub\tsp, #12\n"
~"0x000000c4 <main+4>:\tadd\tr7, sp, #0\n"
~"0x000000c6 <main+6>:\tmov.w\tr3, #3\n"
~"0x000000ca <main+10>:\tstr\tr3, [r7, #4]\n"
~"0x000000cc <main+12>:\tldr\tr3, [r7, #4]\n"  <<< reads memory, traps read
~"0x000000ce <main+14>:\tadd.w\tr3, r3, #1\n"
~"0x000000d2 <main+18>:\tstr\tr3, [r7, #4]\n"    <<< writes mem, changes `a', doesn't trap
~"0x000000d4 <main+20>:\tb.n\t0xcc <main+12>\n"


The problem is that bpstat_check_watchpoint logic for read watchpoints
is bogus for targets that do support read watchpoints properly:

 	    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!

This happens to be PR9605
<http://sourceware.org/bugzilla/show_bug.cgi?id=9605>

The compromise here is reversed.  We should believe the target
when it says a read watchpoint trapped.  On targets that can't
do read-only watchpoints, but lie to the core allowing them to
be inserted, read watchpoints will behave as access watchpoints.
As Jeremy says on the PR: "over-enthusiastic reporting is less
of an issue than under-enthusiastic reporting".

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?

-- 
Pedro Alves

2010-02-18  Pedro Alves  <pedro@codesourcery.com>

	PR9605

	gdb/
	* breakpoint.c (bpstat_check_watchpoint): Stop for read
	watchpoints even if the value changed.

---
 gdb/breakpoint.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2010-02-17 22:26:47.000000000 +0000
+++ src/gdb/breakpoint.c	2010-02-18 00:27:22.000000000 +0000
@@ -3432,14 +3432,16 @@ bpstat_check_watchpoint (bpstat bs)
 	      /* Stop.  */
 	      break;
 	    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;
-		}
+	      /* For read watchpoints, even though reads don't cause
+		 value changes, the value may have changed since the
+		 last time it was read, as we're not supposedly
+		 trapping memory writes.  There are targets that can't
+		 break on data reads only, they have to break on
+		 accesses (reads or writes), even though they still
+		 claim support for read watchpoints.  On those, read
+		 watchpoints will behave as access breakpoints, but
+		 that's a better compromise than missing reads on
+		 targets that do support breaking on reads only.  */
 	      break;
 	    case WP_VALUE_NOT_CHANGED:
 	      if (b->type == bp_hardware_watchpoint