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

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

[Pedro Alves]

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

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

[Eli Zaretskii]

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

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

[Eli Zaretskii]

> Date: Thu, 18 Feb 2010 20:39:35 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.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.

Actually, I think this issue can affect more platforms than just x86,
at least potentially.  It will affect any target that reports only the
address of the watched memory (as opposed to providing an unambiguous
indication, through some handle or index, of which watchpoint breaks).
The relevant use-case is when an address is being watched by several
different watchpoints.  I think by address is currently the only
mechanism by which targets report watchpoints to breakpoint.c, but
maybe my memory fails me.  If I'm right, then every target will be
affected by the proposed change, when multiple watchpoints watch the
same address.

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

On second thought, maybe we can do better than that.  How about the
following strategy?

  . If the bpstat list computed by watchpoints_triggered includes only
    one watchpoint, announce that regardless of the value-changed
    logic.

  . Otherwise, if the list includes write or access watchpoints, and
    the target doesn't support read-only watchpoints, announce the
    read-only watchpoints only if the data did not change.

  . Otherwise, announce every watchpoint in the list unconditionally.

WDYT?

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

[Pedro Alves]

On Friday 19 February 2010 10:17:17, Eli Zaretskii wrote:
> > 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.

My usual concern is with the remote target/protocol, but, I
remembered that a stub can just not support the z3 (read) watchpoint
packet...  That makes things simpler in that we already have
the needed knowlege required without needing new packets
or qSupported features.


> On second thought, maybe we can do better than that.  How about the
> following strategy?

Ah, I like where this is going.  Thanks.

>   . If the bpstat list computed by watchpoints_triggered includes only
>     one watchpoint, announce that regardless of the value-changed
>     logic.
> 
>   . Otherwise, if the list includes write or access watchpoints, and
>     the target doesn't support read-only watchpoints, announce the
>     read-only watchpoints only if the data did not change.
> 
>   . Otherwise, announce every watchpoint in the list unconditionally.
> 
> WDYT?
> 

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.

Here's a patch to implement this, along with a new testcase.  Passes
on x86, that no longer claims it supports read watchpoints, but
now triggers the falling-back-to-access-watchpoints on the core side,
and on a target that supports read watchpoints (*).  The test file includes
a test that sets a write watchpoint watching the same memory as a
read watchpoint to test the ignore-if-changed logic on both
kinds of targets.

 (*) - PPC only supports one watchpoint at a time, but I hacked
       ppc-linux-nat.c to allow inserting a write watchpoint
       in addition to a read watchpoint, iff they both watch the
       same address, by just oring the read|write flags.

WDYT?

-- 
Pedro Alves

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

	PR9605

	gdb/
	* breakpoint.c (insert_bp_location): If inserting the read
	watchpoint failed, fallback to an access watchpoint.
	(bpstat_check_watchpoint): Stop for read watchpoints triggers even
	if the value changed, if not watching the same memory for writes.
	(watchpoint_locations_match): Add comment.
	(update_global_location_list): Copy the location's watchpoint type.
	* i386-nat.c (i386_length_and_rw_bits): It's an internal error to
	handle read watchpoints here.
	(i386_insert_watchpoint): Read watchpoints aren't supported.
	* remote.c (remote_insert_watchpoint): Return 1 for unsupported
	packets.
	* target.h (target_insert_watchpoint): Update description.

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

	PR9605

	gdbserver/
	* i386-low.c (i386_length_and_rw_bits): Throw a fatal error if
	handing a read watchpoint.
	(i386_low_insert_watchpoint): Read watchpoints aren't supported.

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

	PR9605

	gdb/testsuite/
	* gdb.base/watch-read.c, gdb.base/watch-read.exp: New files.

---
 gdb/breakpoint.c         |  143 ++++++++++++++++++++++++++++++++++++++++++++---
 gdb/gdbserver/i386-low.c |    5 +
 gdb/i386-nat.c           |    6 +
 gdb/remote.c             |    5 -
 gdb/target.h             |    7 +-
 5 files changed, 152 insertions(+), 14 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2010-02-21 20:40:52.000000000 +0000
