2010-02-22 Pedro Alves <pedro@codesourcery.com>
authorPedro Alves <palves@redhat.com>
Mon, 22 Feb 2010 23:35:17 +0000 (23:35 +0000)
committerPedro Alves <palves@redhat.com>
Mon, 22 Feb 2010 23:35:17 +0000 (23:35 +0000)
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/ChangeLog
gdb/breakpoint.c
gdb/gdbserver/ChangeLog
gdb/gdbserver/i386-low.c
gdb/i386-nat.c
gdb/remote.c
gdb/target.h
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.base/watch-read.c [new file with mode: 0644]
gdb/testsuite/gdb.base/watch-read.exp [new file with mode: 0644]

index af2a4fb7e7921e527da2b7c34bd4409454b2212a..c82bf882917ecf713391533cf8208d00a4387dec 100644 (file)
@@ -1,3 +1,21 @@
+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-19  Tom Tromey  <tromey@redhat.com>
 
        * p-typeprint.c (pascal_type_print_varspec_prefix): Update.
index 8c97949d7dccdeef1533a3be3376361371c2d508..4224c76cbc5a340cf77758b9827d36fbca7d6727 100644 (file)
@@ -127,6 +127,9 @@ static int breakpoint_address_match (struct address_space *aspace1,
                                     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 breakpoints for read-only addresses.\n"));
              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 breakpoint *bpt)
 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_insert)
                          /* 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 fb968ebe0da2b8d534a5329f647f630cd5cc69f8..f7f96cfef48ba8afeb1f3689fb6d35ee380d5568 100644 (file)
@@ -1,3 +1,11 @@
+2010-02-22  Pedro Alves  <pedro@codesourcery.com>
+
+       PR9605
+
+       * 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-12  Doug Evans  <dje@google.com>
 
        * linux-low.c (linux_supports_tracefork_flag): Document.
index 3fdf563c77eace9fd85289a3d02cb01c0c394f9f..494b8087f2aecb985952e2250fb38d19fbd868c0 100644 (file)
@@ -219,7 +219,7 @@ i386_length_and_rw_bits (int len, enum target_hw_bp_type type)
        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_debug_reg_state *state,
   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 fa0cce6ffc7d06cbb4ad6e8428a379a2c79369f4..82c51d70c20d2d3e41f55aa71a50f3517431967d 100644 (file)
@@ -268,7 +268,8 @@ i386_length_and_rw_bits (int len, enum target_hw_bp_type type)
        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 len, int type)
 {
   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 6b1a27bf4fac96b1978f92f5ddcb1386b27188a3..0ed49b9e5164553533fa0a1c84885018a0105cab 100644 (file)
@@ -7341,7 +7341,7 @@ remote_insert_watchpoint (CORE_ADDR addr, int len, int type)
   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, int len, int type)
   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 7103ab26eeeae2564714aca9af29b81a0f4392e9..7fd9badacaf98b2a79207cbe6658385475271f3a 100644 (file)
@@ -1264,9 +1264,10 @@ extern char *normal_pid_to_str (ptid_t ptid);
     (*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)
index 995ac279800bec809bba4d43ac5cdd06463f667e..701055312fd0d52e79db5107b58d5372b4510700 100644 (file)
@@ -1,3 +1,9 @@
+2010-02-22  Pedro Alves  <pedro@codesourcery.com>
+
+       PR9605
+
+       * gdb.base/watch-read.c, gdb.base/watch-read.exp: New files.
+
 2010-02-19  Tom Tromey  <tromey@redhat.com>
 
        PR c++/8693, PR c++/9496:
diff --git a/gdb/testsuite/gdb.base/watch-read.c b/gdb/testsuite/gdb.base/watch-read.c
new file mode 100644 (file)
index 0000000..27c8703
--- /dev/null
@@ -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;
+}
diff --git a/gdb/testsuite/gdb.base/watch-read.exp b/gdb/testsuite/gdb.base/watch-read.exp
new file mode 100644 (file)
index 0000000..98cab04
--- /dev/null
@@ -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"