[RFA] Fix hw watchpoints in process record.

Pedro Alves pedro@codesourcery.com
Thu Nov 12 00:27:00 GMT 2009


On Thursday 05 November 2009 19:09:40, Pedro Alves wrote:

> this assumes the target/arch has continuable watchpoints,
> like x86/x86_64.  You'll see this misbehave on mips, when Hui
> contributes the support.
> 
> You're evaluating the watchpoint expression on every event --- I
> have a feeling it would make more sense to decouple precord from
> that, like all other targets are decoupled.  That is, have precord
> check for low level watchpoint triggers --- given [addr,length,type]
> low level watchpoints --- when memory changes (at record_mem_entry time),
> instead of evaluating the whole high-level watchpoint expression.
> 
> [IMO, we should mostly think of precord as just another a
> simulator that mostly only interfaces gdb's core through
> the target_ methods.]


Sure enough, trying a simple test like this shows
breakage:

 1       #include <stdio.h>
 2
 3       int glob;
 4
 5       int main ()
 6       {
 7         int loc = -1;
 8
 9         while (1)
 10          {
 11            printf ("glob = %d\n", glob);
 12            glob++;
 13            printf ("loc = %d\n", loc);
 14            loc++;
 15          }
 16      }


 (gdb) start
 Temporary breakpoint 1, main () at test.c:7
 7         int loc = -1;
 (gdb) record
 (gdb) watch loc
 During symbol reading, incomplete CFI data; unspecified registers (e.g., rax) at 0x40049c.
 Hardware watchpoint 2: loc
 (gdb) watch glob
 Hardware watchpoint 3: glob
 (gdb) n
 Hardware watchpoint 2: loc
 
 Old value = 0
 New value = -1
 main () at test.c:11
 11            printf ("glob = %d\n", glob);
 (gdb)
 glob = 0
 12            glob++;
 (gdb)
 Hardware watchpoint 3: glob
 
 <a few more next's, then>
 
 (gdb) reverse-continue
 Continuing.
 Hardware watchpoint 2: loc
 
 Old value = 1
 New value = 0
 main () at test.c:14
 14            loc++;
 (gdb) reverse-continue
 Continuing.
 
 Watchpoint 2 deleted because the program has left the block in
 which its expression is valid.
 
 Watchpoint 2 deleted because the program has left the block in
 which its expression is valid.
 0x00007ffff7aebd15 in _IO_file_write () from /lib/libc.so.6
 (gdb)

Nope, it hasn't left the block, I was continue'ing in the
loop...  The trouble relates to the fact that precord forces
single-stepping --- which reverse single-steps into the
printf call --- and, watchpoint_check wants to check
if the watchpoints are in scope.


Here's the alternative implementation I was suggesting last
week.  It ends up being simpler and more efficient, in fact.
We checking for watchpoint hits less often, and the check is
much cheaper since it doesn't do any extra reads, create
values, or evaluate expressions.

I also didn't see any need for set_executing calls, which
were a sign of trouble.

I've added a comment indicating what needs to be done
to support targets/archs with non-continuable watchpoints
(just check for inserted watchpoints that should trigger,
before actually doing any memory change)

An alternative to that is to expose better the continuable vs
steppable watchpoints at the target layer, so that record can
always claim continuable watchpoints while replaying, dispite
what the target beneath or the arch supports.


This passed the new watch-reverse.exp and watch-precsave.exp.
Playing a bit with the small test shown above didn't show
any issue.  Want to give it a try?  

-- 
Pedro Alves


---
 gdb/NEWS                                     |    4 +
 gdb/breakpoint.c                             |   33 ++++++++++
 gdb/breakpoint.h                             |    5 +
 gdb/record.c                                 |   68 +++++++++++++++++++---
 gdb/testsuite/gdb.reverse/watch-precsave.exp |   80 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.reverse/watch-reverse.exp  |   82 ++++++++++++++++++++++++++-
 6 files changed, 260 insertions(+), 12 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2009-11-12 00:06:31.000000000 +0000
+++ src/gdb/breakpoint.c	2009-11-12 00:06:55.000000000 +0000
@@ -3063,6 +3063,39 @@ watchpoints_triggered (struct target_wai
   return 1;
 }
 