+++ src/gdb/breakpoint.c	2010-02-21 21:16:49.000000000 +0000
@@ -127,6 +127,9 @@ static int breakpoint_address_match (str
 				     struct address_space *aspace2,
 				     CORE_ADDR addr2);
 
+static int watchpoint_locations_match (struct bp_location *loc1,
+				       struct bp_location *loc2);
+
 static void breakpoints_info (char *, int);
 
 static void breakpoint_1 (int, int);
@@ -1485,10 +1488,43 @@ Note: automatically using hardware break
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bpt->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bpt->address, 
+      val = target_insert_watchpoint (bpt->address,
 				      bpt->length,
 				      bpt->watchpoint_type);
-      bpt->inserted = (val != -1);
+
+      /* 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.  */
+	  ALL_BP_LOCATIONS (loc, loc_temp)
+	    if (loc != bpt
+		&& loc->watchpoint_type == hw_access
+		&& watchpoint_locations_match (bpt, loc))
+	      {
+		bpt->duplicate = 1;
+		bpt->inserted = 1;
+		bpt->target_info = loc->target_info;
+		bpt->watchpoint_type = hw_access;
+		val = 0;
+		break;
+	      }
+
+	  if (val == 1)
+	    {
+	      val = target_insert_watchpoint (bpt->address,
+					      bpt->length,
+					      hw_access);
+	      if (val == 0)
+		bpt->watchpoint_type = hw_access;
+	    }
+	}
+
+      bpt->inserted = (val == 0);
     }
 
   else if (bpt->owner->type == bp_catchpoint)
@@ -3434,11 +3470,88 @@ bpstat_check_watchpoint (bpstat bs)
 	    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;
+		  /* There are two cases to consider here:
+
+		     - we're watching the triggered memory for reads.
+		     In that case, trust the target, and always report
+		     the watchpoint hit to the user.  Even though
+		     reads don't cause value changes, the value may
+		     have changed since the last time it was read, and
+		     since we're not trapping writes, we will not see
+		     those, and as such we should ignore our notion of
+		     old value.
+
+		     - we're watching the triggered memory for both
+		     reads and writes.  There are two ways this may
+		     happen:
+
+		      1. this is a target that can't break on data
+		     reads only, but can break on accesses (reads or
+		     writes), such as e.g., x86.  We detect this case
+		     at the time we try to insert read watchpoints.
+
+		      2. Otherwise, the target supports read
+		     watchpoints, but, the user set an access or write
+		     watchpoint watching the same memory as this read
+		     watchpoint.
+
+		    If we're watching memory writes as well as reads,
+		    ignore watchpoint hits when we find that the value
+		    hasn't changed, as reads don't cause changes.
+		    This still gives false positives when the program
+		    writes the same value to memory as what there was
+		    already in memory (we will confuse it for a read),
+		    but it's much better than nothing.  */
+
+		  int other_write_watchpoint = 0;
+
+		  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))
+			  {
+			    struct bp_location *loc;
+
+			    /* 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))
+				{
+				  other_write_watchpoint = 1;
+				  break;
+				}
+			  }
+		    }
+
+		  if (other_write_watchpoint
+		      || bl->watchpoint_type == hw_access)
+		    {
+		      /* We're watching the same memory for writes,
+			 and the value changed since the last time we
+			 updated it, so this trap must be for a write.
+			 Ignore it.  */
+		      bs->print_it = print_it_noop;
+		      bs->stop = 0;
+		    }
 		}
 	      break;
 	    case WP_VALUE_NOT_CHANGED:
