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

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

Hi Pedro,

 Back to this change, was distracted by something else.

On Fri, 9 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.
> 
> If such thing were possible, then wouldn't breakpoints break?
> We store the (masked) address of where we ended up putting
> the breakpoint in bp_tgt->placed_address (remote_insert_breakpoint),
> and if the target reported an address not exactly bp_tgt->placed_address,
> we wouldn't be able to match it up, resulting in spurious SIGTRAPs.
> Hmm, actually, it looks like breakpoint.c:bkpt_breakpoint_hit is broken
> in that it should be using bl->target_info.placed_address instead
> of bl->address ?  How is this not breaking on cases that need
> breakpoint adjustment?  I'm probably missing something.

 Yes, this is about watchpoints, not breakpoints. ;)

 The address matched against comes from stop_reply->watch_data_address 
(see process_stop_reply).  This doesn't appear to be masked anywhere in 
watchpoints_triggered before target_watchpoint_addr_within_range is called 
and remote_insert_watchpoint doesn't propagate the ultimate masked address 
passed down the remote channel back to loc->address either.

 Therefore my understanding is both arguments to 
remote_watchpoint_addr_within_range have to be treated as unmasked -- addr 
because it may have been sign-extended by the remote stub, and start (i.e. 
loc->address) because it has never been masked in the first place.

> >  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.  
> 
> Actually, for implementations of defined interfaces, such as the
> target vector or gdbarch callbacks, we prefer to leave the explanation
> of the interface to where the interface is defined, and, write something
> like 
> 
> /* Implementation of target method FOO.  */

 Umm, there aren't that many comments of this kind there actually...

> This prevents comment bit rot whenever the main comment in the
> interface declaration changes, but implementations' comments
> are forgotten.

 Good point.

> I see that target_watchpoint_addr_within_range is unfortunately
> undocumented in target.h.  Fortunately, you've already written
> the necessary comment.  :-)  Could you place it there instead
> please?  Okay with that change.  Thanks.

 Thanks for your review.  I have applied the final changes below then, as 
separate commits, as after the comment adjustment they are not really 
functionally bound to each other.

  Maciej

2012-02-24  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* target.h (target_watchpoint_addr_within_range): Document macro.

gdb-target-watch-range-doc.diff
Index: gdb-fsf-trunk-quilt/gdb/target.h
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/target.h	2012-02-24 15:23:42.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/target.h	2012-02-24 23:30:01.565618432 +0000
@@ -1483,6 +1483,8 @@ extern int target_ranged_break_num_regis
 #define target_stopped_data_address(target, addr_p) \
     (*target.to_stopped_data_address) (target, addr_p)
 
+/* Return non-zero if ADDR is within the range of a watchpoint spanning
+   LENGTH bytes beginning at START.  */
 #define target_watchpoint_addr_within_range(target, addr, start, length) \
   (*target.to_watchpoint_addr_within_range) (target, addr, start, length)
 

2012-02-24  Maciej W. Rozycki  <macro@codesourcery.com>

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

gdb-remote-watch-range.diff
Index: gdb-fsf-trunk-quilt/gdb/remote.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/remote.c	2012-02-24 15:41:43.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/remote.c	2012-02-24 23:29:05.445646325 +0000
@@ -7844,6 +7844,15 @@ remote_insert_watchpoint (CORE_ADDR addr
 		  _("remote_insert_watchpoint: reached end of function"));
 }
 
+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,
@@ -10704,6 +10713,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;