[PATCH] remote: Fix hw watchpoint address matching

[PATCH] remote: Fix hw watchpoint address matching

[Maciej W. Rozycki]

Hi,

 There is a problem in the remote target where watchpoint addresses are
masked before they are passed on to the target, but later on if such a
watchpoint is hit the generic range-checker compares the masked address
reported by the remote side with the original unmasked address associated
with the watchpoint within GDB.  The result for 32-bit MIPS processors and
KSEG addresses, which are sign-extended to 64 bits and thus non-zero in
the 32 high order bits, is the watchpoint hit is not recognised and a
SIGTRAP reported instead.

 The fix below replaces the generic checker with a specific one that
applies the mask for the purpose of the range check.  This is imperfect if
"set remoteaddresssize" was used to set the mask beyond the width of the
address used by the target, but I think it's about as reasonable as you
can get.  And the setting looks like an awful hack to me, so my suggestion
is merely: "Don't do it."

 Regression-tested with mips-sde-elf, fixing numerous failures like:

(gdb) PASS: gdb.base/display.exp: display/s &sum
cont
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
0x80001176 in do_loops () at .../gdb/testsuite/gdb.base/display.c:22
22                      sum++; f++; force_mem (&k);
5: x/s &sum  0x80004cc0 <sum>:   ""
4: /f f = 3.1415
3: x/i &k
   0x80505b80:  nop
2: /x j = 0x0
1: i = 0
(gdb) FAIL: gdb.base/display.exp: first disp

which is now:

(gdb) PASS: gdb.base/display.exp: display/s &sum
cont
Continuing.
Hardware watchpoint 3: sum

Old value = 0
New value = 1
0x80001176 in do_loops () at .../gdb/testsuite/gdb.base/display.c:22
22                      sum++; f++; force_mem (&k);
5: x/s &sum  0x80004cc0 <sum>:   ""
4: /f f = 3.1415
3: x/i &k
   0x80505b80:  nop
2: /x j = 0x0
1: i = 0
(gdb) PASS: gdb.base/display.exp: first disp

and introducing no new ones.  Tested for mips-linux-gnu successfully too; 
no improvement there as the issue with kernel-mode addresses does not 
apply to Linux userland.  OK to apply?

2011-11-30  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-11-30 16:59:46.215590824 +0000
+++ gdb-fsf-trunk-quilt/gdb/remote.c	2011-11-30 17:01:56.045448615 +0000
@@ -7805,6 +7805,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 >= 0 && diff < length;
+}
+
 
 static int
 remote_remove_watchpoint (CORE_ADDR addr, int len, int type,
@@ -10509,6 +10518,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;

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

[Pedro Alves]

On Wednesday 30 November 2011 18:23:35, Maciej W. Rozycki 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.

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

-- 
Pedro Alves

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

[Maciej W. Rozycki]

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;

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

[Pedro Alves]

On Wednesday 07 December 2011 23:18:21, Maciej W. Rozycki wrote:
> 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.

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.

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

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

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.

-- 
Pedro Alves

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

[Maciej W. Rozycki]

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;

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

[Pedro Alves]

On 02/24/2012 11:48 PM, Maciej W. Rozycki wrote:

> Hi Pedro,
> 
>  Back to this change, was distracted by something else.


So was I.  :-)

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


I fully understand that.  What I was saying is that if that is a concern,
then breakpoints with bit #31 set are quite likely to suffer from exactly
the same issue, with the remote possibly reporting the trapped address as
truncated to 32 bits or as a properly sign-extended 64-bit value.  And I'd
imagine the code that handles the low level traps/exceptions to handle
both watchpoint and breakpoint addresses the same, and the stub code that
marshals those breakpoint addresses to rsp to be just as susceptible to
the issue as code that marshals watchpoint addresses.  Now, I'm not
saying we should run to fix that...

I'm still curious why isn't bkpt_breakpoint_hit broken like I described
above, when a breakpoint is subject to adjustment, such as
that done by mips_breakpoint_from_pc.  I probably need to stare more
at the code.  ;-)

>> /* Implementation of target method FOO.  */
> 
>  Umm, there aren't that many comments of this kind there actually...


We started enforcing that rule not too long ago.

Anyway, thanks for the fixes, and congratulations on your new role.  :-)

-- 
Pedro Alves

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

[Maciej W. Rozycki]

On Tue, 6 Mar 2012, Pedro Alves wrote:

> >  Back to this change, was distracted by something else.
> 
> 
> So was I.  :-)

 And more so, I would imagine -- I'll be missing you and your expertise as 
a colleague, although I am happy we can still meet and work together from 
time to time here.

> >  Yes, this is about watchpoints, not breakpoints. ;)
> 
> 
> I fully understand that.  What I was saying is that if that is a concern,
> then breakpoints with bit #31 set are quite likely to suffer from exactly
> the same issue, with the remote possibly reporting the trapped address as
> truncated to 32 bits or as a properly sign-extended 64-bit value.  And I'd
> imagine the code that handles the low level traps/exceptions to handle
> both watchpoint and breakpoint addresses the same, and the stub code that
> marshals those breakpoint addresses to rsp to be just as susceptible to
> the issue as code that marshals watchpoint addresses.  Now, I'm not
> saying we should run to fix that...
> 
> I'm still curious why isn't bkpt_breakpoint_hit broken like I described
> above, when a breakpoint is subject to adjustment, such as
> that done by mips_breakpoint_from_pc.  I probably need to stare more
> at the code.  ;-)

 My understanding of how breakpoints work is the remote stub does nothing 
specific to report to GDB that it actually has hit a breakpoint.  All it 
does is it reports a(n unspecified) debug trap, no different to one 
reported for single-stepping for example.  Then GDB examines the value of 
the PC reported in the stop reply packet, or, in the absence of one, it 
retrieves the value of that register via an ordinary access, to see if it 
corresponds to a breakpoint address it knows.

 All of this goes through the regular register cache and any necessary 
sign- or zero-extension, as appropriate, is made for each architecture in 
a generic way by the backend involved.  As I don't remember offhand I'd 
have to wade through code to track where exactly it actually happens, but 
I am fairly sure it's there.  If that didn't work, then I would expect GDB 
to be completely unusable for the target affected and therefore easily 
noticed -- unlike this watchpoint corner case.

> >  Umm, there aren't that many comments of this kind there actually...
> 
> 
> We started enforcing that rule not too long ago.

 Good choice, though you can certainly understand the rule isn't obvious 
from code itself yet. ;)

> Anyway, thanks for the fixes, and congratulations on your new role.  :-)

 Thanks.

  Maciej