@@ -4697,6 +4810,12 @@ breakpoint_address_is_meaningful (struct
 static int
 watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
 {
+  /* Note that this checks the owner's type, not the location's.  In
+     case the target does not support read watchpoints, but does
+     support access watchpoints, we'll have bp_read_watchpoint
+     watchpoints with hw_access locations.  Those should be considered
+     duplicates of hw_read locations.  The hw_read locations will
+     become hw_access locations later.  */
   return (loc1->owner->type == loc2->owner->type
 	  && loc1->pspace->aspace == loc2->pspace->aspace
 	  && loc1->address == loc2->address
@@ -8459,6 +8578,16 @@ update_global_location_list (int should_
 			  /* For the sake of should_be_inserted.
 			     Duplicates check below will fix up this later.  */
 			  loc2->duplicate = 0;
+
+			  /* Read watchpoint locations are switched to
+			     access watchpoints, if the former are not
+			     supported, but the latter are.  */
+			  if (is_hardware_watchpoint (old_loc->owner))
+			    {
+			      gdb_assert (is_hardware_watchpoint (loc2->owner));
+			      loc2->watchpoint_type = old_loc->watchpoint_type;
+			    }
+
 			  if (loc2 != old_loc && should_be_inserted (loc2))
 			    {
 			      loc2->inserted = 1;
Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c	2010-02-21 20:40:52.000000000 +0000
+++ src/gdb/gdbserver/i386-low.c	2010-02-21 20:52:24.000000000 +0000
@@ -219,7 +219,7 @@ i386_length_and_rw_bits (int len, enum t
 	rw = DR_RW_WRITE;
 	break;
       case hw_read:
-	/* The i386 doesn't support data-read watchpoints.  */
+	fatal ("The i386 doesn't support data-read watchpoints.\n");
       case hw_access:
 	rw = DR_RW_READ;
 	break;
@@ -458,6 +458,9 @@ i386_low_insert_watchpoint (struct i386_
   int retval;
   enum target_hw_bp_type type = Z_packet_to_hw_type (type_from_packet);
 
+  if (type == hw_read)
+    return 1; /* unsupported */
+
   if (((len != 1 && len != 2 && len != 4)
        && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
Index: src/gdb/i386-nat.c
===================================================================
--- src.orig/gdb/i386-nat.c	2010-02-21 20:40:52.000000000 +0000
+++ src/gdb/i386-nat.c	2010-02-21 20:52:24.000000000 +0000
@@ -268,7 +268,8 @@ i386_length_and_rw_bits (int len, enum t
 	rw = DR_RW_WRITE;
 	break;
       case hw_read:
-	/* The i386 doesn't support data-read watchpoints.  */
+	internal_error (__FILE__, __LINE__,
+			_("The i386 doesn't support data-read watchpoints.\n"));
       case hw_access:
 	rw = DR_RW_READ;
 	break;
@@ -487,6 +488,9 @@ i386_insert_watchpoint (CORE_ADDR addr, 
 {
   int retval;
 
+  if (type == hw_read)
+    return 1; /* unsupported */
+
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
     retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2010-02-21 20:40:52.000000000 +0000
+++ src/gdb/remote.c	2010-02-21 20:52:25.000000000 +0000
@@ -7341,7 +7341,7 @@ remote_insert_watchpoint (CORE_ADDR addr
   enum Z_packet_type packet = watchpoint_to_Z_packet (type);
 
   if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
-    return -1;
+    return 1;
 
   sprintf (rs->buf, "Z%x,", packet);
   p = strchr (rs->buf, '\0');
@@ -7355,8 +7355,9 @@ remote_insert_watchpoint (CORE_ADDR addr
   switch (packet_ok (rs->buf, &remote_protocol_packets[PACKET_Z0 + packet]))
     {
     case PACKET_ERROR:
-    case PACKET_UNKNOWN:
       return -1;
+    case PACKET_UNKNOWN:
+      return 1;
     case PACKET_OK:
       return 0;
     }
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h	2010-02-21 20:40:52.000000000 +0000
+++ src/gdb/target.h	2010-02-21 20:52:25.000000000 +0000
@@ -1264,9 +1264,10 @@ extern char *normal_pid_to_str (ptid_t p
     (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
 
 
-/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.  TYPE is 0
-   for write, 1 for read, and 2 for read/write accesses.  Returns 0 for
-   success, non-zero for failure.  */
+/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.
+   TYPE is 0 for write, 1 for read, and 2 for read/write accesses.
+   Returns 0 for success, 1 if the watchpoint type is not supported,
+   -1 for failure.  */
 
 #define	target_insert_watchpoint(addr, len, type)	\
      (*current_target.to_insert_watchpoint) (addr, len, type)

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

	PR9605

	gdb/testsuite/
	* gdb.base/watch-read.c, gdb.base/watch-read.exp: New files.

---
 gdb/testsuite/gdb.base/watch-read.c   |   33 ++++++++++++++
 gdb/testsuite/gdb.base/watch-read.exp |   78 ++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)

Index: src/gdb/testsuite/gdb.base/watch-read.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/watch-read.c	2010-02-21 20:43:05.000000000 +0000
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2009, 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+volatile int global;
+
+int
+main (void)
+{
+  int foo = -1;
+
+  while (1)
+    {
+      foo = global; /* read line */
+
+      global = foo + 1; /* write line */
+    }
+
+  return 0;
+}
Index: src/gdb/testsuite/gdb.base/watch-read.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/watch-read.exp	2010-02-21 20:43:05.000000000 +0000
@@ -0,0 +1,78 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile "watch-read"
+set srcfile ${testfile}.c
+
+if { [target_info exists gdb,no_hardware_watchpoints] } {
+    untested ${testfile}.exp
+    return -1
+}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    untested ${testfile}.exp
+    return -1
+}
+
+if { ![runto main] } then {
+    fail "run to main ($teststr)"
+    return
+}
+
+set read_line [gdb_get_line_number "read line" $srcfile]
+
+# Test only read watchpoint
+
+gdb_test "rwatch global" \
+    "Hardware read watchpoint .*: global" \
+    "set hardware read watchpoint on global variable"
+
+gdb_test "continue" \
+    "read watchpoint .*: global.*.*Value = 0.*in main.*$srcfile:$read_line.*" \
+    "read watchpoint triggers on first read"
+
+# Check that writes are ignored
+
+gdb_test "continue" \
+    "read watchpoint .*: global.*.*Value = 1.*in main.*$srcfile:$read_line.*" \
+    "read watchpoint triggers on read after after value changed"
+
+gdb_test "watch global" \
+    "atchpoint .*: global" \
+    "set write watchpoint on global variable"
+
+gdb_test "continue" \
+    "atchpoint .*: global.*Old value = 1.*New value = 2.*" \
+    "write watchpoint triggers"
+
+set exp ""
+set exp "${exp}2.*read watchpoint.*keep y.*global.*breakpoint already hit 2 times.*"
+set exp "${exp}3.*watchpoint.*keep y.*global.*breakpoint already hit 1 time.*"
+gdb_test "info watchpoints" \
+    "$exp" \
+    "only write watchpoint triggers when value changes"
+
+gdb_test "continue" \
+    "read watchpoint .*: global.*Value = 2.*in main.*$srcfile:$read_line.*" \
+    "read watchpoint triggers when value doesn't change, trapping reads and writes"
+
+set exp ""
+set exp "${exp}2.*read watchpoint.*keep y.*global.*breakpoint already hit 3 times.*"
+set exp "${exp}3.*watchpoint.*keep y.*global.*breakpoint already hit 1 time.*"
+gdb_test "info watchpoints" \
+    "$exp" \
+    "only read watchpoint triggers when value doesn't change, trapping reads and writes"

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

[Eli Zaretskii]

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

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

[Pedro Alves]

On Monday 22 February 2010 19:24:16, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > +      /* 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?

Note that this is about locations, not high-level user
breakpoints.  E.g.,  two high-level (user) access watchpoints
watching the same memory, can share the same watchpoint location
on the target, even if the condition is different.

E.g.:

 watch global if global == 3
 watch global if global == 4

A simple low-level watchpoint is inserted on the target.

This code is merely applying the same duplicates detection logic
as the last loop of update_global_location_list, but this
is reached after update_global_location_list has been called,
so since we're changing the location's
type (hw_read -> hw_access) very late (at insertion time),
we rescan duplicates here.  (with target accelarated watchpoint
conditions, the duplicates detection will need to be updated to
only consider duplicates locations that have the same condition
expression, but, that change must be done in all places that
do duplicate detection, not just here.)

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

Hmm, watchpoints_triggered doesn't return a list that would
save us iterating over all breakpoints, it does exactly the same
loop and check as below (in fact, I just copied it from there,
comment and all), and stores the result in
b->watchpoint_triggered (as watch_triggered_yes).  So, I guess
I could indeed iterate over all breakpoints as:

		  if (bl->watchpoint_type == hw_read)
		    {
		      struct breakpoint *other_b;

		       ALL_BREAKPOINTS (other_b)
			if ((other_b->type == bp_hardware_watchpoint
			     || other_b->type == bp_access_watchpoint)
			    && (other_b->watchpoint_triggered
				== watch_triggered_yes))
			  {
			    other_write_watchpoint = 1;
			    break;
			  }
		    }

Should be a bit more efficient, and simpler, yes.  Thanks.  I've done
this change, and checked the new test still passes OK on my hacked
PPC gdb.

> 
> > +			    /* 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?

I'm not sure I understand this question.  Does the confusion arise from
the comment quoted above?  This is the exact same check
watchpoints_triggered does.  ADDR is checked for being in
the [loc->address, loc->address+loc->length) range.  If it is, then
watchpoint B is watching ADDR. 

Does this answer your questions?

-- 
Pedro Alves

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

[Pedro Alves]

On Monday 22 February 2010 20:09:39, Pedro Alves wrote:
> E.g.:
> 
>  watch global if global == 3
>  watch global if global == 4
> 
> A simple low-level watchpoint is inserted on the target.

s/simple/single/

-- 
Pedro Alves

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

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Mon, 22 Feb 2010 20:09:39 +0000
> 
> > > +			    /* 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?
> 
> I'm not sure I understand this question.  Does the confusion arise from
> the comment quoted above?  This is the exact same check
> watchpoints_triggered does.  ADDR is checked for being in
> the [loc->address, loc->address+loc->length) range.  If it is, then
> watchpoint B is watching ADDR. 
> 
> Does this answer your questions?

Yes.  I forgot that target_stopped_data_address can return an address
that is anywhere within the watched region.  Sorry, too many years
have passed.

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

[Pedro Alves]

Thanks, Eli.

Below's the patch as installed.  I also added several comments
to the testcase .exp explaining what is being tested.

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

	PR9605

	gdb/
	* breakpoint.c (insert_bp_location): If inserting the read
	watchpoint failed, fallback to an access watchpoint.
	(bpstat_check_watchpoint): Stop for read watchpoint triggers even
	if the value changed, if not watching the same memory for writes.
	(watchpoint_locations_match): Add comment.
	(update_global_location_list): Copy the location's watchpoint type.
	* i386-nat.c (i386_length_and_rw_bits): It's an internal error to
	handle read watchpoints here.
	(i386_insert_watchpoint): Read watchpoints aren't supported.
	* remote.c (remote_insert_watchpoint): Return 1 for unsupported
	packets.
	* target.h (target_insert_watchpoint): Update description.

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

	PR9605

	gdbserver/
	* i386-low.c (i386_length_and_rw_bits): Throw a fatal error if
	handing a read watchpoint.
	(i386_low_insert_watchpoint): Read watchpoints aren't supported.

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

	PR9605

	gdb/testsuite/
	* gdb.base/watch-read.c, gdb.base/watch-read.exp: New files.

---
 gdb/breakpoint.c         |  122 ++++++++++++++++++++++++++++++++++++++++++++---
 gdb/gdbserver/i386-low.c |    5 +
 gdb/i386-nat.c           |    6 +-
 gdb/remote.c             |    5 +
 gdb/target.h             |    7 +-
 5 files changed, 131 insertions(+), 14 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2010-02-22 23:01:49.000000000 +0000
+++ src/gdb/breakpoint.c	2010-02-22 23:21:37.000000000 +0000
@@ -127,6 +127,9 @@ static int breakpoint_address_match (str
 				     struct address_space *aspace2,
 				     CORE_ADDR addr2);
 
+static int watchpoint_locations_match (struct bp_location *loc1,
+				       struct bp_location *loc2);
+
 static void breakpoints_info (char *, int);
 
 static void breakpoint_1 (int, int);
@@ -1485,10 +1488,43 @@ Note: automatically using hardware break
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bpt->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bpt->address, 
+      val = target_insert_watchpoint (bpt->address,
 				      bpt->length,
 				      bpt->watchpoint_type);
-      bpt->inserted = (val != -1);
+
+      /* 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.  */
+	  ALL_BP_LOCATIONS (loc, loc_temp)
+	    if (loc != bpt
+		&& loc->watchpoint_type == hw_access
+		&& watchpoint_locations_match (bpt, loc))
+	      {
+		bpt->duplicate = 1;
+		bpt->inserted = 1;
+		bpt->target_info = loc->target_info;
+		bpt->watchpoint_type = hw_access;
+		val = 0;
+		break;
+	      }
+
+	  if (val == 1)
+	    {
+	      val = target_insert_watchpoint (bpt->address,
+					      bpt->length,
+					      hw_access);
+	      if (val == 0)
+		bpt->watchpoint_type = hw_access;
+	    }
+	}
+
+      bpt->inserted = (val == 0);
     }
 
   else if (bpt->owner->type == bp_catchpoint)
@@ -3434,11 +3470,67 @@ bpstat_check_watchpoint (bpstat bs)
 	    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;
+		  /* There are two cases to consider here:
+
+		     1. we're watching the triggered memory for reads.
+		     In that case, trust the target, and always report
+		     the watchpoint hit to the user.  Even though
+		     reads don't cause value changes, the value may
+		     have changed since the last time it was read, and
+		     since we're not trapping writes, we will not see
+		     those, and as such we should ignore our notion of
+		     old value.
+
+		     2. we're watching the triggered memory for both
+		     reads and writes.  There are two ways this may
+		     happen:
+
+		     2.1. this is a target that can't break on data
+		     reads only, but can break on accesses (reads or
+		     writes), such as e.g., x86.  We detect this case
+		     at the time we try to insert read watchpoints.
+
+		     2.2. otherwise, the target supports read
+		     watchpoints, but, the user set an access or write
+		     watchpoint watching the same memory as this read
+		     watchpoint.
+
+		     If we're watching memory writes as well as reads,
+		     ignore watchpoint hits when we find that the
+		     value hasn't changed, as reads don't cause
+		     changes.  This still gives false positives when
+		     the program writes the same value to memory as
+		     what there was already in memory (we will confuse
+		     it for a read), but it's much better than
+		     nothing.  */
+
+		  int other_write_watchpoint = 0;
+
+		  if (bl->watchpoint_type == hw_read)
+		    {
+		      struct breakpoint *other_b;
+
+		      ALL_BREAKPOINTS (other_b)
+			if ((other_b->type == bp_hardware_watchpoint
+			     || other_b->type == bp_access_watchpoint)
+			    && (other_b->watchpoint_triggered
+				== watch_triggered_yes))
+			  {
+			    other_write_watchpoint = 1;
+			    break;
+			  }
+		    }
+
+		  if (other_write_watchpoint
+		      || bl->watchpoint_type == hw_access)
+		    {
+		      /* We're watching the same memory for writes,
+			 and the value changed since the last time we
+			 updated it, so this trap must be for a write.
+			 Ignore it.  */
+		      bs->print_it = print_it_noop;
+		      bs->stop = 0;
+		    }
 		}
 	      break;
 	    case WP_VALUE_NOT_CHANGED:
@@ -4697,6 +4789,12 @@ breakpoint_address_is_meaningful (struct
 static int
 watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
 {
+  /* Note that this checks the owner's type, not the location's.  In
+     case the target does not support read watchpoints, but does
+     support access watchpoints, we'll have bp_read_watchpoint
+     watchpoints with hw_access locations.  Those should be considered
+     duplicates of hw_read locations.  The hw_read locations will
+     become hw_access locations later.  */
   return (loc1->owner->type == loc2->owner->type
 	  && loc1->pspace->aspace == loc2->pspace->aspace
 	  && loc1->address == loc2->address
@@ -8459,6 +8557,16 @@ update_global_location_list (int should_
 			  /* For the sake of should_be_inserted.
 			     Duplicates check below will fix up this later.  */
 			  loc2->duplicate = 0;
+
+			  /* Read watchpoint locations are switched to
+			     access watchpoints, if the former are not
+			     supported, but the latter are.  */
+			  if (is_hardware_watchpoint (old_loc->owner))
+			    {
+			      gdb_assert (is_hardware_watchpoint (loc2->owner));
+			      loc2->watchpoint_type = old_loc->watchpoint_type;
+			    }
+
 			  if (loc2 != old_loc && should_be_inserted (loc2))
 			    {
 			      loc2->inserted = 1;
Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c	2010-02-22 23:01:49.000000000 +0000
+++ src/gdb/gdbserver/i386-low.c	2010-02-22 23:21:37.000000000 +0000
@@ -219,7 +219,7 @@ i386_length_and_rw_bits (int len, enum t
 	rw = DR_RW_WRITE;
 	break;
       case hw_read:
-	/* The i386 doesn't support data-read watchpoints.  */
+	fatal ("The i386 doesn't support data-read watchpoints.\n");
       case hw_access:
 	rw = DR_RW_READ;
 	break;
@@ -458,6 +458,9 @@ i386_low_insert_watchpoint (struct i386_
   int retval;
   enum target_hw_bp_type type = Z_packet_to_hw_type (type_from_packet);
 
+  if (type == hw_read)
+    return 1; /* unsupported */
+
   if (((len != 1 && len != 2 && len != 4)
        && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
Index: src/gdb/i386-nat.c
===================================================================
--- src.orig/gdb/i386-nat.c	2010-02-22 23:01:49.000000000 +0000
+++ src/gdb/i386-nat.c	2010-02-22 23:21:37.000000000 +0000
@@ -268,7 +268,8 @@ i386_length_and_rw_bits (int len, enum t
 	rw = DR_RW_WRITE;
 	break;
       case hw_read:
-	/* The i386 doesn't support data-read watchpoints.  */
+	internal_error (__FILE__, __LINE__,
+			_("The i386 doesn't support data-read watchpoints.\n"));
       case hw_access:
 	rw = DR_RW_READ;
 	break;
@@ -487,6 +488,9 @@ i386_insert_watchpoint (CORE_ADDR addr, 
 {
   int retval;
 
+  if (type == hw_read)
+    return 1; /* unsupported */
+
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
     retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2010-02-22 23:01:49.000000000 +0000
+++ src/gdb/remote.c	2010-02-22 23:21:37.000000000 +0000
@@ -7341,7 +7341,7 @@ remote_insert_watchpoint (CORE_ADDR addr
   enum Z_packet_type packet = watchpoint_to_Z_packet (type);
 
   if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
-    return -1;
+    return 1;
 
   sprintf (rs->buf, "Z%x,", packet);
   p = strchr (rs->buf, '\0');
@@ -7355,8 +7355,9 @@ remote_insert_watchpoint (CORE_ADDR addr
   switch (packet_ok (rs->buf, &remote_protocol_packets[PACKET_Z0 + packet]))
     {
     case PACKET_ERROR:
-    case PACKET_UNKNOWN:
       return -1;
+    case PACKET_UNKNOWN:
+      return 1;
     case PACKET_OK:
       return 0;
     }
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h	2010-02-22 23:01:49.000000000 +0000
+++ src/gdb/target.h	2010-02-22 23:21:37.000000000 +0000
@@ -1264,9 +1264,10 @@ extern char *normal_pid_to_str (ptid_t p
     (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
 
 
-/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.  TYPE is 0
-   for write, 1 for read, and 2 for read/write accesses.  Returns 0 for
-   success, non-zero for failure.  */
+/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.
+   TYPE is 0 for write, 1 for read, and 2 for read/write accesses.
+   Returns 0 for success, 1 if the watchpoint type is not supported,
+   -1 for failure.  */
 
 #define	target_insert_watchpoint(addr, len, type)	\
      (*current_target.to_insert_watchpoint) (addr, len, type)


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

	PR9605

	gdb/testsuite/
	* gdb.base/watch-read.c, gdb.base/watch-read.exp: New files.

---
 gdb/testsuite/gdb.base/watch-read.c   |   33 ++++++++++
 gdb/testsuite/gdb.base/watch-read.exp |  109 ++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)

Index: src/gdb/testsuite/gdb.base/watch-read.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/watch-read.c	2010-02-22 23:03:27.000000000 +0000
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2009, 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+volatile int global;
+
+int
+main (void)
+{
+  int foo = -1;
+
+  while (1)
+    {
+      foo = global; /* read line */
+
+      global = foo + 1; /* write line */
+    }
+
+  return 0;
+}
Index: src/gdb/testsuite/gdb.base/watch-read.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/watch-read.exp	2010-02-22 23:21:25.000000000 +0000
@@ -0,0 +1,109 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file was written by Pedro Alves <pedro@codesourcery.com>
+
+# This file is part of the gdb testsuite.
+
+#
+# Tests involving read watchpoints, and other kinds of watchpoints
+# watching the same memory as read watchpoints.
+#
+
+set testfile "watch-read"
+set srcfile ${testfile}.c
+
+if { [target_info exists gdb,no_hardware_watchpoints] } {
+    untested ${testfile}.exp
+    return -1
+}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    untested ${testfile}.exp
+    return -1
+}
+
+if { ![runto main] } then {
+    fail "run to main"
+    return
+}
+
+set read_line [gdb_get_line_number "read line" $srcfile]
+
+# Test running to a read of `global', with a read watchpoint set
+# watching it.
+
+gdb_test "rwatch global" \
+    "Hardware read watchpoint .*: global" \
+    "set hardware read watchpoint on global variable"
+
+# The first read is on entry to the loop.
+
+gdb_test "continue" \
+    "read watchpoint .*: global.*.*Value = 0.*in main.*$srcfile:$read_line.*" \
+    "read watchpoint triggers on first read"
+
+# The second read happens on second loop iteration, after `global'
+# having been incremented.  On architectures where gdb has to emulate
+# read watchpoints with access watchpoints, this tests the
+# only-report-if-value-changed logic.  On targets that support real
+# read watchpoints, this tests that GDB ignores the watchpoint's old
+# value, knowing that some untrapped write could have changed it, and
+# so reports the read watchpoint unconditionally.
+
+gdb_test "continue" \
+    "read watchpoint .*: global.*.*Value = 1.*in main.*$srcfile:$read_line.*" \
+    "read watchpoint triggers on read after value changed"
+
+# The following tests check that when the user sets a write or access
+# watchpoint watching the same memory as a read watchpoint, GDB also
+# applies the only-report-if-value-changed logic even on targets that
+# support real read watchpoints.
+
+# The program should be stopped at the read line.  Set a write
+# watchpoint (leaving the read watchpoint) and continue.  Only the
+# write watchpoint should be reported as triggering.
+
+gdb_test "watch global" \
+    "atchpoint .*: global" \
+    "set write watchpoint on global variable"
+
+gdb_test "continue" \
+    "atchpoint .*: global.*Old value = 1.*New value = 2.*" \
+    "write watchpoint triggers"
+
+set exp ""
+set exp "${exp}2.*read watchpoint.*keep y.*global.*breakpoint already hit 2 times.*"
+set exp "${exp}3.*watchpoint.*keep y.*global.*breakpoint already hit 1 time.*"
+gdb_test "info watchpoints" \
+    "$exp" \
+    "only write watchpoint triggers when value changes"
+
+# The program is now stopped at the write line.  Continuing should
+# stop at the read line, and only the read watchpoint should be
+# reported as triggering.
+
+gdb_test "continue" \
+    "read watchpoint .*: global.*Value = 2.*in main.*$srcfile:$read_line.*" \
+    "read watchpoint triggers when value doesn't change, trapping reads and writes"
+
+set exp ""
+set exp "${exp}2.*read watchpoint.*keep y.*global.*breakpoint already hit 3 times.*"
+set exp "${exp}3.*watchpoint.*keep y.*global.*breakpoint already hit 1 time.*"
+gdb_test "info watchpoints" \
+    "$exp" \
+    "only read watchpoint triggers when value doesn't change"