read watchpoints don't work on targets that support read watchpoints
Pedro Alves
pedro@codesourcery.com
Mon Feb 22 23:39:00 GMT 2010
Thanks, Eli.
Below's the patch as installed. I also added several comments
to the testcase .exp explaining what is being tested.
--
Pedro Alves
2010-02-22 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 watchpoint 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-22 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-22 Pedro Alves <pedro@codesourcery.com>
PR9605
gdb/testsuite/
* gdb.base/watch-read.c, gdb.base/watch-read.exp: New files.
---
gdb/breakpoint.c | 122 ++++++++++++++++++++++++++++++++++++++++++++---
gdb/gdbserver/i386-low.c | 5 +
gdb/i386-nat.c | 6 +-
gdb/remote.c | 5 +
gdb/target.h | 7 +-
5 files changed, 131 insertions(+), 14 deletions(-)
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c 2010-02-22 23:01:49.000000000 +0000
+++ src/gdb/breakpoint.c 2010-02-22 23:21:37.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,67 @@ 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:
+
+ 1. 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.
+
+ 2. we're watching the triggered memory for both
+ reads and writes. There are two ways this may
+ happen:
+
+ 2.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.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)
+ {
+ struct breakpoint *other_b;
+
+ ALL_BREAKPOINTS (other_b)
+ if ((other_b->type == bp_hardware_watchpoint
+ || other_b->type == bp_access_watchpoint)
+ && (other_b->watchpoint_triggered
+ == watch_triggered_yes))
+ {
+ 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 +4789,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 +8557,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-22 23:01:49.000000000 +0000
+++ src/gdb/gdbserver/i386-low.c 2010-02-22 23:21:37.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-22 23:01:49.000000000 +0000
+++ src/gdb/i386-nat.c 2010-02-22 23:21:37.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-22 23:01:49.000000000 +0000
+++ src/gdb/remote.c 2010-02-22 23:21:37.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-22 23:01:49.000000000 +0000
+++ src/gdb/target.h 2010-02-22 23:21:37.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-22 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 | 109 ++++++++++++++++++++++++++++++++++
2 files changed, 142 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-22 23:03:27.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-22 23:21:25.000000000 +0000
@@ -0,0 +1,109 @@
+# 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/>.
+
+# This file was written by Pedro Alves <pedro@codesourcery.com>
+
+# This file is part of the gdb testsuite.
+
+#
+# Tests involving read watchpoints, and other kinds of watchpoints
+# watching the same memory as read watchpoints.
+#
+
+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"
+ return
+}
+
+set read_line [gdb_get_line_number "read line" $srcfile]
+
+# Test running to a read of `global', with a read watchpoint set
+# watching it.
+
+gdb_test "rwatch global" \
+ "Hardware read watchpoint .*: global" \
+ "set hardware read watchpoint on global variable"
+
+# The first read is on entry to the loop.
+
+gdb_test "continue" \
+ "read watchpoint .*: global.*.*Value = 0.*in main.*$srcfile:$read_line.*" \
+ "read watchpoint triggers on first read"
+
+# The second read happens on second loop iteration, after `global'
+# having been incremented. On architectures where gdb has to emulate
+# read watchpoints with access watchpoints, this tests the
+# only-report-if-value-changed logic. On targets that support real
+# read watchpoints, this tests that GDB ignores the watchpoint's old
+# value, knowing that some untrapped write could have changed it, and
+# so reports the read watchpoint unconditionally.
+
+gdb_test "continue" \
+ "read watchpoint .*: global.*.*Value = 1.*in main.*$srcfile:$read_line.*" \
+ "read watchpoint triggers on read after value changed"
+
+# The following tests check that when the user sets a write or access
+# watchpoint watching the same memory as a read watchpoint, GDB also
+# applies the only-report-if-value-changed logic even on targets that
+# support real read watchpoints.
+
+# The program should be stopped at the read line. Set a write
+# watchpoint (leaving the read watchpoint) and continue. Only the
+# write watchpoint should be reported as triggering.
+
+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"
+
+# The program is now stopped at the write line. Continuing should
+# stop at the read line, and only the read watchpoint should be
+# reported as triggering.
+
+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"
More information about the Gdb-patches
mailing list