Re: [PATCH] remote: Fix hw watchpoint address matching

["Maciej W. Rozycki" <macro@codesourcery.com>]

On Wed, 7 Dec 2011, Pedro Alves wrote:

> > +static int
> > +remote_watchpoint_addr_within_range (struct target_ops *target, CORE_ADDR addr,
> > +				     CORE_ADDR start, int length)
> > +{
> > +  CORE_ADDR diff = remote_address_masked (addr - start);
> > +
> > +  return diff >= 0 && diff < length;
> > +}
> 
> CORE_ADDR is unsigned.  `>= 0' is always true.

 Umm...

> Wouldn't it be much more readable to:
> 
> {
>   CORE_ADDR start = remote_address_masked (start);
> 
>   return start <= addr && addr < start + length;
> }
> 
> ?
> 
> (assuming addr is already masked, since that was the address
> the target reported.)

 This makes me nervous.  I think we should be liberal on what we accept.  
In particular ILP32 ABIs on 64-bit targets may be affected.  An example is 
the MIPS n64 ABI where the width of general registers is 64 bits and 
addresses are sign-extended 32 bits.  When bit #31 is set in the address, 
the remote stub may possibly report the value as truncated to 32 bits or 
as a properly sign-extended 64-bit value.  Not that I observed this 
anywhere, but I think we should accept both.

 Here's an updated version; I have annotated the function now too per 
Joel's suggestion elsewhere even though these are rather scarce throughout 
remote.c.  I can rewrite it to call remote_address_masked separately for 
addr and start if that makes you feel better, although frankly I don't see 
an obvious benefit from doing that and there's a small performance loss.

 Thanks for your review.  Any further comments?

2011-12-07  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* remote.c (remote_watchpoint_addr_within_range): New function.
	(init_remote_ops): Use it.

  Maciej

gdb-remote-watch-range.diff
Index: gdb-fsf-trunk-quilt/gdb/remote.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/remote.c	2011-12-07 21:07:11.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/remote.c	2011-12-07 22:19:03.255601238 +0000
@@ -7808,6 +7808,18 @@ remote_insert_watchpoint (CORE_ADDR addr
 		  _("remote_insert_watchpoint: reached end of function"));
 }
 
+/* Return 1 if ADDR is within the range of a watchpoint spanning LENGTH
+   bytes beginning at START, otherwise 0.  */
+
+static int
+remote_watchpoint_addr_within_range (struct target_ops *target, CORE_ADDR addr,
+				     CORE_ADDR start, int length)
+{
+  CORE_ADDR diff = remote_address_masked (addr - start);
+
+  return diff < length;
+}
+
 
 static int
 remote_remove_watchpoint (CORE_ADDR addr, int len, int type,
@@ -10607,6 +10619,8 @@ Specify the serial device it is connecte
   remote_ops.to_remove_breakpoint = remote_remove_breakpoint;
   remote_ops.to_stopped_by_watchpoint = remote_stopped_by_watchpoint;
   remote_ops.to_stopped_data_address = remote_stopped_data_address;
+  remote_ops.to_watchpoint_addr_within_range =
+    remote_watchpoint_addr_within_range;
   remote_ops.to_can_use_hw_breakpoint = remote_check_watch_resources;
   remote_ops.to_insert_hw_breakpoint = remote_insert_hw_breakpoint;
   remote_ops.to_remove_hw_breakpoint = remote_remove_hw_breakpoint;