This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] remote: Fix hw watchpoint address matching
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Pedro Alves <pedro at codesourcery dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Wed, 7 Dec 2011 23:18:21 +0000
- Subject: Re: [PATCH] remote: Fix hw watchpoint address matching
- References: <alpine.DEB.1.10.1111301733090.4191@tp.orcam.me.uk> <201112071724.06407.pedro@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;