+int
+hardware_watchpoint_inserted_in_range (CORE_ADDR addr, ULONGEST len)
+{
+  struct breakpoint *bpt;
+
+  ALL_BREAKPOINTS (bpt)
+    {
+      struct bp_location *loc;
+
+      if (bpt->type != bp_hardware_watchpoint
+	  && bpt->type != bp_access_watchpoint)
+	continue;
+
+      if (!breakpoint_enabled (bpt))
+	continue;
+
+      for (loc = bpt->loc; loc; loc = loc->next)
+	if (loc->inserted)
+	  {
+	    CORE_ADDR l, h;
+
+	    /* Check for interception.  */
+
+	    l = max (loc->address, addr);
+	    h = min (loc->address + loc->length, addr + len);
+
+	    if (l < h)
+	      return 1;
+	  }
+    }
+  return 0;
+}
+
 /* Possible return values for watchpoint_check (this can't be an enum
    because of check_errors).  */
 /* The watchpoint has been deleted.  */
Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h	2009-11-12 00:06:31.000000000 +0000
+++ src/gdb/breakpoint.h	2009-11-12 00:06:55.000000000 +0000
@@ -978,4 +978,9 @@ extern struct breakpoint *get_tracepoint
    is newly allocated; the caller should free when done with it.  */
 extern VEC(breakpoint_p) *all_tracepoints (void);
 
+/* Returns true if there's a hardware watchpoint or access watchpoint
+   inserted in the range defined by ADDR and LEN.  */
+extern int hardware_watchpoint_inserted_in_range (CORE_ADDR addr,
+						  ULONGEST len);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2009-11-12 00:06:31.000000000 +0000
+++ src/gdb/record.c	2009-11-12 00:06:55.000000000 +0000
@@ -224,6 +224,7 @@ static int (*record_beneath_to_insert_br
 						   struct bp_target_info *);
 static int (*record_beneath_to_remove_breakpoint) (struct gdbarch *,
 						   struct bp_target_info *);
+static int (*record_beneath_to_stopped_by_watchpoint) (void);
 
 /* Alloc and free functions for record_reg, record_mem, and record_end 
    entries.  */
@@ -673,6 +674,9 @@ record_gdb_operation_disable_set (void)
   return old_cleanups;
 }
 
+/* Flag set to TRUE for target_stopped_by_watchpoint.  */
+static int record_hw_watchpoint = 0;
+
 /* Execute one instruction from the record log.  Each instruction in
    the log will be represented by an arbitrary sequence of register
    entries and memory entries, followed by an 'end' entry.  */
@@ -739,7 +743,21 @@ record_exec_insn (struct regcache *regca
                                entry->u.mem.len);
                   }
                 else
-                  memcpy (record_get_loc (entry), mem, entry->u.mem.len);
+		  {
+		    memcpy (record_get_loc (entry), mem, entry->u.mem.len);
+
+		    /* We've changed memory --- check if a hardware
+		       watchpoint should trap.  Note that this
+		       presently assumes the target beneath supports
+		       continuable watchpoints.  On non-continuable
+		       watchpoints target, we'll want to check this
+		       _before_ actually doing the memory change, and
+		       not doing the change at all if the watchpoint
+		       traps.  */
+		    if (hardware_watchpoint_inserted_in_range
+			(entry->u.mem.addr, entry->u.mem.len))
+		      record_hw_watchpoint = 1;
+		  }
               }
           }
       }
@@ -770,6 +788,7 @@ static int (*tmp_to_insert_breakpoint) (
 					struct bp_target_info *);
 static int (*tmp_to_remove_breakpoint) (struct gdbarch *,
 					struct bp_target_info *);
+static int (*tmp_to_stopped_by_watchpoint) (void);
 
 static void record_restore (void);
 
@@ -894,6 +913,8 @@ record_open (char *name, int from_tty)
 	tmp_to_insert_breakpoint = t->to_insert_breakpoint;
       if (!tmp_to_remove_breakpoint)
 	tmp_to_remove_breakpoint = t->to_remove_breakpoint;
+      if (!tmp_to_stopped_by_watchpoint)
+	tmp_to_stopped_by_watchpoint = t->to_stopped_by_watchpoint;
     }
   if (!tmp_to_xfer_partial)
     error (_("Could not find 'to_xfer_partial' method on the target stack."));
@@ -915,6 +936,7 @@ record_open (char *name, int from_tty)
   record_beneath_to_xfer_partial = tmp_to_xfer_partial;
   record_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint;
   record_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint;
+  record_beneath_to_stopped_by_watchpoint = tmp_to_stopped_by_watchpoint;
 
   if (current_target.to_stratum == core_stratum)
     record_core_open_1 (name, from_tty);
