This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: read watchpoints don't work on targets that support read watchpoints
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 (¤t_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
+ (¤t_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"