From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6416 invoked by alias); 22 Feb 2010 23:39:17 -0000 Received: (qmail 6367 invoked by uid 22791); 22 Feb 2010 23:39:12 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 22 Feb 2010 23:39:05 +0000 Received: (qmail 8617 invoked from network); 22 Feb 2010 23:39:03 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 22 Feb 2010 23:39:03 -0000 From: Pedro Alves To: gdb-patches@sourceware.org, Eli Zaretskii Subject: Re: read watchpoints don't work on targets that support read watchpoints Date: Mon, 22 Feb 2010 23:39:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-19-generic; KDE/4.3.2; x86_64; ; ) References: <201002180111.31520.pedro@codesourcery.com> <201002222009.39573.pedro@codesourcery.com> <83mxz1m0x6.fsf@gnu.org> In-Reply-To: <83mxz1m0x6.fsf@gnu.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201002222338.59235.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-02/txt/msg00565.txt.bz2 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 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 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 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 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 . */ + +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 . + +# This file was written by Pedro Alves + +# 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"