@@ -1069,14 +1091,23 @@ record_wait (struct target_ops *ops,
 		{
 		  struct regcache *regcache;
 
-		  /* Yes -- check if there is a breakpoint.  */
+		  /* Yes -- this is likely our single-step finishing,
+		     but check if there's any reason the core would be
+		     interested in the event.  */
+
 		  registers_changed ();
 		  regcache = get_current_regcache ();
 		  tmp_pc = regcache_read_pc (regcache);
-		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-						  tmp_pc))
+
+		  if (target_stopped_by_watchpoint ())
+		    {
+		      /* Always interested in watchpoints.  */
+		    }
+		  else if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
+						       tmp_pc))
 		    {
-		      /* There is a breakpoint.  GDB will want to stop.  */
+		      /* There is a breakpoint here.  Let the core
+			 handle it.  */
 		      struct gdbarch *gdbarch = get_regcache_arch (regcache);
 		      CORE_ADDR decr_pc_after_break
 			= gdbarch_decr_pc_after_break (gdbarch);
@@ -1086,10 +1117,8 @@ record_wait (struct target_ops *ops,
 		    }
 		  else
 		    {
-		      /* There is not a breakpoint, and gdb is not
-		         stepping, therefore gdb will not stop.
-			 Therefore we will not return to gdb.
-		         Record the insn and resume.  */
+		      /* This must be a single-step trap.  Record the
+		         insn and issue another step.  */
 		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
 			break;
 
@@ -1116,6 +1145,7 @@ record_wait (struct target_ops *ops,
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
       CORE_ADDR tmp_pc;
 
+      record_hw_watchpoint = 0;
       status->kind = TARGET_WAITKIND_STOPPED;
 
       /* Check breakpoint when forward execute.  */
@@ -1219,6 +1249,14 @@ record_wait (struct target_ops *ops,
 					   gdbarch_decr_pc_after_break (gdbarch));
 		      continue_flag = 0;
 		    }
+
+		  if (record_hw_watchpoint)
+		    {
+		      if (record_debug)
+			fprintf_unfiltered (gdb_stdlog,
+					    "Process record: hit hw watchpoint.\n");
+		      continue_flag = 0;
+		    }
 		  /* Check target signal */
 		  if (record_list->u.end.sigval != TARGET_SIGNAL_0)
 		    /* FIXME: better way to check */
@@ -1260,6 +1298,16 @@ replay_out:
   return inferior_ptid;
 }
 
+/* to_stopped_by_watchpoint method */
+static int
+record_stopped_by_watchpoint (void)
+{
+  if (RECORD_IS_REPLAY)
+    return record_hw_watchpoint;
+  else
+    return record_beneath_to_stopped_by_watchpoint ();
+}
+
 /* "to_disconnect" method for process record target.  */
 
 static void
@@ -1545,6 +1593,7 @@ init_record_ops (void)
   record_ops.to_remove_breakpoint = record_remove_breakpoint;
   record_ops.to_can_execute_reverse = record_can_execute_reverse;
   record_ops.to_stratum = record_stratum;
+  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_ops.to_magic = OPS_MAGIC;
 }
 
@@ -1750,6 +1799,7 @@ init_record_core_ops (void)
   record_core_ops.to_can_execute_reverse = record_can_execute_reverse;
   record_core_ops.to_has_execution = record_core_has_execution;
   record_core_ops.to_stratum = record_stratum;
+  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_core_ops.to_magic = OPS_MAGIC;
 }
 
Index: src/gdb/NEWS
===================================================================
--- src.orig/gdb/NEWS	2009-11-12 00:06:31.000000000 +0000
+++ src/gdb/NEWS	2009-11-12 00:06:55.000000000 +0000
@@ -71,6 +71,10 @@ show follow-exec-mode
   creates a new one.  This is useful to be able to restart the old
   executable after the inferior having done an exec call.
 
+* Bug fixes
+
+Process record now works correctly with hardware watchpoints.
+
 *** Changes in GDB 7.0
 
 * GDB now has an interface for JIT compilation.  Applications that
Index: src/gdb/testsuite/gdb.reverse/watch-reverse.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.reverse/watch-reverse.exp	2009-11-12 00:06:31.000000000 +0000
+++ src/gdb/testsuite/gdb.reverse/watch-reverse.exp	2009-11-12 00:10:23.000000000 +0000
@@ -38,8 +38,8 @@ if [target_info exists gdb,use_precord] 
     # FIXME: command ought to acknowledge, so we can test if it succeeded.
 }
 
-# Only software watchpoints can be used in reverse
-gdb_test "set can-use-hw-watchpoints 0" "" ""
+# Test software watchpoints
+gdb_test "set can-use-hw-watchpoints 0" "" "disable hw watchpoints"
 
 gdb_test "break marker1" \
     "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
@@ -122,3 +122,81 @@ gdb_test "continue" \
 gdb_test "continue" \
     ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
     "watchpoint hit in reverse, fifth time"
+
+gdb_test "set can-use-hw-watchpoints 1" "" "enable hw watchpoints"
+
+###
+###
+###
+
+# FIXME 'set exec-dir' command should give some output so we can test.
+gdb_test "set exec-direction forward" "" "set forward"
+
+# Continue until first change, from -1 to 0
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = -1.*New value = 0.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, first time"
+
+# Continue until the next change, from 0 to 1.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = 1.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, second time"
+
+# Continue until the next change, from 1 to 2.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 1.*New value = 2.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, third time"
+
+# Continue until the next change, from 2 to 3.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 2.*New value = 3.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, fourth time"
+
+# Continue until the next change, from 3 to 4.
+# Note that this one is outside the loop.
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 3.*New value = 4.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, fifth time"
+
+# Continue until we hit the finishing marker function.
+# Make sure we hit no more watchpoints.
+
+gdb_test "continue" "marker2 .*" "replay forward to marker2"
+
+###
+###
+###
+
+# FIXME 'set exec-dir' command should give some output so we can test.
+gdb_test "set exec-direction reverse" "" "set reverse"
+
+# Reverse until the previous change, from 4 to 3
+# Note that this one is outside the loop
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 4.*New value = 3.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, first time"
+
+# Reverse until the previous change, from 3 to 2.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 3.*New value = 2.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, second time"
+
+# Reverse until the previous change, from 2 to 1.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 2.*New value = 1.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, third time"
+
+# Reverse until the previous change, from 1 to 0.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 1.*New value = 0.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, fourth time"
+
+# Reverse until first change, from 0 to -1
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, fifth time"
+
Index: src/gdb/testsuite/gdb.reverse/watch-precsave.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.reverse/watch-precsave.exp	2009-11-12 00:06:31.000000000 +0000
+++ src/gdb/testsuite/gdb.reverse/watch-precsave.exp	2009-11-12 00:10:07.000000000 +0000
@@ -1,4 +1,4 @@
-# Copyright 2008, 2009 Free Software Foundation, Inc.
+# Copyright 2009 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
@@ -140,3 +140,81 @@ gdb_test "continue" \
 gdb_test "continue" \
     ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
     "watchpoint hit in reverse, fifth time"
+
+gdb_test "set can-use-hw-watchpoints 1" "" "enable hw watchpoints"
+
+###
+###
+###
+
+# FIXME 'set exec-dir' command should give some output so we can test.
+gdb_test "set exec-direction forward" "" "set forward"
+
+# Continue until first change, from -1 to 0
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = -1.*New value = 0.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, first time"
+
+# Continue until the next change, from 0 to 1.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = 1.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, second time"
+
+# Continue until the next change, from 1 to 2.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 1.*New value = 2.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, third time"
+
+# Continue until the next change, from 2 to 3.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 2.*New value = 3.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, fourth time"
+
+# Continue until the next change, from 3 to 4.
+# Note that this one is outside the loop.
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 3.*New value = 4.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit, forward replay, fifth time"
+
+# Continue until we hit the finishing marker function.
+# Make sure we hit no more watchpoints.
+
+gdb_test "continue" "marker2 .*" "replay forward to marker2"
+
+###
+###
+###
+
+# FIXME 'set exec-dir' command should give some output so we can test.
+gdb_test "set exec-direction reverse" "" "set reverse"
+
+# Reverse until the previous change, from 4 to 3
+# Note that this one is outside the loop
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 4.*New value = 3.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, first time"
+
+# Reverse until the previous change, from 3 to 2.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 3.*New value = 2.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, second time"
+
+# Reverse until the previous change, from 2 to 1.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 2.*New value = 1.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, third time"
+
+# Reverse until the previous change, from 1 to 0.
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 1.*New value = 0.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, fourth time"
+
+# Reverse until first change, from 0 to -1
+
+gdb_test "continue" \
+    ".*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = -1.*ival3 = count; ival4 = count;.*" \
+    "watchpoint hit in reverse, HW, fifth time"
+



More information about the Gdb-patches mailing list