[RFA] Fix hw watchpoints in process record.

[RFA] Fix hw watchpoints in process record.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

Here is a patch to make hardware watchpoints work in precord.

Biggest change is in breakpoint.c, but it's mainly mechanical.
I abstracted most of the function "watchpoint_check" into a
separate function so it could be called from two places.

Then in record_wait, if not replay mode we call
target_stopped_by_watchpoint to see if a watchpoint
was triggered.  In replay mode we call the new function
hw_watchpoint_check.


[-- Attachment #2: watchpoint.txt --]
[-- Type: text/plain, Size: 10165 bytes --]

2009-10-31  Michael Snyder  <msnyder@vmware.com>
	Make hardware watchpoints work for process record.
	* breakpoint.c (watchpoint_check_1): Abstracted from watchpoint_check.
	(watchpoint_check_2): Check_error entry point for above.
	(watchpoint_check): Call watchpoint_check_1.
	(hw_watchpoint_check): New function.  Return true if 
	a hardware watchpoint expression has changed.
	* breakpoint.h (hw_watchpoint_check): Export.

	* record.c (record_beneath_to_stopped_by_watchpoint): New pointer.
	(record_open): Initialize above pointer.
	(record_stopped_by_watchpoint): New target method.
	(record_wait): Check to see if hitting hardware watchpoint.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2009-10-31 17:31:15.000000000 -0700
+++ gdb/breakpoint.c	2009-10-31 17:50:07.000000000 -0700
@@ -3075,18 +3075,13 @@
 #define BP_TEMPFLAG 1
 #define BP_HARDWAREFLAG 2
 
-/* Check watchpoint condition.  */
-
 static int
-watchpoint_check (void *p)
+watchpoint_check_1 (void *p, struct value **new_valp)
 {
-  bpstat bs = (bpstat) p;
-  struct breakpoint *b;
+  struct breakpoint *b = p;
   struct frame_info *fr;
   int within_current_scope;
 
-  b = bs->breakpoint_at->owner;
-
   if (b->exp_valid_block == NULL)
     within_current_scope = 1;
   else
@@ -3137,20 +3132,16 @@
          we might be in the middle of evaluating a function call.  */
 
       struct value *mark = value_mark ();
-      struct value *new_val;
 
-      fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
-      if ((b->val != NULL) != (new_val != NULL)
-	  || (b->val != NULL && !value_equal (b->val, new_val)))
+      fetch_watchpoint_value (b->exp, new_valp, NULL, NULL);
+      if ((b->val != NULL) != (*new_valp != NULL)
+	  || (b->val != NULL && !value_equal (b->val, *new_valp)))
 	{
-	  if (new_val != NULL)
+	  if (*new_valp != NULL)
 	    {
-	      release_value (new_val);
+	      release_value (*new_valp);
 	      value_free_to_mark (mark);
 	    }
-	  bs->old_val = b->val;
-	  b->val = new_val;
-	  b->val_valid = 1;
 	  /* We will stop here */
 	  return WP_VALUE_CHANGED;
 	}
@@ -3181,8 +3172,9 @@
 	  (uiout, "reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_SCOPE));
       ui_out_text (uiout, "\nWatchpoint ");
       ui_out_field_int (uiout, "wpnum", b->number);
-      ui_out_text (uiout, " deleted because the program has left the block in\n\
-which its expression is valid.\n");     
+      ui_out_text (uiout,
+		   " deleted because the program has left the block in\n\
+which its expression is valid.\n");
 
       if (b->related_breakpoint)
 	b->related_breakpoint->disposition = disp_del_at_next_stop;
@@ -3192,6 +3184,36 @@
     }
 }
 
+static int
+watchpoint_check_2 (void *p)
+{
+  struct value *notused;
+
+  return watchpoint_check_1 (p, &notused);
+}
+
+/* Check watchpoint condition.  */
+
+static int
+watchpoint_check (void *p)
+{
+  bpstat bs = (bpstat) p;
+  struct value *new_val;
+  struct breakpoint *b;
+  int ret;
+
+  b = bs->breakpoint_at->owner;
+  ret = watchpoint_check_1 (b, &new_val);
+
+  if (ret == WP_VALUE_CHANGED)
+    {
+      bs->old_val = b->val;
+      b->val = new_val;
+      b->val_valid = 1;
+    }
+  return ret;
+}
+
 /* Return true if it looks like target has stopped due to hitting
    breakpoint location BL.  This function does not check if we
    should stop, only if BL explains the stop.   */
@@ -3250,6 +3272,29 @@
   return 1;
 }
 
+int
+hw_watchpoint_check (void)
+{
+  struct breakpoint *b;
+  struct value *new_val;
+
+  ALL_BREAKPOINTS (b)
+    if (b->type == bp_hardware_watchpoint
+	|| b->type == bp_access_watchpoint)
+      {
+	char *msg
+	  = xstrprintf (_("Error evaluating expression for watchpoint %d\n"),
+			b->number);
+	struct cleanup *cleanups = make_cleanup (xfree, msg);
+	int e = catch_errors (watchpoint_check_2, b, msg, RETURN_MASK_ALL);
+	do_cleanups (cleanups);
+	if (e == WP_VALUE_CHANGED)
+	  return 1;	/* should stop */
+      }
+  return 0;		/* don't stop */
+}
+
+
 /* If BS refers to a watchpoint, determine if the watched values
    has actually changed, and we should stop.  If not, set BS->stop
    to 0.  */
@@ -3267,7 +3312,7 @@
       CORE_ADDR addr;
       struct value *v;
       int must_check_value = 0;
-      
+
       if (b->type == bp_watchpoint)
 	/* For a software watchpoint, we must always check the
 	   watched value.  */
@@ -3284,7 +3329,7 @@
 	   value.  Access and read watchpoints are out of luck; without
 	   a data address, we can't figure it out.  */
 	must_check_value = 1;
-      
+
       if (must_check_value)
 	{
 	  char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
Index: gdb/breakpoint.h
===================================================================
--- gdb.orig/breakpoint.h	2009-10-31 17:31:21.000000000 -0700
+++ gdb/breakpoint.h	2009-10-31 17:33:23.000000000 -0700
@@ -978,4 +978,6 @@
    is newly allocated; the caller should free when done with it.  */
 extern VEC(breakpoint_p) *all_tracepoints (void);
 
+extern int hw_watchpoint_check (void);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: gdb/record.c
===================================================================
--- gdb.orig/record.c	2009-10-31 17:31:00.000000000 -0700
+++ gdb/record.c	2009-10-31 17:34:24.000000000 -0700
@@ -224,6 +224,7 @@
 						   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.  */
@@ -770,6 +771,7 @@
 					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 +896,8 @@
 	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 +919,7 @@
   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);
@@ -1010,6 +1015,9 @@
     record_list = record_list->prev;
 }
 
+/* Flag set to TRUE for target_stopped_by_watchpoint.  */
+static int record_hw_watchpoint = 0;
+
 /* "to_wait" target method for process record target.
 
    In record mode, the target is always run in singlestep mode
@@ -1069,21 +1077,27 @@
 		{
 		  struct regcache *regcache;
 
-		  /* Yes -- check if there is a breakpoint.  */
+		  /* Yes -- check if there is a breakpoint or watchpoint.  */
 		  registers_changed ();
 		  regcache = get_current_regcache ();
 		  tmp_pc = regcache_read_pc (regcache);
 		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-						  tmp_pc))
+						  tmp_pc)
+		      || target_stopped_by_watchpoint ())
 		    {
-		      /* There is a breakpoint.  GDB will want to stop.  */
-		      struct gdbarch *gdbarch = get_regcache_arch (regcache);
+		      /* There is a breakpoint, or watchpoint.
+			 GDB will want to stop.  */
+		      if (!target_stopped_by_watchpoint ())
+			{
+			  struct gdbarch *gdbarch
+			    = get_regcache_arch (regcache);
 		      CORE_ADDR decr_pc_after_break
 			= gdbarch_decr_pc_after_break (gdbarch);
 		      if (decr_pc_after_break)
 			regcache_write_pc (regcache,
 					   tmp_pc + decr_pc_after_break);
 		    }
+		    }
 		  else
 		    {
 		      /* There is not a breakpoint, and gdb is not
@@ -1116,9 +1130,10 @@
       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.  */
+      /* Check for breakpoint or watchpoint when forward execute.  */
       if (execution_direction == EXEC_FORWARD)
 	{
 	  tmp_pc = regcache_read_pc (regcache);
@@ -1136,6 +1151,14 @@
 				   gdbarch_decr_pc_after_break (gdbarch));
 	      goto replay_out;
 	    }
+	  if (hw_watchpoint_check ())
+	    {
+	      if (record_debug)
+		fprintf_unfiltered (gdb_stdlog,
+				    "Process record: hit hw watchpoint.\n");
+	      record_hw_watchpoint = 1;
+	    }
+
 	}
 
       record_get_sig = 0;
@@ -1219,6 +1242,15 @@
 					   gdbarch_decr_pc_after_break (gdbarch));
 		      continue_flag = 0;
 		    }
+		  /* check watchpoint */
+		  if (hw_watchpoint_check ())
+		    {
+		      if (record_debug)
+			fprintf_unfiltered (gdb_stdlog,
+					    "Process record: hit hw watchpoint.\n");
+		      record_hw_watchpoint = 1;
+		      continue_flag = 0;
+		    }
 		  /* Check target signal */
 		  if (record_list->u.end.sigval != TARGET_SIGNAL_0)
 		    /* FIXME: better way to check */
@@ -1260,6 +1292,16 @@
   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
@@ -1587,6 +1629,7 @@
   /* Add bookmark target methods.  */
   record_ops.to_get_bookmark = record_get_bookmark;
   record_ops.to_goto_bookmark = record_goto_bookmark;
+  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_ops.to_magic = OPS_MAGIC;
 }
 
@@ -1795,6 +1838,7 @@
   /* Add bookmark target methods.  */
   record_core_ops.to_get_bookmark = record_get_bookmark;
   record_core_ops.to_goto_bookmark = record_goto_bookmark;
+  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_core_ops.to_magic = OPS_MAGIC;
 }
 

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

Hi Michael,

I do some test with this patch.  But sometime hb didn't work in replay mode.
For example:
Temporary breakpoint 1, main () at 1.c:20
20	       int     b = 0;
(gdb) record
(gdb) n
During symbol reading, incomplete CFI data; unspecified registers
(e.g., eax) at 0x80483be.
21	       int     c = 1;
(gdb) hb
Hardware assisted breakpoint 2 at 0x80483c8: file 1.c, line 21.
(gdb) c
Continuing.
a = 0 b = 0 c = 1
a = 3
a = 3 b = 3 c = 1
a = 3 b = 3 c = 2
a = 1 b = 3 c = 2
The next instruction is syscall exit_group.  It will make the program
exit.  Do you want to stop the program?([y] or n)
Process record: inferior program stopped.

Program received signal SIGTRAP, Trace/breakpoint trap.
0xb7fe3405 in __kernel_vsyscall ()
(gdb) rc
Continuing.

Breakpoint 2, main () at 1.c:21
21	       int     c = 1;
(gdb) info b
Num     Type           Disp Enb Address    What
2       hw breakpoint  keep y   0x080483c8 in main at 1.c:21
	breakpoint already hit 1 time
(gdb) c
Continuing.

No more reverse-execution history.
0xb7fe3405 in __kernel_vsyscall ()
(gdb) rc
Continuing.

Breakpoint 2, main () at 1.c:21
21	       int     c = 1;


Thanks,
Hui

On Sun, Nov 1, 2009 at 09:15, Michael Snyder <msnyder@vmware.com> wrote:
> Here is a patch to make hardware watchpoints work in precord.
>
> Biggest change is in breakpoint.c, but it's mainly mechanical.
> I abstracted most of the function "watchpoint_check" into a
> separate function so it could be called from two places.
>
> Then in record_wait, if not replay mode we call
> target_stopped_by_watchpoint to see if a watchpoint
> was triggered.  In replay mode we call the new function
> hw_watchpoint_check.
>
>
> 2009-10-31  Michael Snyder  <msnyder@vmware.com>
>        Make hardware watchpoints work for process record.
>        * breakpoint.c (watchpoint_check_1): Abstracted from
> watchpoint_check.
>        (watchpoint_check_2): Check_error entry point for above.
>        (watchpoint_check): Call watchpoint_check_1.
>        (hw_watchpoint_check): New function.  Return true if
>        a hardware watchpoint expression has changed.
>        * breakpoint.h (hw_watchpoint_check): Export.
>
>        * record.c (record_beneath_to_stopped_by_watchpoint): New pointer.
>        (record_open): Initialize above pointer.
>        (record_stopped_by_watchpoint): New target method.
>        (record_wait): Check to see if hitting hardware watchpoint.
>
> Index: gdb/breakpoint.c
> ===================================================================
> --- gdb.orig/breakpoint.c       2009-10-31 17:31:15.000000000 -0700
> +++ gdb/breakpoint.c    2009-10-31 17:50:07.000000000 -0700
> @@ -3075,18 +3075,13 @@
>  #define BP_TEMPFLAG 1
>  #define BP_HARDWAREFLAG 2
>
> -/* Check watchpoint condition.  */
> -
>  static int
> -watchpoint_check (void *p)
> +watchpoint_check_1 (void *p, struct value **new_valp)
>  {
> -  bpstat bs = (bpstat) p;
> -  struct breakpoint *b;
> +  struct breakpoint *b = p;
>   struct frame_info *fr;
>   int within_current_scope;
>
> -  b = bs->breakpoint_at->owner;
> -
>   if (b->exp_valid_block == NULL)
>     within_current_scope = 1;
>   else
> @@ -3137,20 +3132,16 @@
>          we might be in the middle of evaluating a function call.  */
>
>       struct value *mark = value_mark ();
> -      struct value *new_val;
>
> -      fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
> -      if ((b->val != NULL) != (new_val != NULL)
> -         || (b->val != NULL && !value_equal (b->val, new_val)))
> +      fetch_watchpoint_value (b->exp, new_valp, NULL, NULL);
> +      if ((b->val != NULL) != (*new_valp != NULL)
> +         || (b->val != NULL && !value_equal (b->val, *new_valp)))
>        {
> -         if (new_val != NULL)
> +         if (*new_valp != NULL)
>            {
> -             release_value (new_val);
> +             release_value (*new_valp);
>              value_free_to_mark (mark);
>            }
> -         bs->old_val = b->val;
> -         b->val = new_val;
> -         b->val_valid = 1;
>          /* We will stop here */
>          return WP_VALUE_CHANGED;
>        }
> @@ -3181,8 +3172,9 @@
>          (uiout, "reason", async_reason_lookup
> (EXEC_ASYNC_WATCHPOINT_SCOPE));
>       ui_out_text (uiout, "\nWatchpoint ");
>       ui_out_field_int (uiout, "wpnum", b->number);
> -      ui_out_text (uiout, " deleted because the program has left the block
> in\n\
> -which its expression is valid.\n");
> +      ui_out_text (uiout,
> +                  " deleted because the program has left the block in\n\
> +which its expression is valid.\n");
>
>       if (b->related_breakpoint)
>        b->related_breakpoint->disposition = disp_del_at_next_stop;
> @@ -3192,6 +3184,36 @@
>     }
>  }
>
> +static int
> +watchpoint_check_2 (void *p)
> +{
> +  struct value *notused;
> +
> +  return watchpoint_check_1 (p, &notused);
> +}
> +
> +/* Check watchpoint condition.  */
> +
> +static int
> +watchpoint_check (void *p)
> +{
> +  bpstat bs = (bpstat) p;
> +  struct value *new_val;
> +  struct breakpoint *b;
> +  int ret;
> +
> +  b = bs->breakpoint_at->owner;
> +  ret = watchpoint_check_1 (b, &new_val);
> +
> +  if (ret == WP_VALUE_CHANGED)
> +    {
> +      bs->old_val = b->val;
> +      b->val = new_val;
> +      b->val_valid = 1;
> +    }
> +  return ret;
> +}
> +
>  /* Return true if it looks like target has stopped due to hitting
>    breakpoint location BL.  This function does not check if we
>    should stop, only if BL explains the stop.   */
> @@ -3250,6 +3272,29 @@
>   return 1;
>  }
>
> +int
> +hw_watchpoint_check (void)
> +{
> +  struct breakpoint *b;
> +  struct value *new_val;
> +
> +  ALL_BREAKPOINTS (b)
> +    if (b->type == bp_hardware_watchpoint
> +       || b->type == bp_access_watchpoint)
> +      {
> +       char *msg
> +         = xstrprintf (_("Error evaluating expression for watchpoint
> %d\n"),
> +                       b->number);
> +       struct cleanup *cleanups = make_cleanup (xfree, msg);
> +       int e = catch_errors (watchpoint_check_2, b, msg, RETURN_MASK_ALL);
> +       do_cleanups (cleanups);
> +       if (e == WP_VALUE_CHANGED)
> +         return 1;     /* should stop */
> +      }
> +  return 0;            /* don't stop */
> +}
> +
> +
>  /* If BS refers to a watchpoint, determine if the watched values
>    has actually changed, and we should stop.  If not, set BS->stop
>    to 0.  */
> @@ -3267,7 +3312,7 @@
>       CORE_ADDR addr;
>       struct value *v;
>       int must_check_value = 0;
> -
> +
>       if (b->type == bp_watchpoint)
>        /* For a software watchpoint, we must always check the
>           watched value.  */
> @@ -3284,7 +3329,7 @@
>           value.  Access and read watchpoints are out of luck; without
>           a data address, we can't figure it out.  */
>        must_check_value = 1;
> -
> +
>       if (must_check_value)
>        {
>          char *message = xstrprintf ("Error evaluating expression for
> watchpoint %d\n",
> Index: gdb/breakpoint.h
> ===================================================================
> --- gdb.orig/breakpoint.h       2009-10-31 17:31:21.000000000 -0700
> +++ gdb/breakpoint.h    2009-10-31 17:33:23.000000000 -0700
> @@ -978,4 +978,6 @@
>    is newly allocated; the caller should free when done with it.  */
>  extern VEC(breakpoint_p) *all_tracepoints (void);
>
> +extern int hw_watchpoint_check (void);
> +
>  #endif /* !defined (BREAKPOINT_H) */
> Index: gdb/record.c
> ===================================================================
> --- gdb.orig/record.c   2009-10-31 17:31:00.000000000 -0700
> +++ gdb/record.c        2009-10-31 17:34:24.000000000 -0700
> @@ -224,6 +224,7 @@
>                                                   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.  */
> @@ -770,6 +771,7 @@
>                                        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 +896,8 @@
>        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 +919,7 @@
>   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);
> @@ -1010,6 +1015,9 @@
>     record_list = record_list->prev;
>  }
>
> +/* Flag set to TRUE for target_stopped_by_watchpoint.  */
> +static int record_hw_watchpoint = 0;
> +
>  /* "to_wait" target method for process record target.
>
>    In record mode, the target is always run in singlestep mode
> @@ -1069,21 +1077,27 @@
>                {
>                  struct regcache *regcache;
>
> -                 /* Yes -- check if there is a breakpoint.  */
> +                 /* Yes -- check if there is a breakpoint or watchpoint.
>  */
>                  registers_changed ();
>                  regcache = get_current_regcache ();
>                  tmp_pc = regcache_read_pc (regcache);
>                  if (breakpoint_inserted_here_p (get_regcache_aspace
> (regcache),
> -                                                 tmp_pc))
> +                                                 tmp_pc)
> +                     || target_stopped_by_watchpoint ())
>                    {
> -                     /* There is a breakpoint.  GDB will want to stop.  */
> -                     struct gdbarch *gdbarch = get_regcache_arch
> (regcache);
> +                     /* There is a breakpoint, or watchpoint.
> +                        GDB will want to stop.  */
> +                     if (!target_stopped_by_watchpoint ())
> +                       {
> +                         struct gdbarch *gdbarch
> +                           = get_regcache_arch (regcache);
>                      CORE_ADDR decr_pc_after_break
>                        = gdbarch_decr_pc_after_break (gdbarch);
>                      if (decr_pc_after_break)
>                        regcache_write_pc (regcache,
>                                           tmp_pc + decr_pc_after_break);
>                    }
> +                   }
>                  else
>                    {
>                      /* There is not a breakpoint, and gdb is not
> @@ -1116,9 +1130,10 @@
>       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.  */
> +      /* Check for breakpoint or watchpoint when forward execute.  */
>       if (execution_direction == EXEC_FORWARD)
>        {
>          tmp_pc = regcache_read_pc (regcache);
> @@ -1136,6 +1151,14 @@
>                                   gdbarch_decr_pc_after_break (gdbarch));
>              goto replay_out;
>            }
> +         if (hw_watchpoint_check ())
> +           {
> +             if (record_debug)
> +               fprintf_unfiltered (gdb_stdlog,
> +                                   "Process record: hit hw watchpoint.\n");
> +             record_hw_watchpoint = 1;
> +           }
> +
>        }
>
>       record_get_sig = 0;
> @@ -1219,6 +1242,15 @@
>                                           gdbarch_decr_pc_after_break
> (gdbarch));
>                      continue_flag = 0;
>                    }
> +                 /* check watchpoint */
> +                 if (hw_watchpoint_check ())
> +                   {
> +                     if (record_debug)
> +                       fprintf_unfiltered (gdb_stdlog,
> +                                           "Process record: hit hw
> watchpoint.\n");
> +                     record_hw_watchpoint = 1;
> +                     continue_flag = 0;
> +                   }
>                  /* Check target signal */
>                  if (record_list->u.end.sigval != TARGET_SIGNAL_0)
>                    /* FIXME: better way to check */
> @@ -1260,6 +1292,16 @@
>   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
> @@ -1587,6 +1629,7 @@
>   /* Add bookmark target methods.  */
>   record_ops.to_get_bookmark = record_get_bookmark;
>   record_ops.to_goto_bookmark = record_goto_bookmark;
> +  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>   record_ops.to_magic = OPS_MAGIC;
>  }
>
> @@ -1795,6 +1838,7 @@
>   /* Add bookmark target methods.  */
>   record_core_ops.to_get_bookmark = record_get_bookmark;
>   record_core_ops.to_goto_bookmark = record_goto_bookmark;
> +  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>   record_core_ops.to_magic = OPS_MAGIC;
>  }
>
>
>

Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder]

Hui Zhu wrote:
> Hi Michael,
> 
> I do some test with this patch.  But sometime hb didn't work in replay mode.
> For example:
> Temporary breakpoint 1, main () at 1.c:20
> 20             int     b = 0;
> (gdb) record
> (gdb) n
> During symbol reading, incomplete CFI data; unspecified registers
> (e.g., eax) at 0x80483be.
> 21             int     c = 1;
> (gdb) hb
> Hardware assisted breakpoint 2 at 0x80483c8: file 1.c, line 21.
> (gdb) c
> Continuing.
> a = 0 b = 0 c = 1
> a = 3
> a = 3 b = 3 c = 1
> a = 3 b = 3 c = 2
> a = 1 b = 3 c = 2
> The next instruction is syscall exit_group.  It will make the program
> exit.  Do you want to stop the program?([y] or n)
> Process record: inferior program stopped.
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> 0xb7fe3405 in __kernel_vsyscall ()
> (gdb) rc
> Continuing.
> 
> Breakpoint 2, main () at 1.c:21
> 21             int     c = 1;
> (gdb) info b
> Num     Type           Disp Enb Address    What
> 2       hw breakpoint  keep y   0x080483c8 in main at 1.c:21
>         breakpoint already hit 1 time
> (gdb) c
> Continuing.
> 
> No more reverse-execution history.
> 0xb7fe3405 in __kernel_vsyscall ()
> (gdb) rc
> Continuing.
> 
> Breakpoint 2, main () at 1.c:21
> 21             int     c = 1;
> 
> 
> Thanks,
> Hui
> 


Source code for your test case?

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]

On Thu, Nov 5, 2009 at 01:51, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Hi Michael,
>>
>> I do some test with this patch.  But sometime hb didn't work in replay
>> mode.
>> For example:
>> Temporary breakpoint 1, main () at 1.c:20
>> 20             int     b = 0;
>> (gdb) record
>> (gdb) n
>> During symbol reading, incomplete CFI data; unspecified registers
>> (e.g., eax) at 0x80483be.
>> 21             int     c = 1;
>> (gdb) hb
>> Hardware assisted breakpoint 2 at 0x80483c8: file 1.c, line 21.
>> (gdb) c
>> Continuing.
>> a = 0 b = 0 c = 1
>> a = 3
>> a = 3 b = 3 c = 1
>> a = 3 b = 3 c = 2
>> a = 1 b = 3 c = 2
>> The next instruction is syscall exit_group.  It will make the program
>> exit.  Do you want to stop the program?([y] or n)
>> Process record: inferior program stopped.
>>
>> Program received signal SIGTRAP, Trace/breakpoint trap.
>> 0xb7fe3405 in __kernel_vsyscall ()
>> (gdb) rc
>> Continuing.
>>
>> Breakpoint 2, main () at 1.c:21
>> 21             int     c = 1;
>> (gdb) info b
>> Num     Type           Disp Enb Address    What
>> 2       hw breakpoint  keep y   0x080483c8 in main at 1.c:21
>>        breakpoint already hit 1 time
>> (gdb) c
>> Continuing.
>>
>> No more reverse-execution history.
>> 0xb7fe3405 in __kernel_vsyscall ()
>> (gdb) rc
>> Continuing.
>>
>> Breakpoint 2, main () at 1.c:21
>> 21             int     c = 1;
>>
>>
>> Thanks,
>> Hui
>>
>
>
> Source code for your test case?
>
>

[-- Attachment #2: 1.c --]
[-- Type: text/x-csrc, Size: 427 bytes --]

int     a = 0;
void
cool2 ()
{
	printf ("a = %d\n", a);
}
int
cool ()
{
       a += 3;


cool2();

       return (a);
}
int
main()
{
       int     b = 0;
       int     c = 1;


	printf ("a = %d b = %d c = %d\n", a, b, c);
       b = cool ();
	printf ("a = %d b = %d c = %d\n", a, b, c);


       c += 1;
	printf ("a = %d b = %d c = %d\n", a, b, c);
       a -= 2;
	printf ("a = %d b = %d c = %d\n", a, b, c);


return (0);
}

Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 169 bytes --]

Hui Zhu wrote:
> Hi Michael,
> 
> I do some test with this patch.  But sometime hb didn't work in replay mode.

OK, thanks, I see the problem.

Try this patch instead.


[-- Attachment #2: watchpoint.txt --]
[-- Type: text/plain, Size: 18388 bytes --]

2009-10-31  Michael Snyder  <msnyder@vmware.com>
	Make hardware watchpoints work for process record.
	* breakpoint.c (watchpoint_check_1): Abstracted from watchpoint_check.
	(watchpoint_check_2): Check_error entry point for above.
	(watchpoint_check): Call watchpoint_check_1.
	(hw_watchpoint_check): New function.  Return true if 
	a hardware watchpoint expression has changed.
	* breakpoint.h (hw_watchpoint_check): Export.

	* record.c (record_beneath_to_stopped_by_watchpoint): New pointer.
	(record_open): Initialize above pointer.
	(record_stopped_by_watchpoint): New target method.
	(record_wait): Check to see if hitting hardware watchpoint.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2009-11-05 09:54:36.000000000 -0800
+++ gdb/breakpoint.c	2009-11-05 10:19:07.000000000 -0800
@@ -3075,18 +3075,13 @@
 #define BP_TEMPFLAG 1
 #define BP_HARDWAREFLAG 2
 
-/* Check watchpoint condition.  */
-
 static int
-watchpoint_check (void *p)
+watchpoint_check_1 (void *p, struct value **new_valp)
 {
-  bpstat bs = (bpstat) p;
-  struct breakpoint *b;
+  struct breakpoint *b = p;
   struct frame_info *fr;
   int within_current_scope;
 
-  b = bs->breakpoint_at->owner;
-
   if (b->exp_valid_block == NULL)
     within_current_scope = 1;
   else
@@ -3137,20 +3132,16 @@
          we might be in the middle of evaluating a function call.  */
 
       struct value *mark = value_mark ();
-      struct value *new_val;
 
-      fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
-      if ((b->val != NULL) != (new_val != NULL)
-	  || (b->val != NULL && !value_equal (b->val, new_val)))
+      fetch_watchpoint_value (b->exp, new_valp, NULL, NULL);
+      if ((b->val != NULL) != (*new_valp != NULL)
+	  || (b->val != NULL && !value_equal (b->val, *new_valp)))
 	{
-	  if (new_val != NULL)
+	  if (*new_valp != NULL)
 	    {
-	      release_value (new_val);
+	      release_value (*new_valp);
 	      value_free_to_mark (mark);
 	    }
-	  bs->old_val = b->val;
-	  b->val = new_val;
-	  b->val_valid = 1;
 	  /* We will stop here */
 	  return WP_VALUE_CHANGED;
 	}
@@ -3181,8 +3172,9 @@
 	  (uiout, "reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_SCOPE));
       ui_out_text (uiout, "\nWatchpoint ");
       ui_out_field_int (uiout, "wpnum", b->number);
-      ui_out_text (uiout, " deleted because the program has left the block in\n\
-which its expression is valid.\n");     
+      ui_out_text (uiout,
+		   " deleted because the program has left the block in\n\
+which its expression is valid.\n");
 
       if (b->related_breakpoint)
 	b->related_breakpoint->disposition = disp_del_at_next_stop;
@@ -3192,6 +3184,36 @@
     }
 }
 
+static int
+watchpoint_check_2 (void *p)
+{
+  struct value *notused;
+
+  return watchpoint_check_1 (p, &notused);
+}
+
+/* Check watchpoint condition.  */
+
+static int
+watchpoint_check (void *p)
+{
+  bpstat bs = (bpstat) p;
+  struct value *new_val;
+  struct breakpoint *b;
+  int ret;
+
+  b = bs->breakpoint_at->owner;
+  ret = watchpoint_check_1 (b, &new_val);
+
+  if (ret == WP_VALUE_CHANGED)
+    {
+      bs->old_val = b->val;
+      b->val = new_val;
+      b->val_valid = 1;
+    }
+  return ret;
+}
+
 /* Return true if it looks like target has stopped due to hitting
    breakpoint location BL.  This function does not check if we
    should stop, only if BL explains the stop.   */
@@ -3250,6 +3272,29 @@
   return 1;
 }
 
+int
+hw_watchpoint_check (void)
+{
+  struct breakpoint *b;
+  struct value *new_val;
+
+  ALL_BREAKPOINTS (b)
+    if (b->type == bp_hardware_watchpoint
+	|| b->type == bp_access_watchpoint)
+      {
+	char *msg
+	  = xstrprintf (_("Error evaluating expression for watchpoint %d\n"),
+			b->number);
+	struct cleanup *cleanups = make_cleanup (xfree, msg);
+	int e = catch_errors (watchpoint_check_2, b, msg, RETURN_MASK_ALL);
+	do_cleanups (cleanups);
+	if (e == WP_VALUE_CHANGED)
+	  return 1;	/* should stop */
+      }
+  return 0;		/* don't stop */
+}
+
+
 /* If BS refers to a watchpoint, determine if the watched values
    has actually changed, and we should stop.  If not, set BS->stop
    to 0.  */
@@ -3267,7 +3312,7 @@
       CORE_ADDR addr;
       struct value *v;
       int must_check_value = 0;
-      
+
       if (b->type == bp_watchpoint)
 	/* For a software watchpoint, we must always check the
 	   watched value.  */
@@ -3284,7 +3329,7 @@
 	   value.  Access and read watchpoints are out of luck; without
 	   a data address, we can't figure it out.  */
 	must_check_value = 1;
-      
+
       if (must_check_value)
 	{
 	  char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
Index: gdb/breakpoint.h
===================================================================
--- gdb.orig/breakpoint.h	2009-11-05 09:54:36.000000000 -0800
+++ gdb/breakpoint.h	2009-11-05 10:13:18.000000000 -0800
@@ -978,4 +978,6 @@
    is newly allocated; the caller should free when done with it.  */
 extern VEC(breakpoint_p) *all_tracepoints (void);
 
+extern int hw_watchpoint_check (void);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: gdb/record.c
===================================================================
--- gdb.orig/record.c	2009-11-05 09:54:36.000000000 -0800
+++ gdb/record.c	2009-11-05 10:24:30.000000000 -0800
@@ -224,6 +224,7 @@
 						   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.  */
@@ -770,6 +771,7 @@
 					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 +896,8 @@
 	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 +919,7 @@
   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);
@@ -1010,6 +1015,9 @@
     record_list = record_list->prev;
 }
 
+/* Flag set to TRUE for target_stopped_by_watchpoint.  */
+static int record_hw_watchpoint = 0;
+
 /* "to_wait" target method for process record target.
 
    In record mode, the target is always run in singlestep mode
@@ -1069,21 +1077,27 @@
 		{
 		  struct regcache *regcache;
 
-		  /* Yes -- check if there is a breakpoint.  */
+		  /* Yes -- check if there is a breakpoint or watchpoint.  */
 		  registers_changed ();
 		  regcache = get_current_regcache ();
 		  tmp_pc = regcache_read_pc (regcache);
 		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-						  tmp_pc))
+						  tmp_pc)
+		      || target_stopped_by_watchpoint ())
 		    {
-		      /* There is a breakpoint.  GDB will want to stop.  */
-		      struct gdbarch *gdbarch = get_regcache_arch (regcache);
+		      /* There is a breakpoint, or watchpoint.
+			 GDB will want to stop.  */
+		      if (!target_stopped_by_watchpoint ())
+			{
+			  struct gdbarch *gdbarch
+			    = get_regcache_arch (regcache);
 		      CORE_ADDR decr_pc_after_break
 			= gdbarch_decr_pc_after_break (gdbarch);
 		      if (decr_pc_after_break)
 			regcache_write_pc (regcache,
 					   tmp_pc + decr_pc_after_break);
 		    }
+		    }
 		  else
 		    {
 		      /* There is not a breakpoint, and gdb is not
@@ -1116,9 +1130,10 @@
       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.  */
+      /* Check for breakpoint or watchpoint when forward execute.  */
       if (execution_direction == EXEC_FORWARD)
 	{
 	  tmp_pc = regcache_read_pc (regcache);
@@ -1136,6 +1151,15 @@
 				   gdbarch_decr_pc_after_break (gdbarch));
 	      goto replay_out;
 	    }
+	  set_executing (inferior_ptid, 0);
+	  if (hw_watchpoint_check ())
+	    {
+	      if (record_debug)
+		fprintf_unfiltered (gdb_stdlog,
+				    "Process record: hit hw watchpoint.\n");
+	      record_hw_watchpoint = 1;
+	    }
+
 	}
 
       record_get_sig = 0;
@@ -1155,6 +1179,7 @@
 	 stop.  */
       do
 	{
+	  set_executing (inferior_ptid, 0);
 	  /* Check for beginning and end of log.  */
 	  if (execution_direction == EXEC_REVERSE
 	      && record_list == &record_first)
@@ -1219,6 +1244,15 @@
 					   gdbarch_decr_pc_after_break (gdbarch));
 		      continue_flag = 0;
 		    }
+		  /* check watchpoint */
+		  if (hw_watchpoint_check ())
+		    {
+		      if (record_debug)
+			fprintf_unfiltered (gdb_stdlog,
+					    "Process record: hit hw watchpoint.\n");
+		      record_hw_watchpoint = 1;
+		      continue_flag = 0;
+		    }
 		  /* Check target signal */
 		  if (record_list->u.end.sigval != TARGET_SIGNAL_0)
 		    /* FIXME: better way to check */
@@ -1238,6 +1272,7 @@
 		  if (record_list->next)
 		    record_list = record_list->next;
 		}
+	      set_executing (inferior_ptid, 1);
 	    }
 	}
       while (continue_flag);
@@ -1260,6 +1295,16 @@
   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
@@ -1599,6 +1644,7 @@
   /* Add bookmark target methods.  */
   record_ops.to_get_bookmark = record_get_bookmark;
   record_ops.to_goto_bookmark = record_goto_bookmark;
+  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_ops.to_magic = OPS_MAGIC;
 }
 
@@ -1807,6 +1853,7 @@
   /* Add bookmark target methods.  */
   record_core_ops.to_get_bookmark = record_get_bookmark;
   record_core_ops.to_goto_bookmark = record_goto_bookmark;
+  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_core_ops.to_magic = OPS_MAGIC;
 }
 
Index: gdb/NEWS
===================================================================
--- gdb.orig/NEWS	2009-11-05 09:54:36.000000000 -0800
+++ gdb/NEWS	2009-11-05 10:13:18.000000000 -0800
@@ -88,6 +88,10 @@
   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: gdb/testsuite/gdb.reverse/watch-reverse.exp
===================================================================
--- gdb.orig/testsuite/gdb.reverse/watch-reverse.exp	2009-11-05 09:54:36.000000000 -0800
+++ gdb/testsuite/gdb.reverse/watch-reverse.exp	2009-11-05 10:13:18.000000000 -0800
@@ -38,8 +38,8 @@
     # 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" \
     ".*\[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: gdb/testsuite/gdb.reverse/watch-precsave.exp
===================================================================
--- gdb.orig/testsuite/gdb.reverse/watch-precsave.exp	2009-11-05 09:54:36.000000000 -0800
+++ gdb/testsuite/gdb.reverse/watch-precsave.exp	2009-11-05 10:13:18.000000000 -0800
@@ -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" \
     ".*\[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"
+

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

On Thursday 05 November 2009 18:40:31, Michael Snyder wrote:
> Hui Zhu wrote:
> > Hi Michael,
> > 
> > I do some test with this patch.  But sometime hb didn't work in replay mode.
> 
> OK, thanks, I see the problem.
> 
> Try this patch instead.
> 
> 

Quick comment:

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.]

-- 
Pedro Alves

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

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"
+

Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder]

OK, I withdraw my patch in favor of this one.
If Hui likes it, let's check it in.

Thanks for your help, Pedro!

Pedro Alves wrote:
> 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"
> +
> 

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

Hi guys,

There are some patches about hw breakpoint are running on maillist.
If you don't mind, I suggest we wait until they OK.

Thanks,
Hui

On Sat, Nov 21, 2009 at 02:06, Michael Snyder <msnyder@vmware.com> wrote:
> OK, I withdraw my patch in favor of this one.
> If Hui likes it, let's check it in.
>
> Thanks for your help, Pedro!
>
> Pedro Alves wrote:
>>
>> 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"
>> +
>>
>
>

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

On Saturday 21 November 2009 09:39:34, Hui Zhu wrote:

> There are some patches about hw breakpoint are running on maillist.
> If you don't mind, I suggest we wait until they OK.

What/which patches?

-- 
Pedro Alves

Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder]

Hui Zhu wrote:
> Hi guys,
> 
> There are some patches about hw breakpoint are running on maillist.
> If you don't mind, I suggest we wait until they OK.

I don't think there are any conflicts...

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

I think this patch still not OK.

(gdb) start
Temporary breakpoint 1 at 0x400554: file 1.c, line 20.
Starting program: /home/teawater/gdb/bgdbno/gdb/a.out

Temporary breakpoint 1, main () at 1.c:20
20	       int     b = 0;
(gdb) disas
Dump of assembler code for function main:
   0x000000000040054c <+0>:	push   %rbp
   0x000000000040054d <+1>:	mov    %rsp,%rbp
   0x0000000000400550 <+4>:	sub    $0x10,%rsp
=> 0x0000000000400554 <+8>:	movl   $0x0,-0x4(%rbp)
   0x000000000040055b <+15>:	movl   $0x1,-0x8(%rbp)
   0x0000000000400562 <+22>:	mov    0x200ac8(%rip),%esi        # 0x601030 <a>
   0x0000000000400568 <+28>:	mov    -0x8(%rbp),%ecx
   0x000000000040056b <+31>:	mov    -0x4(%rbp),%edx
   0x000000000040056e <+34>:	mov    $0x4006f4,%edi
   0x0000000000400573 <+39>:	mov    $0x0,%eax
   0x0000000000400578 <+44>:	callq  0x4003f8 <printf@plt>
   0x000000000040057d <+49>:	mov    $0x0,%eax
   0x0000000000400582 <+54>:	callq  0x400527 <cool>
   0x0000000000400587 <+59>:	mov    %eax,-0x4(%rbp)
   0x000000000040058a <+62>:	mov    0x200aa0(%rip),%esi        # 0x601030 <a>
   0x0000000000400590 <+68>:	mov    -0x8(%rbp),%ecx
   0x0000000000400593 <+71>:	mov    -0x4(%rbp),%edx
   0x0000000000400596 <+74>:	mov    $0x4006f4,%edi
   0x000000000040059b <+79>:	mov    $0x0,%eax
   0x00000000004005a0 <+84>:	callq  0x4003f8 <printf@plt>
   0x00000000004005a5 <+89>:	addl   $0x1,-0x8(%rbp)
   0x00000000004005a9 <+93>:	mov    0x200a81(%rip),%esi        # 0x601030 <a>
   0x00000000004005af <+99>:	mov    -0x8(%rbp),%ecx
   0x00000000004005b2 <+102>:	mov    -0x4(%rbp),%edx
   0x00000000004005b5 <+105>:	mov    $0x4006f4,%edi
   0x00000000004005ba <+110>:	mov    $0x0,%eax
   0x00000000004005bf <+115>:	callq  0x4003f8 <printf@plt>
   0x00000000004005c4 <+120>:	mov    0x200a66(%rip),%eax        # 0x601030 <a>
   0x00000000004005ca <+126>:	sub    $0x2,%eax
   0x00000000004005cd <+129>:	mov    %eax,0x200a5d(%rip)        # 0x601030 <a>
   0x00000000004005d3 <+135>:	mov    0x200a57(%rip),%esi        # 0x601030 <a>
   0x00000000004005d9 <+141>:	mov    -0x8(%rbp),%ecx
   0x00000000004005dc <+144>:	mov    -0x4(%rbp),%edx
   0x00000000004005df <+147>:	mov    $0x4006f4,%edi
---Type <return> to continue, or q <return> to quit---q
Quit
(gdb) hb *0x000000000040055b
During symbol reading, incomplete CFI data; unspecified registers
(e.g., rax) at 0x400550.
Hardware assisted breakpoint 2 at 0x40055b: file 1.c, line 21.
(gdb) record
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
0x000000000040055c in main () at 1.c:21
21	       int     c = 1;
(gdb) info reg pc
pc: 0x40055c


The stop pc is not right.  Looks we need do some special works when
this is a simple hb.

Thanks,
Hui


On Sun, Nov 22, 2009 at 03:46, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Hi guys,
>>
>> There are some patches about hw breakpoint are running on maillist.
>> If you don't mind, I suggest we wait until they OK.
>
> I don't think there are any conflicts...
>

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

On Sunday 22 November 2009 02:10:23, Hui Zhu wrote:
> I think this patch still not OK.
...
> (gdb) hb *0x000000000040055b
> During symbol reading, incomplete CFI data; unspecified registers
> (e.g., rax) at 0x400550.
> Hardware assisted breakpoint 2 at 0x40055b: file 1.c, line 21.
> (gdb) record
> (gdb) c
> Continuing.
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> 0x000000000040055c in main () at 1.c:21
> 21	       int     c = 1;
> (gdb) info reg pc
> pc: 0x40055c
> 
> The stop pc is not right.  Looks we need do some special works when
> this is a simple hb.

You're confusing hardware watchpoints with hardware breakpoints.
The patch handles hardware _watchpoints_ only.

Fixing hardware breakpoints is a different and unrelated
issue.

Actually, the problem you're seeing sounds simple: The
breakpoint is set at 0x40055b, but GDB shows the SIGTRAP trigger
at 0x40055c.  Off by one, that is.  Completelly untested,
but I bet that something like this fixes it:

record_wait:
		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
						  tmp_pc))
		    {
		      /* There is a breakpoint.  GDB will want to stop.  */
+		  if (software_breakpoint_inserted_here_p (get_regcache_aspace (regcache),
+						  tmp_pc))

		      struct gdbarch *gdbarch = get_regcache_arch (regcache);
		      CORE_ADDR decr_pc_after_break
			= gdbarch_decr_pc_after_break (gdbarch);
		      if (decr_pc_after_break)
			regcache_write_pc (regcache,
					   tmp_pc + decr_pc_after_break);
+	           }
		    }

-- 
Pedro Alves

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

I've applied this slightly updated patch to fix hardware watchpoints.
We were missing a target_stopped_data_address method in precord, plus,
address space matches in the new breakpoint.c method.

Michael, I left out the new NEWS entry, as I didn't see Eli's OK
on that.  I also left out the new tests, as I wasn't sure you
had updated versions of those or not.  I'll let you handle them.

I'll apply a patch to fix hardware breakpoints next.

-- 
Pedro Alves

2009-11-22  Pedro Alves  <pedro@codesourcery.com>
	    Michael Snyder  <msnyder@vmware.com>

        Make hardware watchpoints work for process record.

        * breakpoint.c (hardware_watchpoint_inserted_in_range): New.
	* breakpoint.h (hardware_watchpoint_inserted_in_range): Declare.
	* record.c (record_beneath_to_stopped_by_watchpoint)
	(record_beneath_to_stopped_data_address, record_hw_watchpoint):
	New globals.
	(record_exec_insn): Check for watchpoint hits.
	(tmp_to_stopped_by_watchpoint, tmp_to_stopped_data_address): New
	globals.
	(record_open): Set tmp_to_stopped_by_watchpoint,
	tmp_to_stopped_data_address,
	record_beneath_to_stopped_by_watchpoint and
	record_beneath_to_stopped_data_address.
	(record_wait): Report watchpoint hits to the core.  Update and
	extend comments.
	(record_stopped_by_watchpoint): New.
	(record_stopped_data_address): New.
	(init_record_ops): Install them.
	(init_record_core_ops): Ditto.

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

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2009-11-22 14:48:48.000000000 +0000
+++ src/gdb/breakpoint.c	2009-11-22 15:29:10.000000000 +0000
@@ -2380,6 +2380,38 @@ software_breakpoint_inserted_here_p (str
   return 0;
 }
 
+int
+hardware_watchpoint_inserted_in_range (struct address_space *aspace,
+				       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->pspace->aspace == aspace && loc->inserted)
+	  {
+	    CORE_ADDR l, h;
+
+	    /* Check for intersection.  */
+	    l = max (loc->address, addr);
+	    h = min (loc->address + loc->length, addr + len);
+	    if (l < h)
+	      return 1;
+	  }
+    }
+  return 0;
+}
+
 /* breakpoint_thread_match (PC, PTID) returns true if the breakpoint at
    PC is valid for process/thread PTID.  */
 
Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h	2009-11-22 14:48:48.000000000 +0000
+++ src/gdb/breakpoint.h	2009-11-22 15:02:43.000000000 +0000
@@ -736,6 +736,12 @@ extern int regular_breakpoint_inserted_h
 
 extern int software_breakpoint_inserted_here_p (struct address_space *, CORE_ADDR);
 
+/* 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 (struct address_space *,
+						  CORE_ADDR addr,
+						  ULONGEST len);
+
 extern int breakpoint_thread_match (struct address_space *, CORE_ADDR, ptid_t);
 
 extern void until_break_command (char *, int, int);
Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2009-11-22 14:49:45.000000000 +0000
+++ src/gdb/record.c	2009-11-22 15:26:45.000000000 +0000
@@ -224,6 +224,9 @@ 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);
+static int (*record_beneath_to_stopped_data_address) (struct target_ops *,
+						      CORE_ADDR *);
 
 /* Alloc and free functions for record_reg, record_mem, and record_end 
    entries.  */
@@ -673,6 +676,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 +745,22 @@ 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
+			(get_regcache_aspace (regcache),
+			 entry->u.mem.addr, entry->u.mem.len))
+		      record_hw_watchpoint = 1;
+		  }
               }
           }
       }
@@ -770,6 +791,8 @@ 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 int (*tmp_to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
 
 static void record_restore (void);
 
@@ -894,6 +917,10 @@ 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_stopped_data_address)
+	tmp_to_stopped_data_address = t->to_stopped_data_address;
     }
   if (!tmp_to_xfer_partial)
     error (_("Could not find 'to_xfer_partial' method on the target stack."));
@@ -915,6 +942,8 @@ 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;
+  record_beneath_to_stopped_data_address = tmp_to_stopped_data_address;
 
   if (current_target.to_stratum == core_stratum)
     record_core_open_1 (name, from_tty);
@@ -1069,14 +1098,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 ())
 		    {
-		      /* There is a breakpoint.  GDB will want to stop.  */
+		      /* Always interested in watchpoints.  */
+		    }
+		  else if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
+						       tmp_pc))
+		    {
+		      /* 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 +1124,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 +1152,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 +1256,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 +1305,24 @@ replay_out:
   return inferior_ptid;
 }
 
+static int
+record_stopped_by_watchpoint (void)
+{
+  if (RECORD_IS_REPLAY)
+    return record_hw_watchpoint;
+  else
+    return record_beneath_to_stopped_by_watchpoint ();
+}
+
+static int
+record_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p)
+{
+  if (RECORD_IS_REPLAY)
+    return 0;
+  else
+    return record_beneath_to_stopped_data_address (ops, addr_p);
+}
+
 /* "to_disconnect" method for process record target.  */
 
 static void
@@ -1594,6 +1657,7 @@ init_record_ops (void)
   record_ops.to_xfer_partial = record_xfer_partial;
   record_ops.to_insert_breakpoint = record_insert_breakpoint;
   record_ops.to_remove_breakpoint = record_remove_breakpoint;
+  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_ops.to_can_execute_reverse = record_can_execute_reverse;
   record_ops.to_stratum = record_stratum;
   /* Add bookmark target methods.  */
@@ -1801,6 +1865,7 @@ init_record_core_ops (void)
   record_core_ops.to_xfer_partial = record_core_xfer_partial;
   record_core_ops.to_insert_breakpoint = record_core_insert_breakpoint;
   record_core_ops.to_remove_breakpoint = record_core_remove_breakpoint;
+  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   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;

Re: [RFA] Fix hw watchpoints in process record.

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Sun, 22 Nov 2009 15:44:29 +0000
> Cc: Hui Zhu <teawater@gmail.com>,  Michael Snyder <msnyder@vmware.com>
> 
> Michael, I left out the new NEWS entry, as I didn't see Eli's OK
> on that.

The NEWS entry is okay with me.

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

On Sunday 22 November 2009 15:44:29, Pedro Alves wrote:
> We were missing a target_stopped_data_address method in precord
...

>         * record.c (record_beneath_to_stopped_by_watchpoint)
>         (record_beneath_to_stopped_data_address, record_hw_watchpoint):
>         New globals.
...
>         (record_stopped_by_watchpoint): New.
>         (record_stopped_data_address): New.
>         (init_record_ops): Install them.
>         (init_record_core_ops): Ditto.

"Them", yeah right...  I actually managed to forget to
install record_stopped_data_address in the version I committed.

> @@ -1594,6 +1657,7 @@ init_record_ops (void)
>    record_ops.to_xfer_partial = record_xfer_partial;
>    record_ops.to_insert_breakpoint = record_insert_breakpoint;
>    record_ops.to_remove_breakpoint = record_remove_breakpoint;
> +  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>    record_ops.to_can_execute_reverse = record_can_execute_reverse;
>    record_ops.to_stratum = record_stratum;
>    /* Add bookmark target methods.  */
> @@ -1801,6 +1865,7 @@ init_record_core_ops (void)
>    record_core_ops.to_xfer_partial = record_core_xfer_partial;
>    record_core_ops.to_insert_breakpoint = record_core_insert_breakpoint;
>    record_core_ops.to_remove_breakpoint = record_core_remove_breakpoint;
> +  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>    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;

...

I've applied the patch below to fix it.


Without this, if the target beneath supports reporting the
stopped data address, there's a case where record can miss
a watchpoint.  That is the case of stopping recording when
stopped at a watchpoint, and then continue/step backwards
until a different watchpoint triggers.  The stopp_data_address
of the target beneath is called by mistake, and that may
reports the wrong stopped_data_address, from the last time
the target really ran.  E.g., on x86_64-linux :

 >./gdb -q ./gdb
 (top-gdb) start
 Temporary breakpoint 3 at 0x454727: file ../../src/gdb/gdb.c, line 28.
 Starting program: /home/pedro/gdb/baseline/build/gdb/gdb
 [Thread debugging using libthread_db enabled]
 
 Temporary breakpoint 3, main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:28
 28        memset (&args, 0, sizeof args);
 (top-gdb) record
 (top-gdb) watch args.argv
 Hardware watchpoint 4: args.argv
 (top-gdb) c
 Continuing.
 Hardware watchpoint 4: args.argv
 
 Old value = (char **) 0x0
 New value = (char **) 0x7fffffffe3a8
 main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:31
 31        args.use_windows = 0;
 (top-gdb) del
 Delete all breakpoints? (y or n) y
 (top-gdb) watch args.argc
 Hardware watchpoint 5: args.argc
 (top-gdb) reverse-continue
 Continuing.

 No more reverse-execution history.
 main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:28
 28        memset (&args, 0, sizeof args);
 (top-gdb)  

It should have triggered a watchpoint.  With the
patch applied, the last reverse-continue does this instead:

 (top-gdb) reverse-continue
 Continuing.
 Hardware watchpoint 5: args.argc

 Old value = 1
 New value = 0
 0x000000000045473d in main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:29
 29        args.argc = argc;
 (top-gdb) 

which is correct.


This deserves a testcase, but I haven't written it yet.  Will do
(unless someone else wants to, which I'd appreciate :-) ).

-- 
Pedro Alves

2009-11-24  Pedro Alves  <pedro@codesourcery.com>

	* record.c (init_record_ops, init_record_core_ops): Actually
	install record_stopped_data_address.

---
 gdb/record.c |    2 ++
 1 file changed, 2 insertions(+)

Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2009-11-24 01:36:42.000000000 +0000
+++ src/gdb/record.c	2009-11-24 01:37:01.000000000 +0000
@@ -1668,6 +1668,7 @@ init_record_ops (void)
   record_ops.to_insert_breakpoint = record_insert_breakpoint;
   record_ops.to_remove_breakpoint = record_remove_breakpoint;
   record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
+  record_ops.to_stopped_data_address = record_stopped_data_address;
   record_ops.to_can_execute_reverse = record_can_execute_reverse;
   record_ops.to_stratum = record_stratum;
   /* Add bookmark target methods.  */
@@ -1876,6 +1877,7 @@ init_record_core_ops (void)
   record_core_ops.to_insert_breakpoint = record_core_insert_breakpoint;
   record_core_ops.to_remove_breakpoint = record_core_remove_breakpoint;
   record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
+  record_core_ops.to_stopped_data_address = record_stopped_data_address;
   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;

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

Thanks.

Hui

On Tue, Nov 24, 2009 at 09:58, Pedro Alves <pedro@codesourcery.com> wrote:
> On Sunday 22 November 2009 15:44:29, Pedro Alves wrote:
>> We were missing a target_stopped_data_address method in precord
> ...
>
>>         * record.c (record_beneath_to_stopped_by_watchpoint)
>>         (record_beneath_to_stopped_data_address, record_hw_watchpoint):
>>         New globals.
> ...
>>         (record_stopped_by_watchpoint): New.
>>         (record_stopped_data_address): New.
>>         (init_record_ops): Install them.
>>         (init_record_core_ops): Ditto.
>
> "Them", yeah right...  I actually managed to forget to
> install record_stopped_data_address in the version I committed.
>
>> @@ -1594,6 +1657,7 @@ init_record_ops (void)
>>    record_ops.to_xfer_partial = record_xfer_partial;
>>    record_ops.to_insert_breakpoint = record_insert_breakpoint;
>>    record_ops.to_remove_breakpoint = record_remove_breakpoint;
>> +  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>>    record_ops.to_can_execute_reverse = record_can_execute_reverse;
>>    record_ops.to_stratum = record_stratum;
>>    /* Add bookmark target methods.  */
>> @@ -1801,6 +1865,7 @@ init_record_core_ops (void)
>>    record_core_ops.to_xfer_partial = record_core_xfer_partial;
>>    record_core_ops.to_insert_breakpoint = record_core_insert_breakpoint;
>>    record_core_ops.to_remove_breakpoint = record_core_remove_breakpoint;
>> +  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>>    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;
>
> ...
>
> I've applied the patch below to fix it.
>
>
> Without this, if the target beneath supports reporting the
> stopped data address, there's a case where record can miss
> a watchpoint.  That is the case of stopping recording when
> stopped at a watchpoint, and then continue/step backwards
> until a different watchpoint triggers.  The stopp_data_address
> of the target beneath is called by mistake, and that may
> reports the wrong stopped_data_address, from the last time
> the target really ran.  E.g., on x86_64-linux :
>
>  >./gdb -q ./gdb
>  (top-gdb) start
>  Temporary breakpoint 3 at 0x454727: file ../../src/gdb/gdb.c, line 28.
>  Starting program: /home/pedro/gdb/baseline/build/gdb/gdb
>  [Thread debugging using libthread_db enabled]
>
>  Temporary breakpoint 3, main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:28
>  28        memset (&args, 0, sizeof args);
>  (top-gdb) record
>  (top-gdb) watch args.argv
>  Hardware watchpoint 4: args.argv
>  (top-gdb) c
>  Continuing.
>  Hardware watchpoint 4: args.argv
>
>  Old value = (char **) 0x0
>  New value = (char **) 0x7fffffffe3a8
>  main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:31
>  31        args.use_windows = 0;
>  (top-gdb) del
>  Delete all breakpoints? (y or n) y
>  (top-gdb) watch args.argc
>  Hardware watchpoint 5: args.argc
>  (top-gdb) reverse-continue
>  Continuing.
>
>  No more reverse-execution history.
>  main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:28
>  28        memset (&args, 0, sizeof args);
>  (top-gdb)
>
> It should have triggered a watchpoint.  With the
> patch applied, the last reverse-continue does this instead:
>
>  (top-gdb) reverse-continue
>  Continuing.
>  Hardware watchpoint 5: args.argc
>
>  Old value = 1
>  New value = 0
>  0x000000000045473d in main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:29
>  29        args.argc = argc;
>  (top-gdb)
>
> which is correct.
>
>
> This deserves a testcase, but I haven't written it yet.  Will do
> (unless someone else wants to, which I'd appreciate :-) ).
>
> --
> Pedro Alves
>
> 2009-11-24  Pedro Alves  <pedro@codesourcery.com>
>
>        * record.c (init_record_ops, init_record_core_ops): Actually
>        install record_stopped_data_address.
>
> ---
>  gdb/record.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> Index: src/gdb/record.c
> ===================================================================
> --- src.orig/gdb/record.c       2009-11-24 01:36:42.000000000 +0000
> +++ src/gdb/record.c    2009-11-24 01:37:01.000000000 +0000
> @@ -1668,6 +1668,7 @@ init_record_ops (void)
>   record_ops.to_insert_breakpoint = record_insert_breakpoint;
>   record_ops.to_remove_breakpoint = record_remove_breakpoint;
>   record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
> +  record_ops.to_stopped_data_address = record_stopped_data_address;
>   record_ops.to_can_execute_reverse = record_can_execute_reverse;
>   record_ops.to_stratum = record_stratum;
>   /* Add bookmark target methods.  */
> @@ -1876,6 +1877,7 @@ init_record_core_ops (void)
>   record_core_ops.to_insert_breakpoint = record_core_insert_breakpoint;
>   record_core_ops.to_remove_breakpoint = record_core_remove_breakpoint;
>   record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
> +  record_core_ops.to_stopped_data_address = record_stopped_data_address;
>   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;
>

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

On Sunday 22 November 2009 12:55:25, Pedro Alves wrote:

> Fixing hardware breakpoints is a different and unrelated
> issue.
> 
> Actually, the problem you're seeing sounds simple: The
> breakpoint is set at 0x40055b, but GDB shows the SIGTRAP trigger
> at 0x40055c.  Off by one, that is.  Completelly untested,
> but I bet that something like this fixes it:
> 
> record_wait:
> 		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
> 						  tmp_pc))
> 		    {
> 		      /* There is a breakpoint.  GDB will want to stop.  */
> +		  if (software_breakpoint_inserted_here_p (get_regcache_aspace (regcache),
> +						  tmp_pc))
> 
> 		      struct gdbarch *gdbarch = get_regcache_arch (regcache);
> 		      CORE_ADDR decr_pc_after_break
> 			= gdbarch_decr_pc_after_break (gdbarch);
> 		      if (decr_pc_after_break)
> 			regcache_write_pc (regcache,
> 					   tmp_pc + decr_pc_after_break);
> +	           }
> 		    }
> 

As in the patch below.  Applied.

-- 
Pedro Alves

2009-11-22  Pedro Alves  <pedro@codesourcery.com>

        Make hardware breakpoints work for process repord.

	* record.c (record_wait): Only adjust PC on software breakpoints
	hits.

---
 gdb/record.c |   54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2009-11-22 15:36:49.000000000 +0000
+++ src/gdb/record.c	2009-11-22 15:39:10.000000000 +0000
@@ -1097,6 +1097,7 @@ record_wait (struct target_ops *ops,
 		  && status->value.sig == TARGET_SIGNAL_TRAP)
 		{
 		  struct regcache *regcache;
+		  struct address_space *aspace;
 
 		  /* Yes -- this is likely our single-step finishing,
 		     but check if there's any reason the core would be
@@ -1105,22 +1106,25 @@ record_wait (struct target_ops *ops,
 		  registers_changed ();
 		  regcache = get_current_regcache ();
 		  tmp_pc = regcache_read_pc (regcache);
+		  aspace = get_regcache_aspace (regcache);
 
 		  if (target_stopped_by_watchpoint ())
 		    {
 		      /* Always interested in watchpoints.  */
 		    }
-		  else if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-						       tmp_pc))
+		  else if (breakpoint_inserted_here_p (aspace, tmp_pc))
 		    {
 		      /* 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);
-		      if (decr_pc_after_break)
-			regcache_write_pc (regcache,
-					   tmp_pc + decr_pc_after_break);
+		      if (software_breakpoint_inserted_here_p (aspace, tmp_pc))
+			{
+			  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+			  CORE_ADDR decr_pc_after_break
+			    = gdbarch_decr_pc_after_break (gdbarch);
+			  if (decr_pc_after_break)
+			    regcache_write_pc (regcache,
+					       tmp_pc + decr_pc_after_break);
+			}
 		    }
 		  else
 		    {
@@ -1147,6 +1151,7 @@ record_wait (struct target_ops *ops,
     {
       struct regcache *regcache = get_current_regcache ();
       struct gdbarch *gdbarch = get_regcache_arch (regcache);
+      struct address_space *aspace = get_regcache_aspace (regcache);
       int continue_flag = 1;
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
@@ -1159,18 +1164,20 @@ record_wait (struct target_ops *ops,
       if (execution_direction == EXEC_FORWARD)
 	{
 	  tmp_pc = regcache_read_pc (regcache);
-	  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-					  tmp_pc))
+	  if (breakpoint_inserted_here_p (aspace, tmp_pc))
 	    {
+	      int decr_pc_after_break = gdbarch_decr_pc_after_break (gdbarch);
+
 	      if (record_debug)
 		fprintf_unfiltered (gdb_stdlog,
 				    "Process record: break at %s.\n",
 				    paddress (gdbarch, tmp_pc));
-	      if (gdbarch_decr_pc_after_break (gdbarch)
-		  && !record_resume_step)
+
+	      if (decr_pc_after_break
+		  && !record_resume_step
+		  && software_breakpoint_inserted_here_p (aspace, tmp_pc))
 		regcache_write_pc (regcache,
-				   tmp_pc +
-				   gdbarch_decr_pc_after_break (gdbarch));
+				   tmp_pc + decr_pc_after_break);
 	      goto replay_out;
 	    }
 	}
@@ -1240,28 +1247,31 @@ record_wait (struct target_ops *ops,
 
 		  /* check breakpoint */
 		  tmp_pc = regcache_read_pc (regcache);
-		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-						  tmp_pc))
+		  if (breakpoint_inserted_here_p (aspace, tmp_pc))
 		    {
+		      int decr_pc_after_break
+			= gdbarch_decr_pc_after_break (gdbarch);
+
 		      if (record_debug)
 			fprintf_unfiltered (gdb_stdlog,
 					    "Process record: break "
 					    "at %s.\n",
 					    paddress (gdbarch, tmp_pc));
-		      if (gdbarch_decr_pc_after_break (gdbarch)
+		      if (decr_pc_after_break
 			  && execution_direction == EXEC_FORWARD
-			  && !record_resume_step)
+			  && !record_resume_step
+			  && software_breakpoint_inserted_here_p (aspace,
+								  tmp_pc))
 			regcache_write_pc (regcache,
-					   tmp_pc +
-					   gdbarch_decr_pc_after_break (gdbarch));
+					   tmp_pc + decr_pc_after_break);
 		      continue_flag = 0;
 		    }
 
 		  if (record_hw_watchpoint)
 		    {
 		      if (record_debug)
-			fprintf_unfiltered (gdb_stdlog,
-					    "Process record: hit hw watchpoint.\n");
+			fprintf_unfiltered (gdb_stdlog, "\
+Process record: hit hw watchpoint.\n");
 		      continue_flag = 0;
 		    }
 		  /* Check target signal */

Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder]

Hui Zhu wrote:
> I think this patch still not OK.
> 
> (gdb) start
> Temporary breakpoint 1 at 0x400554: file 1.c, line 20.
> Starting program: /home/teawater/gdb/bgdbno/gdb/a.out
> 
> Temporary breakpoint 1, main () at 1.c:20
> 20	       int     b = 0;
> (gdb) disas
> Dump of assembler code for function main:
>    0x000000000040054c <+0>:	push   %rbp
>    0x000000000040054d <+1>:	mov    %rsp,%rbp
>    0x0000000000400550 <+4>:	sub    $0x10,%rsp
> => 0x0000000000400554 <+8>:	movl   $0x0,-0x4(%rbp)
>    0x000000000040055b <+15>:	movl   $0x1,-0x8(%rbp)
>    0x0000000000400562 <+22>:	mov    0x200ac8(%rip),%esi        # 0x601030 <a>
>    0x0000000000400568 <+28>:	mov    -0x8(%rbp),%ecx
>    0x000000000040056b <+31>:	mov    -0x4(%rbp),%edx
>    0x000000000040056e <+34>:	mov    $0x4006f4,%edi
>    0x0000000000400573 <+39>:	mov    $0x0,%eax
>    0x0000000000400578 <+44>:	callq  0x4003f8 <printf@plt>
>    0x000000000040057d <+49>:	mov    $0x0,%eax
>    0x0000000000400582 <+54>:	callq  0x400527 <cool>
>    0x0000000000400587 <+59>:	mov    %eax,-0x4(%rbp)
>    0x000000000040058a <+62>:	mov    0x200aa0(%rip),%esi        # 0x601030 <a>
>    0x0000000000400590 <+68>:	mov    -0x8(%rbp),%ecx
>    0x0000000000400593 <+71>:	mov    -0x4(%rbp),%edx
>    0x0000000000400596 <+74>:	mov    $0x4006f4,%edi
>    0x000000000040059b <+79>:	mov    $0x0,%eax
>    0x00000000004005a0 <+84>:	callq  0x4003f8 <printf@plt>
>    0x00000000004005a5 <+89>:	addl   $0x1,-0x8(%rbp)
>    0x00000000004005a9 <+93>:	mov    0x200a81(%rip),%esi        # 0x601030 <a>
>    0x00000000004005af <+99>:	mov    -0x8(%rbp),%ecx
>    0x00000000004005b2 <+102>:	mov    -0x4(%rbp),%edx
>    0x00000000004005b5 <+105>:	mov    $0x4006f4,%edi
>    0x00000000004005ba <+110>:	mov    $0x0,%eax
>    0x00000000004005bf <+115>:	callq  0x4003f8 <printf@plt>
>    0x00000000004005c4 <+120>:	mov    0x200a66(%rip),%eax        # 0x601030 <a>
>    0x00000000004005ca <+126>:	sub    $0x2,%eax
>    0x00000000004005cd <+129>:	mov    %eax,0x200a5d(%rip)        # 0x601030 <a>
>    0x00000000004005d3 <+135>:	mov    0x200a57(%rip),%esi        # 0x601030 <a>
>    0x00000000004005d9 <+141>:	mov    -0x8(%rbp),%ecx
>    0x00000000004005dc <+144>:	mov    -0x4(%rbp),%edx
>    0x00000000004005df <+147>:	mov    $0x4006f4,%edi
> ---Type <return> to continue, or q <return> to quit---q
> Quit
> (gdb) hb *0x000000000040055b
> During symbol reading, incomplete CFI data; unspecified registers
> (e.g., rax) at 0x400550.
> Hardware assisted breakpoint 2 at 0x40055b: file 1.c, line 21.
> (gdb) record
> (gdb) c
> Continuing.
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> 0x000000000040055c in main () at 1.c:21
> 21	       int     c = 1;
> (gdb) info reg pc
> pc: 0x40055c
> 
> 
> The stop pc is not right.  Looks we need do some special works when
> this is a simple hb.

Ouch, yeah, that looks like a decr_pc_after_break issue maybe?


> On Sun, Nov 22, 2009 at 03:46, Michael Snyder <msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>> Hi guys,
>>>
>>> There are some patches about hw breakpoint are running on maillist.
>>> If you don't mind, I suggest we wait until they OK.
>> I don't think there are any conflicts...
>>

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

On Sunday 22 November 2009 20:16:40, Michael Snyder wrote:

> > The stop pc is not right.  Looks we need do some special works when
> > this is a simple hb.
> 
> Ouch, yeah, that looks like a decr_pc_after_break issue maybe?

Right, see the rest of the thread, it's fixed already.

-- 
Pedro Alves

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

Cool.  Everything is fine.

Thanks for you guys work on it.

Hui

On Mon, Nov 23, 2009 at 07:02, Pedro Alves <pedro@codesourcery.com> wrote:
>
> On Sunday 22 November 2009 20:16:40, Michael Snyder wrote:
>
> > > The stop pc is not right.  Looks we need do some special works when
> > > this is a simple hb.
> >
> > Ouch, yeah, that looks like a decr_pc_after_break issue maybe?
>
> Right, see the rest of the thread, it's fixed already.
>
> --
> Pedro Alves

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

Still not very well, with the old program:

(gdb) start
Temporary breakpoint 1 at 0x80483c1: file 1.c, line 20.
Starting program: /home/teawater/gdb/a.out
warning: the debug information found in "/lib/ld-2.7.so" does not
match "/lib/ld-linux.so.2" (CRC mismatch).

warning: the debug information found in
"/lib/tls/i686/cmov/libc-2.7.so" does not match
"/lib/tls/i686/cmov/libc.so.6" (CRC mismatch).


Temporary breakpoint 1, main () at 1.c:20
20	       int     b = 0;
(gdb) record
(gdb) n
During symbol reading, incomplete CFI data; unspecified registers
(e.g., eax) at 0x80483be.
21	       int     c = 1;
(gdb)
24		printf ("a = %d b = %d c = %d\n", a, b, c);
(gdb) hw
Undefined command: "hw".  Try "help".
(gdb) hb
Hardware assisted breakpoint 2 at 0x80483cf: file 1.c, line 24.
(gdb) c
Continuing.
a = 0 b = 0 c = 1
a = 3
a = 3 b = 3 c = 1
a = 3 b = 3 c = 2
a = 1 b = 3 c = 2
The next instruction is syscall exit_group.  It will make the program
exit.  Do you want to stop the program?([y] or n)
Process record: inferior program stopped.

Program received signal SIGTRAP, Trace/breakpoint trap.
0xb7fe3405 in __kernel_vsyscall ()
(gdb) rc
Continuing.

Breakpoint 2, main () at 1.c:24
24		printf ("a = %d b = %d c = %d\n", a, b, c);
(gdb) rc
Continuing.

No more reverse-execution history.
main () at 1.c:20
20	       int     b = 0;
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
0x080483d0 in main () at 1.c:24
24		printf ("a = %d b = %d c = %d\n", a, b, c);
(gdb) c
Continuing.

No more reverse-execution history.
0xb7fe3405 in __kernel_vsyscall ()
(gdb) rc
Continuing.

No more reverse-execution history.
main () at 1.c:20
20	       int     b = 0;
(gdb) c
Continuing.

No more reverse-execution history.
0xb7fe3405 in __kernel_vsyscall ()
(gdb) rc
Continuing.

No more reverse-execution history.
main () at 1.c:20
20	       int     b = 0;
(gdb) info b
Num     Type           Disp Enb Address    What
2       hw breakpoint  keep y   0x080483cf in main at 1.c:24
	breakpoint already hit 1 time
(gdb) c
Continuing.

No more reverse-execution history.
0xb7fe3405 in __kernel_vsyscall ()


Thanks,
Hui

On Fri, Nov 6, 2009 at 02:40, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Hi Michael,
>>
>> I do some test with this patch.  But sometime hb didn't work in replay
>> mode.
>
> OK, thanks, I see the problem.
>
> Try this patch instead.
>
>
> 2009-10-31  Michael Snyder  <msnyder@vmware.com>
>        Make hardware watchpoints work for process record.
>        * breakpoint.c (watchpoint_check_1): Abstracted from
> watchpoint_check.
>        (watchpoint_check_2): Check_error entry point for above.
>        (watchpoint_check): Call watchpoint_check_1.
>        (hw_watchpoint_check): New function.  Return true if
>        a hardware watchpoint expression has changed.
>        * breakpoint.h (hw_watchpoint_check): Export.
>
>        * record.c (record_beneath_to_stopped_by_watchpoint): New pointer.
>        (record_open): Initialize above pointer.
>        (record_stopped_by_watchpoint): New target method.
>        (record_wait): Check to see if hitting hardware watchpoint.
>
> Index: gdb/breakpoint.c
> ===================================================================
> --- gdb.orig/breakpoint.c       2009-11-05 09:54:36.000000000 -0800
> +++ gdb/breakpoint.c    2009-11-05 10:19:07.000000000 -0800
> @@ -3075,18 +3075,13 @@
>  #define BP_TEMPFLAG 1
>  #define BP_HARDWAREFLAG 2
>
> -/* Check watchpoint condition.  */
> -
>  static int
> -watchpoint_check (void *p)
> +watchpoint_check_1 (void *p, struct value **new_valp)
>  {
> -  bpstat bs = (bpstat) p;
> -  struct breakpoint *b;
> +  struct breakpoint *b = p;
>   struct frame_info *fr;
>   int within_current_scope;
>
> -  b = bs->breakpoint_at->owner;
> -
>   if (b->exp_valid_block == NULL)
>     within_current_scope = 1;
>   else
> @@ -3137,20 +3132,16 @@
>          we might be in the middle of evaluating a function call.  */
>
>       struct value *mark = value_mark ();
> -      struct value *new_val;
>
> -      fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
> -      if ((b->val != NULL) != (new_val != NULL)
> -         || (b->val != NULL && !value_equal (b->val, new_val)))
> +      fetch_watchpoint_value (b->exp, new_valp, NULL, NULL);
> +      if ((b->val != NULL) != (*new_valp != NULL)
> +         || (b->val != NULL && !value_equal (b->val, *new_valp)))
>        {
> -         if (new_val != NULL)
> +         if (*new_valp != NULL)
>            {
> -             release_value (new_val);
> +             release_value (*new_valp);
>              value_free_to_mark (mark);
>            }
> -         bs->old_val = b->val;
> -         b->val = new_val;
> -         b->val_valid = 1;
>          /* We will stop here */
>          return WP_VALUE_CHANGED;
>        }
> @@ -3181,8 +3172,9 @@
>          (uiout, "reason", async_reason_lookup
> (EXEC_ASYNC_WATCHPOINT_SCOPE));
>       ui_out_text (uiout, "\nWatchpoint ");
>       ui_out_field_int (uiout, "wpnum", b->number);
> -      ui_out_text (uiout, " deleted because the program has left the block
> in\n\
> -which its expression is valid.\n");
> +      ui_out_text (uiout,
> +                  " deleted because the program has left the block in\n\
> +which its expression is valid.\n");
>
>       if (b->related_breakpoint)
>        b->related_breakpoint->disposition = disp_del_at_next_stop;
> @@ -3192,6 +3184,36 @@
>     }
>  }
>
> +static int
> +watchpoint_check_2 (void *p)
> +{
> +  struct value *notused;
> +
> +  return watchpoint_check_1 (p, &notused);
> +}
> +
> +/* Check watchpoint condition.  */
> +
> +static int
> +watchpoint_check (void *p)
> +{
> +  bpstat bs = (bpstat) p;
> +  struct value *new_val;
> +  struct breakpoint *b;
> +  int ret;
> +
> +  b = bs->breakpoint_at->owner;
> +  ret = watchpoint_check_1 (b, &new_val);
> +
> +  if (ret == WP_VALUE_CHANGED)
> +    {
> +      bs->old_val = b->val;
> +      b->val = new_val;
> +      b->val_valid = 1;
> +    }
> +  return ret;
> +}
> +
>  /* Return true if it looks like target has stopped due to hitting
>    breakpoint location BL.  This function does not check if we
>    should stop, only if BL explains the stop.   */
> @@ -3250,6 +3272,29 @@
>   return 1;
>  }
>
> +int
> +hw_watchpoint_check (void)
> +{
> +  struct breakpoint *b;
> +  struct value *new_val;
> +
> +  ALL_BREAKPOINTS (b)
> +    if (b->type == bp_hardware_watchpoint
> +       || b->type == bp_access_watchpoint)
> +      {
> +       char *msg
> +         = xstrprintf (_("Error evaluating expression for watchpoint
> %d\n"),
> +                       b->number);
> +       struct cleanup *cleanups = make_cleanup (xfree, msg);
> +       int e = catch_errors (watchpoint_check_2, b, msg, RETURN_MASK_ALL);
> +       do_cleanups (cleanups);
> +       if (e == WP_VALUE_CHANGED)
> +         return 1;     /* should stop */
> +      }
> +  return 0;            /* don't stop */
> +}
> +
> +
>  /* If BS refers to a watchpoint, determine if the watched values
>    has actually changed, and we should stop.  If not, set BS->stop
>    to 0.  */
> @@ -3267,7 +3312,7 @@
>       CORE_ADDR addr;
>       struct value *v;
>       int must_check_value = 0;
> -
> +
>       if (b->type == bp_watchpoint)
>        /* For a software watchpoint, we must always check the
>           watched value.  */
> @@ -3284,7 +3329,7 @@
>           value.  Access and read watchpoints are out of luck; without
>           a data address, we can't figure it out.  */
>        must_check_value = 1;
> -
> +
>       if (must_check_value)
>        {
>          char *message = xstrprintf ("Error evaluating expression for
> watchpoint %d\n",
> Index: gdb/breakpoint.h
> ===================================================================
> --- gdb.orig/breakpoint.h       2009-11-05 09:54:36.000000000 -0800
> +++ gdb/breakpoint.h    2009-11-05 10:13:18.000000000 -0800
> @@ -978,4 +978,6 @@
>    is newly allocated; the caller should free when done with it.  */
>  extern VEC(breakpoint_p) *all_tracepoints (void);
>
> +extern int hw_watchpoint_check (void);
> +
>  #endif /* !defined (BREAKPOINT_H) */
> Index: gdb/record.c
> ===================================================================
> --- gdb.orig/record.c   2009-11-05 09:54:36.000000000 -0800
> +++ gdb/record.c        2009-11-05 10:24:30.000000000 -0800
> @@ -224,6 +224,7 @@
>                                                   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.  */
> @@ -770,6 +771,7 @@
>                                        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 +896,8 @@
>        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 +919,7 @@
>   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);
> @@ -1010,6 +1015,9 @@
>     record_list = record_list->prev;
>  }
>
> +/* Flag set to TRUE for target_stopped_by_watchpoint.  */
> +static int record_hw_watchpoint = 0;
> +
>  /* "to_wait" target method for process record target.
>
>    In record mode, the target is always run in singlestep mode
> @@ -1069,21 +1077,27 @@
>                {
>                  struct regcache *regcache;
>
> -                 /* Yes -- check if there is a breakpoint.  */
> +                 /* Yes -- check if there is a breakpoint or watchpoint.
>  */
>                  registers_changed ();
>                  regcache = get_current_regcache ();
>                  tmp_pc = regcache_read_pc (regcache);
>                  if (breakpoint_inserted_here_p (get_regcache_aspace
> (regcache),
> -                                                 tmp_pc))
> +                                                 tmp_pc)
> +                     || target_stopped_by_watchpoint ())
>                    {
> -                     /* There is a breakpoint.  GDB will want to stop.  */
> -                     struct gdbarch *gdbarch = get_regcache_arch
> (regcache);
> +                     /* There is a breakpoint, or watchpoint.
> +                        GDB will want to stop.  */
> +                     if (!target_stopped_by_watchpoint ())
> +                       {
> +                         struct gdbarch *gdbarch
> +                           = get_regcache_arch (regcache);
>                      CORE_ADDR decr_pc_after_break
>                        = gdbarch_decr_pc_after_break (gdbarch);
>                      if (decr_pc_after_break)
>                        regcache_write_pc (regcache,
>                                           tmp_pc + decr_pc_after_break);
>                    }
> +                   }
>                  else
>                    {
>                      /* There is not a breakpoint, and gdb is not
> @@ -1116,9 +1130,10 @@
>       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.  */
> +      /* Check for breakpoint or watchpoint when forward execute.  */
>       if (execution_direction == EXEC_FORWARD)
>        {
>          tmp_pc = regcache_read_pc (regcache);
> @@ -1136,6 +1151,15 @@
>                                   gdbarch_decr_pc_after_break (gdbarch));
>              goto replay_out;
>            }
> +         set_executing (inferior_ptid, 0);
> +         if (hw_watchpoint_check ())
> +           {
> +             if (record_debug)
> +               fprintf_unfiltered (gdb_stdlog,
> +                                   "Process record: hit hw watchpoint.\n");
> +             record_hw_watchpoint = 1;
> +           }
> +
>        }
>
>       record_get_sig = 0;
> @@ -1155,6 +1179,7 @@
>         stop.  */
>       do
>        {
> +         set_executing (inferior_ptid, 0);
>          /* Check for beginning and end of log.  */
>          if (execution_direction == EXEC_REVERSE
>              && record_list == &record_first)
> @@ -1219,6 +1244,15 @@
>                                           gdbarch_decr_pc_after_break
> (gdbarch));
>                      continue_flag = 0;
>                    }
> +                 /* check watchpoint */
> +                 if (hw_watchpoint_check ())
> +                   {
> +                     if (record_debug)
> +                       fprintf_unfiltered (gdb_stdlog,
> +                                           "Process record: hit hw
> watchpoint.\n");
> +                     record_hw_watchpoint = 1;
> +                     continue_flag = 0;
> +                   }
>                  /* Check target signal */
>                  if (record_list->u.end.sigval != TARGET_SIGNAL_0)
>                    /* FIXME: better way to check */
> @@ -1238,6 +1272,7 @@
>                  if (record_list->next)
>                    record_list = record_list->next;
>                }
> +             set_executing (inferior_ptid, 1);
>            }
>        }
>       while (continue_flag);
> @@ -1260,6 +1295,16 @@
>   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
> @@ -1599,6 +1644,7 @@
>   /* Add bookmark target methods.  */
>   record_ops.to_get_bookmark = record_get_bookmark;
>   record_ops.to_goto_bookmark = record_goto_bookmark;
> +  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>   record_ops.to_magic = OPS_MAGIC;
>  }
>
> @@ -1807,6 +1853,7 @@
>   /* Add bookmark target methods.  */
>   record_core_ops.to_get_bookmark = record_get_bookmark;
>   record_core_ops.to_goto_bookmark = record_goto_bookmark;
> +  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>   record_core_ops.to_magic = OPS_MAGIC;
>  }
>
> Index: gdb/NEWS
> ===================================================================
> --- gdb.orig/NEWS       2009-11-05 09:54:36.000000000 -0800
> +++ gdb/NEWS    2009-11-05 10:13:18.000000000 -0800
> @@ -88,6 +88,10 @@
>   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: gdb/testsuite/gdb.reverse/watch-reverse.exp
> ===================================================================
> --- gdb.orig/testsuite/gdb.reverse/watch-reverse.exp    2009-11-05
> 09:54:36.000000000 -0800
> +++ gdb/testsuite/gdb.reverse/watch-reverse.exp 2009-11-05
> 10:13:18.000000000 -0800
> @@ -38,8 +38,8 @@
>     # 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" \
>     ".*\[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: gdb/testsuite/gdb.reverse/watch-precsave.exp
> ===================================================================
> --- gdb.orig/testsuite/gdb.reverse/watch-precsave.exp   2009-11-05
> 09:54:36.000000000 -0800
> +++ gdb/testsuite/gdb.reverse/watch-precsave.exp        2009-11-05
> 10:13:18.000000000 -0800
> @@ -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" \
>     ".*\[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"
> +
>
>

Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder]

Hui Zhu wrote:
> Still not very well, with the old program:
> 
> (gdb) start
> Temporary breakpoint 1 at 0x80483c1: file 1.c, line 20.
> Starting program: /home/teawater/gdb/a.out
> warning: the debug information found in "/lib/ld-2.7.so" does not
> match "/lib/ld-linux.so.2" (CRC mismatch).
> 
> warning: the debug information found in
> "/lib/tls/i686/cmov/libc-2.7.so" does not match
> "/lib/tls/i686/cmov/libc.so.6" (CRC mismatch).
> 
> 
> Temporary breakpoint 1, main () at 1.c:20
> 20             int     b = 0;
> (gdb) record
> (gdb) n
> During symbol reading, incomplete CFI data; unspecified registers
> (e.g., eax) at 0x80483be.
> 21             int     c = 1;
> (gdb)
> 24              printf ("a = %d b = %d c = %d\n", a, b, c);
> (gdb) hw
> Undefined command: "hw".  Try "help".
> (gdb) hb
> Hardware assisted breakpoint 2 at 0x80483cf: file 1.c, line 24.
> (gdb) c
> Continuing.
> a = 0 b = 0 c = 1
> a = 3
> a = 3 b = 3 c = 1
> a = 3 b = 3 c = 2
> a = 1 b = 3 c = 2
> The next instruction is syscall exit_group.  It will make the program
> exit.  Do you want to stop the program?([y] or n)
> Process record: inferior program stopped.
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> 0xb7fe3405 in __kernel_vsyscall ()
> (gdb) rc
> Continuing.
> 
> Breakpoint 2, main () at 1.c:24
> 24              printf ("a = %d b = %d c = %d\n", a, b, c);
> (gdb) rc
> Continuing.
> 
> No more reverse-execution history.
> main () at 1.c:20
> 20             int     b = 0;
> (gdb) c
> Continuing.
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> 0x080483d0 in main () at 1.c:24
> 24              printf ("a = %d b = %d c = %d\n", a, b, c);
> (gdb) c
> Continuing.
> 
> No more reverse-execution history.
> 0xb7fe3405 in __kernel_vsyscall ()
> (gdb) rc
> Continuing.
> 
> No more reverse-execution history.
> main () at 1.c:20
> 20             int     b = 0;
> (gdb) c
> Continuing.
> 
> No more reverse-execution history.
> 0xb7fe3405 in __kernel_vsyscall ()
> (gdb) rc
> Continuing.
> 
> No more reverse-execution history.
> main () at 1.c:20
> 20             int     b = 0;
> (gdb) info b
> Num     Type           Disp Enb Address    What
> 2       hw breakpoint  keep y   0x080483cf in main at 1.c:24
>         breakpoint already hit 1 time
> (gdb) c
> Continuing.
> 
> No more reverse-execution history.
> 0xb7fe3405 in __kernel_vsyscall ()
> 
> 
> Thanks,
> Hui

That's odd, I don't get anything like that.
It basically works correctly for me, except for
an unrelated bug that I'm currently looking into.

I'm using RHEL4 and gcc (GCC) 3.4.6 20060404 (Red Hat 3.4.6-11)

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

My os is ubuntu 8.0.4 i386 gcc version 4.2.4.

The following is a simple hw in my pc:
Temporary breakpoint 1, main () at 1.c:20
20	       int     b = 0;
(gdb) hb 31
During symbol reading, incomplete CFI data; unspecified registers
(e.g., eax) at 0x80483be.
Hardware assisted breakpoint 2 at 0x8048447: file 1.c, line 31.
(gdb) set debug infrun 1
(gdb) c
Continuing.
infrun: clear_proceed_status_thread (process 20269)
infrun: proceed (addr=0xffffffff, signal=144, step=0)
infrun: resume (step=0, signal=0), trap_expected=0
infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
a = 0 b = 0 c = 1
a = 3
a = 3 b = 3 c = 1
a = 3 b = 3 c = 2
infrun: target_wait (-1, status) =
infrun:   20269 [process 20269],
infrun:   status->kind = stopped, signal = SIGTRAP
infrun: infwait_normal_state
infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x8048447
infrun: BPSTAT_WHAT_STOP_NOISY
infrun: stop_stepping

Breakpoint 2, main () at 1.c:31
31	       a -= 2;


The following is the hw with prec:
Temporary breakpoint 1, main () at 1.c:20
20	       int     b = 0;
(gdb) record
(gdb) set debug infrun 1
(gdb) hb 31
During symbol reading, incomplete CFI data; unspecified registers
(e.g., eax) at 0x80483be.
Hardware assisted breakpoint 2 at 0x8048447: file 1.c, line 31.
(gdb) c
Continuing.
infrun: clear_proceed_status_thread (process 1842)
infrun: proceed (addr=0xffffffff, signal=144, step=0)
infrun: resume (step=0, signal=0), trap_expected=0
infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
a = 0 b = 0 c = 1
a = 3
a = 3 b = 3 c = 1
a = 3 b = 3 c = 2
infrun: target_wait (-1, status) =
infrun:   1842 [process 1842],
infrun:   status->kind = stopped, signal = SIGTRAP
infrun: infwait_normal_state
infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x8048448
infrun: random signal 5

Program received signal SIGTRAP, Trace/breakpoint trap.
infrun: stop_stepping
0x08048448 in main () at 1.c:31
31	       a -= 2;


Looks like the stop_pc is not right.

Thanks,
Hui


On Tue, Nov 10, 2009 at 05:19, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Still not very well, with the old program:
>>
>> (gdb) start
>> Temporary breakpoint 1 at 0x80483c1: file 1.c, line 20.
>> Starting program: /home/teawater/gdb/a.out
>> warning: the debug information found in "/lib/ld-2.7.so" does not
>> match "/lib/ld-linux.so.2" (CRC mismatch).
>>
>> warning: the debug information found in
>> "/lib/tls/i686/cmov/libc-2.7.so" does not match
>> "/lib/tls/i686/cmov/libc.so.6" (CRC mismatch).
>>
>>
>> Temporary breakpoint 1, main () at 1.c:20
>> 20             int     b = 0;
>> (gdb) record
>> (gdb) n
>> During symbol reading, incomplete CFI data; unspecified registers
>> (e.g., eax) at 0x80483be.
>> 21             int     c = 1;
>> (gdb)
>> 24              printf ("a = %d b = %d c = %d\n", a, b, c);
>> (gdb) hw
>> Undefined command: "hw".  Try "help".
>> (gdb) hb
>> Hardware assisted breakpoint 2 at 0x80483cf: file 1.c, line 24.
>> (gdb) c
>> Continuing.
>> a = 0 b = 0 c = 1
>> a = 3
>> a = 3 b = 3 c = 1
>> a = 3 b = 3 c = 2
>> a = 1 b = 3 c = 2
>> The next instruction is syscall exit_group.  It will make the program
>> exit.  Do you want to stop the program?([y] or n)
>> Process record: inferior program stopped.
>>
>> Program received signal SIGTRAP, Trace/breakpoint trap.
>> 0xb7fe3405 in __kernel_vsyscall ()
>> (gdb) rc
>> Continuing.
>>
>> Breakpoint 2, main () at 1.c:24
>> 24              printf ("a = %d b = %d c = %d\n", a, b, c);
>> (gdb) rc
>> Continuing.
>>
>> No more reverse-execution history.
>> main () at 1.c:20
>> 20             int     b = 0;
>> (gdb) c
>> Continuing.
>>
>> Program received signal SIGTRAP, Trace/breakpoint trap.
>> 0x080483d0 in main () at 1.c:24
>> 24              printf ("a = %d b = %d c = %d\n", a, b, c);
>> (gdb) c
>> Continuing.
>>
>> No more reverse-execution history.
>> 0xb7fe3405 in __kernel_vsyscall ()
>> (gdb) rc
>> Continuing.
>>
>> No more reverse-execution history.
>> main () at 1.c:20
>> 20             int     b = 0;
>> (gdb) c
>> Continuing.
>>
>> No more reverse-execution history.
>> 0xb7fe3405 in __kernel_vsyscall ()
>> (gdb) rc
>> Continuing.
>>
>> No more reverse-execution history.
>> main () at 1.c:20
>> 20             int     b = 0;
>> (gdb) info b
>> Num     Type           Disp Enb Address    What
>> 2       hw breakpoint  keep y   0x080483cf in main at 1.c:24
>>        breakpoint already hit 1 time
>> (gdb) c
>> Continuing.
>>
>> No more reverse-execution history.
>> 0xb7fe3405 in __kernel_vsyscall ()
>>
>>
>> Thanks,
>> Hui
>
> That's odd, I don't get anything like that.
> It basically works correctly for me, except for
> an unrelated bug that I'm currently looking into.
>
> I'm using RHEL4 and gcc (GCC) 3.4.6 20060404 (Red Hat 3.4.6-11)
>
>
>

Re: [RFA] Fix hw watchpoints in process record.

[Tom Tromey]

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:

Michael>  static int
Michael> -watchpoint_check (void *p)
Michael> +watchpoint_check_1 (void *p, struct value **new_valp)

I think the first argument should be of type struct breakpoint *.
All the calls to this function seem to pass in a breakpoint, and AFAICT
there is nothing requiring this code not to use the proper type.

Tom

Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

Tom Tromey wrote:
>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
> 
> Michael>  static int
> Michael> -watchpoint_check (void *p)
> Michael> +watchpoint_check_1 (void *p, struct value **new_valp)
> 
> I think the first argument should be of type struct breakpoint *.
> All the calls to this function seem to pass in a breakpoint, and AFAICT
> there is nothing requiring this code not to use the proper type.

Actually, the call from watchpoint_check_2 passes a void pointer,
but that does not preclude your suggestion.  How's this?


[-- Attachment #2: watchpoint.txt --]
[-- Type: text/plain, Size: 18257 bytes --]

2009-10-31  Michael Snyder  <msnyder@vmware.com>
	Make hardware watchpoints work for process record.
	* breakpoint.c (watchpoint_check_1): Abstracted from watchpoint_check.
	(watchpoint_check_2): Check_error entry point for above.
	(watchpoint_check): Call watchpoint_check_1.
	(hw_watchpoint_check): New function.  Return true if 
	a hardware watchpoint expression has changed.
	* breakpoint.h (hw_watchpoint_check): Export.

	* record.c (record_beneath_to_stopped_by_watchpoint): New pointer.
	(record_open): Initialize above pointer.
	(record_stopped_by_watchpoint): New target method.
	(record_wait): Check to see if hitting hardware watchpoint.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2009-11-10 14:00:16.000000000 -0800
+++ gdb/breakpoint.c	2009-11-10 15:28:36.000000000 -0800
@@ -3075,18 +3075,12 @@
 #define BP_TEMPFLAG 1
 #define BP_HARDWAREFLAG 2
 
-/* Check watchpoint condition.  */
-
 static int
-watchpoint_check (void *p)
+watchpoint_check_1 (struct breakpoint *b, struct value **new_valp)
 {
-  bpstat bs = (bpstat) p;
-  struct breakpoint *b;
   struct frame_info *fr;
   int within_current_scope;
 
-  b = bs->breakpoint_at->owner;
-
   if (b->exp_valid_block == NULL)
     within_current_scope = 1;
   else
@@ -3137,20 +3131,16 @@
          we might be in the middle of evaluating a function call.  */
 
       struct value *mark = value_mark ();
-      struct value *new_val;
 
-      fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
-      if ((b->val != NULL) != (new_val != NULL)
-	  || (b->val != NULL && !value_equal (b->val, new_val)))
+      fetch_watchpoint_value (b->exp, new_valp, NULL, NULL);
+      if ((b->val != NULL) != (*new_valp != NULL)
+	  || (b->val != NULL && !value_equal (b->val, *new_valp)))
 	{
-	  if (new_val != NULL)
+	  if (*new_valp != NULL)
 	    {
-	      release_value (new_val);
+	      release_value (*new_valp);
 	      value_free_to_mark (mark);
 	    }
-	  bs->old_val = b->val;
-	  b->val = new_val;
-	  b->val_valid = 1;
 	  /* We will stop here */
 	  return WP_VALUE_CHANGED;
 	}
@@ -3181,8 +3171,9 @@
 	  (uiout, "reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_SCOPE));
       ui_out_text (uiout, "\nWatchpoint ");
       ui_out_field_int (uiout, "wpnum", b->number);
-      ui_out_text (uiout, " deleted because the program has left the block in\n\
-which its expression is valid.\n");     
+      ui_out_text (uiout,
+		   " deleted because the program has left the block in\n\
+which its expression is valid.\n");
 
       if (b->related_breakpoint)
 	b->related_breakpoint->disposition = disp_del_at_next_stop;
@@ -3192,6 +3183,36 @@
     }
 }
 
+static int
+watchpoint_check_2 (void *p)
+{
+  struct value *notused;
+
+  return watchpoint_check_1 (p, &notused);
+}
+
+/* Check watchpoint condition.  */
+
+static int
+watchpoint_check (void *p)
+{
+  bpstat bs = (bpstat) p;
+  struct value *new_val;
+  struct breakpoint *b;
+  int ret;
+
+  b = bs->breakpoint_at->owner;
+  ret = watchpoint_check_1 (b, &new_val);
+
+  if (ret == WP_VALUE_CHANGED)
+    {
+      bs->old_val = b->val;
+      b->val = new_val;
+      b->val_valid = 1;
+    }
+  return ret;
+}
+
 /* Return true if it looks like target has stopped due to hitting
    breakpoint location BL.  This function does not check if we
    should stop, only if BL explains the stop.   */
@@ -3250,6 +3271,25 @@
   return 1;
 }
 
+int
+hw_watchpoint_check (void)
+{
+  struct breakpoint *b;
+  struct value *new_val;
+  char *msg = _("Error evaluating expression for watchpoint.\n");
+
+  ALL_BREAKPOINTS (b)
+    if (b->type == bp_hardware_watchpoint
+	|| b->type == bp_access_watchpoint)
+      {
+	int e = catch_errors (watchpoint_check_2, b, msg, RETURN_MASK_ALL);
+	if (e == WP_VALUE_CHANGED)
+	  return 1;	/* should stop */
+      }
+  return 0;		/* don't stop */
+}
+
+
 /* If BS refers to a watchpoint, determine if the watched values
    has actually changed, and we should stop.  If not, set BS->stop
    to 0.  */
@@ -3267,7 +3307,7 @@
       CORE_ADDR addr;
       struct value *v;
       int must_check_value = 0;
-      
+
       if (b->type == bp_watchpoint)
 	/* For a software watchpoint, we must always check the
 	   watched value.  */
@@ -3284,7 +3324,7 @@
 	   value.  Access and read watchpoints are out of luck; without
 	   a data address, we can't figure it out.  */
 	must_check_value = 1;
-      
+
       if (must_check_value)
 	{
 	  char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
Index: gdb/breakpoint.h
===================================================================
--- gdb.orig/breakpoint.h	2009-11-10 14:00:08.000000000 -0800
+++ gdb/breakpoint.h	2009-11-10 14:31:18.000000000 -0800
@@ -978,4 +978,6 @@
    is newly allocated; the caller should free when done with it.  */
 extern VEC(breakpoint_p) *all_tracepoints (void);
 
+extern int hw_watchpoint_check (void);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: gdb/record.c
===================================================================
--- gdb.orig/record.c	2009-11-10 14:00:15.000000000 -0800
+++ gdb/record.c	2009-11-10 14:31:18.000000000 -0800
@@ -224,6 +224,7 @@
 						   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.  */
@@ -770,6 +771,7 @@
 					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 +896,8 @@
 	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 +919,7 @@
   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);
@@ -1010,6 +1015,9 @@
     record_list = record_list->prev;
 }
 
+/* Flag set to TRUE for target_stopped_by_watchpoint.  */
+static int record_hw_watchpoint = 0;
+
 /* "to_wait" target method for process record target.
 
    In record mode, the target is always run in singlestep mode
@@ -1069,21 +1077,27 @@
 		{
 		  struct regcache *regcache;
 
-		  /* Yes -- check if there is a breakpoint.  */
+		  /* Yes -- check if there is a breakpoint or watchpoint.  */
 		  registers_changed ();
 		  regcache = get_current_regcache ();
 		  tmp_pc = regcache_read_pc (regcache);
 		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-						  tmp_pc))
+						  tmp_pc)
+		      || target_stopped_by_watchpoint ())
 		    {
-		      /* There is a breakpoint.  GDB will want to stop.  */
-		      struct gdbarch *gdbarch = get_regcache_arch (regcache);
+		      /* There is a breakpoint, or watchpoint.
+			 GDB will want to stop.  */
+		      if (!target_stopped_by_watchpoint ())
+			{
+			  struct gdbarch *gdbarch
+			    = get_regcache_arch (regcache);
 		      CORE_ADDR decr_pc_after_break
 			= gdbarch_decr_pc_after_break (gdbarch);
 		      if (decr_pc_after_break)
 			regcache_write_pc (regcache,
 					   tmp_pc + decr_pc_after_break);
 		    }
+		    }
 		  else
 		    {
 		      /* There is not a breakpoint, and gdb is not
@@ -1116,9 +1130,10 @@
       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.  */
+      /* Check for breakpoint or watchpoint when forward execute.  */
       if (execution_direction == EXEC_FORWARD)
 	{
 	  tmp_pc = regcache_read_pc (regcache);
@@ -1136,6 +1151,15 @@
 				   gdbarch_decr_pc_after_break (gdbarch));
 	      goto replay_out;
 	    }
+	  set_executing (inferior_ptid, 0);
+	  if (hw_watchpoint_check ())
+	    {
+	      if (record_debug)
+		fprintf_unfiltered (gdb_stdlog,
+				    "Process record: hit hw watchpoint.\n");
+	      record_hw_watchpoint = 1;
+	    }
+
 	}
 
       record_get_sig = 0;
@@ -1155,6 +1179,7 @@
 	 stop.  */
       do
 	{
+	  set_executing (inferior_ptid, 0);
 	  /* Check for beginning and end of log.  */
 	  if (execution_direction == EXEC_REVERSE
 	      && record_list == &record_first)
@@ -1219,6 +1244,15 @@
 					   gdbarch_decr_pc_after_break (gdbarch));
 		      continue_flag = 0;
 		    }
+		  /* check watchpoint */
+		  if (hw_watchpoint_check ())
+		    {
+		      if (record_debug)
+			fprintf_unfiltered (gdb_stdlog,
+					    "Process record: hit hw watchpoint.\n");
+		      record_hw_watchpoint = 1;
+		      continue_flag = 0;
+		    }
 		  /* Check target signal */
 		  if (record_list->u.end.sigval != TARGET_SIGNAL_0)
 		    /* FIXME: better way to check */
@@ -1238,6 +1272,7 @@
 		  if (record_list->next)
 		    record_list = record_list->next;
 		}
+	      set_executing (inferior_ptid, 1);
 	    }
 	}
       while (continue_flag);
@@ -1260,6 +1295,16 @@
   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
@@ -1599,6 +1644,7 @@
   /* Add bookmark target methods.  */
   record_ops.to_get_bookmark = record_get_bookmark;
   record_ops.to_goto_bookmark = record_goto_bookmark;
+  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_ops.to_magic = OPS_MAGIC;
 }
 
@@ -1807,6 +1853,7 @@
   /* Add bookmark target methods.  */
   record_core_ops.to_get_bookmark = record_get_bookmark;
   record_core_ops.to_goto_bookmark = record_goto_bookmark;
+  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
   record_core_ops.to_magic = OPS_MAGIC;
 }
 
Index: gdb/NEWS
===================================================================
--- gdb.orig/NEWS	2009-11-10 14:00:16.000000000 -0800
+++ gdb/NEWS	2009-11-10 14:31:18.000000000 -0800
@@ -88,6 +88,10 @@
   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: gdb/testsuite/gdb.reverse/watch-reverse.exp
===================================================================
--- gdb.orig/testsuite/gdb.reverse/watch-reverse.exp	2009-11-10 14:00:08.000000000 -0800
+++ gdb/testsuite/gdb.reverse/watch-reverse.exp	2009-11-10 14:31:18.000000000 -0800
@@ -38,8 +38,8 @@
     # 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" \
     ".*\[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: gdb/testsuite/gdb.reverse/watch-precsave.exp
===================================================================
--- gdb.orig/testsuite/gdb.reverse/watch-precsave.exp	2009-11-10 14:00:08.000000000 -0800
+++ gdb/testsuite/gdb.reverse/watch-precsave.exp	2009-11-10 14:31:18.000000000 -0800
@@ -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" \
     ".*\[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"
+

Re: [RFA] Fix hw watchpoints in process record.

[Hui Zhu]

Sorry Michael,

I was so lazy this week.  Because the weather is cold in Beijing.  :)

 		  if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-						  tmp_pc))
+						  tmp_pc)
+		      || target_stopped_by_watchpoint ())
 		    {
-		      /* There is a breakpoint.  GDB will want to stop.  */
-		      struct gdbarch *gdbarch = get_regcache_arch (regcache);
+		      /* There is a breakpoint, or watchpoint.
+			 GDB will want to stop.  */
+		      if (!target_stopped_by_watchpoint ())
+			{
+			  struct gdbarch *gdbarch
+			    = get_regcache_arch (regcache);
 		      CORE_ADDR decr_pc_after_break
 			= gdbarch_decr_pc_after_break (gdbarch);
 		      if (decr_pc_after_break)
 			regcache_write_pc (regcache,
 					   tmp_pc + decr_pc_after_break);
 		    }
+		    }

I think this part need a little format fix.

I cannot get the hb in record mode because:
i386_stopped_by_watchpoint return 0 to me.  This is really really odd.
I think this is a arch or OS issue.  :(

Could some nice guy help us do some test with it patch?  If this patch
running OK in another PC, Please ignore my trouble, this patch is OK.
It must a fight with OS or something special for me.  :)

Thanks,
Hui

On Wed, Nov 11, 2009 at 07:30, Michael Snyder <msnyder@vmware.com> wrote:
> Tom Tromey wrote:
>>>>>>>
>>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
>>
>> Michael>  static int
>> Michael> -watchpoint_check (void *p)
>> Michael> +watchpoint_check_1 (void *p, struct value **new_valp)
>>
>> I think the first argument should be of type struct breakpoint *.
>> All the calls to this function seem to pass in a breakpoint, and AFAICT
>> there is nothing requiring this code not to use the proper type.
>
> Actually, the call from watchpoint_check_2 passes a void pointer,
> but that does not preclude your suggestion.  How's this?
>
>
> 2009-10-31  Michael Snyder  <msnyder@vmware.com>
>        Make hardware watchpoints work for process record.
>        * breakpoint.c (watchpoint_check_1): Abstracted from
> watchpoint_check.
>        (watchpoint_check_2): Check_error entry point for above.
>        (watchpoint_check): Call watchpoint_check_1.
>        (hw_watchpoint_check): New function.  Return true if
>        a hardware watchpoint expression has changed.
>        * breakpoint.h (hw_watchpoint_check): Export.
>
>        * record.c (record_beneath_to_stopped_by_watchpoint): New pointer.
>        (record_open): Initialize above pointer.
>        (record_stopped_by_watchpoint): New target method.
>        (record_wait): Check to see if hitting hardware watchpoint.
>
> Index: gdb/breakpoint.c
> ===================================================================
> --- gdb.orig/breakpoint.c       2009-11-10 14:00:16.000000000 -0800
> +++ gdb/breakpoint.c    2009-11-10 15:28:36.000000000 -0800
> @@ -3075,18 +3075,12 @@
>  #define BP_TEMPFLAG 1
>  #define BP_HARDWAREFLAG 2
>
> -/* Check watchpoint condition.  */
> -
>  static int
> -watchpoint_check (void *p)
> +watchpoint_check_1 (struct breakpoint *b, struct value **new_valp)
>  {
> -  bpstat bs = (bpstat) p;
> -  struct breakpoint *b;
>   struct frame_info *fr;
>   int within_current_scope;
>
> -  b = bs->breakpoint_at->owner;
> -
>   if (b->exp_valid_block == NULL)
>     within_current_scope = 1;
>   else
> @@ -3137,20 +3131,16 @@
>          we might be in the middle of evaluating a function call.  */
>
>       struct value *mark = value_mark ();
> -      struct value *new_val;
>
> -      fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
> -      if ((b->val != NULL) != (new_val != NULL)
> -         || (b->val != NULL && !value_equal (b->val, new_val)))
> +      fetch_watchpoint_value (b->exp, new_valp, NULL, NULL);
> +      if ((b->val != NULL) != (*new_valp != NULL)
> +         || (b->val != NULL && !value_equal (b->val, *new_valp)))
>        {
> -         if (new_val != NULL)
> +         if (*new_valp != NULL)
>            {
> -             release_value (new_val);
> +             release_value (*new_valp);
>              value_free_to_mark (mark);
>            }
> -         bs->old_val = b->val;
> -         b->val = new_val;
> -         b->val_valid = 1;
>          /* We will stop here */
>          return WP_VALUE_CHANGED;
>        }
> @@ -3181,8 +3171,9 @@
>          (uiout, "reason", async_reason_lookup
> (EXEC_ASYNC_WATCHPOINT_SCOPE));
>       ui_out_text (uiout, "\nWatchpoint ");
>       ui_out_field_int (uiout, "wpnum", b->number);
> -      ui_out_text (uiout, " deleted because the program has left the block
> in\n\
> -which its expression is valid.\n");
> +      ui_out_text (uiout,
> +                  " deleted because the program has left the block in\n\
> +which its expression is valid.\n");
>
>       if (b->related_breakpoint)
>        b->related_breakpoint->disposition = disp_del_at_next_stop;
> @@ -3192,6 +3183,36 @@
>     }
>  }
>
> +static int
> +watchpoint_check_2 (void *p)
> +{
> +  struct value *notused;
> +
> +  return watchpoint_check_1 (p, &notused);
> +}
> +
> +/* Check watchpoint condition.  */
> +
> +static int
> +watchpoint_check (void *p)
> +{
> +  bpstat bs = (bpstat) p;
> +  struct value *new_val;
> +  struct breakpoint *b;
> +  int ret;
> +
> +  b = bs->breakpoint_at->owner;
> +  ret = watchpoint_check_1 (b, &new_val);
> +
> +  if (ret == WP_VALUE_CHANGED)
> +    {
> +      bs->old_val = b->val;
> +      b->val = new_val;
> +      b->val_valid = 1;
> +    }
> +  return ret;
> +}
> +
>  /* Return true if it looks like target has stopped due to hitting
>    breakpoint location BL.  This function does not check if we
>    should stop, only if BL explains the stop.   */
> @@ -3250,6 +3271,25 @@
>   return 1;
>  }
>
> +int
> +hw_watchpoint_check (void)
> +{
> +  struct breakpoint *b;
> +  struct value *new_val;
> +  char *msg = _("Error evaluating expression for watchpoint.\n");
> +
> +  ALL_BREAKPOINTS (b)
> +    if (b->type == bp_hardware_watchpoint
> +       || b->type == bp_access_watchpoint)
> +      {
> +       int e = catch_errors (watchpoint_check_2, b, msg, RETURN_MASK_ALL);
> +       if (e == WP_VALUE_CHANGED)
> +         return 1;     /* should stop */
> +      }
> +  return 0;            /* don't stop */
> +}
> +
> +
>  /* If BS refers to a watchpoint, determine if the watched values
>    has actually changed, and we should stop.  If not, set BS->stop
>    to 0.  */
> @@ -3267,7 +3307,7 @@
>       CORE_ADDR addr;
>       struct value *v;
>       int must_check_value = 0;
> -
> +
>       if (b->type == bp_watchpoint)
>        /* For a software watchpoint, we must always check the
>           watched value.  */
> @@ -3284,7 +3324,7 @@
>           value.  Access and read watchpoints are out of luck; without
>           a data address, we can't figure it out.  */
>        must_check_value = 1;
> -
> +
>       if (must_check_value)
>        {
>          char *message = xstrprintf ("Error evaluating expression for
> watchpoint %d\n",
> Index: gdb/breakpoint.h
> ===================================================================
> --- gdb.orig/breakpoint.h       2009-11-10 14:00:08.000000000 -0800
> +++ gdb/breakpoint.h    2009-11-10 14:31:18.000000000 -0800
> @@ -978,4 +978,6 @@
>    is newly allocated; the caller should free when done with it.  */
>  extern VEC(breakpoint_p) *all_tracepoints (void);
>
> +extern int hw_watchpoint_check (void);
> +
>  #endif /* !defined (BREAKPOINT_H) */
> Index: gdb/record.c
> ===================================================================
> --- gdb.orig/record.c   2009-11-10 14:00:15.000000000 -0800
> +++ gdb/record.c        2009-11-10 14:31:18.000000000 -0800
> @@ -224,6 +224,7 @@
>                                                   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.  */
> @@ -770,6 +771,7 @@
>                                        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 +896,8 @@
>        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 +919,7 @@
>   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);
> @@ -1010,6 +1015,9 @@
>     record_list = record_list->prev;
>  }
>
> +/* Flag set to TRUE for target_stopped_by_watchpoint.  */
> +static int record_hw_watchpoint = 0;
> +
>  /* "to_wait" target method for process record target.
>
>    In record mode, the target is always run in singlestep mode
> @@ -1069,21 +1077,27 @@
>                {
>                  struct regcache *regcache;
>
> -                 /* Yes -- check if there is a breakpoint.  */
> +                 /* Yes -- check if there is a breakpoint or watchpoint.
>  */
>                  registers_changed ();
>                  regcache = get_current_regcache ();
>                  tmp_pc = regcache_read_pc (regcache);
>                  if (breakpoint_inserted_here_p (get_regcache_aspace
> (regcache),
> -                                                 tmp_pc))
> +                                                 tmp_pc)
> +                     || target_stopped_by_watchpoint ())
>                    {
> -                     /* There is a breakpoint.  GDB will want to stop.  */
> -                     struct gdbarch *gdbarch = get_regcache_arch
> (regcache);
> +                     /* There is a breakpoint, or watchpoint.
> +                        GDB will want to stop.  */
> +                     if (!target_stopped_by_watchpoint ())
> +                       {
> +                         struct gdbarch *gdbarch
> +                           = get_regcache_arch (regcache);
>                      CORE_ADDR decr_pc_after_break
>                        = gdbarch_decr_pc_after_break (gdbarch);
>                      if (decr_pc_after_break)
>                        regcache_write_pc (regcache,
>                                           tmp_pc + decr_pc_after_break);
>                    }
> +                   }
>                  else
>                    {
>                      /* There is not a breakpoint, and gdb is not
> @@ -1116,9 +1130,10 @@
>       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.  */
> +      /* Check for breakpoint or watchpoint when forward execute.  */
>       if (execution_direction == EXEC_FORWARD)
>        {
>          tmp_pc = regcache_read_pc (regcache);
> @@ -1136,6 +1151,15 @@
>                                   gdbarch_decr_pc_after_break (gdbarch));
>              goto replay_out;
>            }
> +         set_executing (inferior_ptid, 0);
> +         if (hw_watchpoint_check ())
> +           {
> +             if (record_debug)
> +               fprintf_unfiltered (gdb_stdlog,
> +                                   "Process record: hit hw watchpoint.\n");
> +             record_hw_watchpoint = 1;
> +           }
> +
>        }
>
>       record_get_sig = 0;
> @@ -1155,6 +1179,7 @@
>         stop.  */
>       do
>        {
> +         set_executing (inferior_ptid, 0);
>          /* Check for beginning and end of log.  */
>          if (execution_direction == EXEC_REVERSE
>              && record_list == &record_first)
> @@ -1219,6 +1244,15 @@
>                                           gdbarch_decr_pc_after_break
> (gdbarch));
>                      continue_flag = 0;
>                    }
> +                 /* check watchpoint */
> +                 if (hw_watchpoint_check ())
> +                   {
> +                     if (record_debug)
> +                       fprintf_unfiltered (gdb_stdlog,
> +                                           "Process record: hit hw
> watchpoint.\n");
> +                     record_hw_watchpoint = 1;
> +                     continue_flag = 0;
> +                   }
>                  /* Check target signal */
>                  if (record_list->u.end.sigval != TARGET_SIGNAL_0)
>                    /* FIXME: better way to check */
> @@ -1238,6 +1272,7 @@
>                  if (record_list->next)
>                    record_list = record_list->next;
>                }
> +             set_executing (inferior_ptid, 1);
>            }
>        }
>       while (continue_flag);
> @@ -1260,6 +1295,16 @@
>   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
> @@ -1599,6 +1644,7 @@
>   /* Add bookmark target methods.  */
>   record_ops.to_get_bookmark = record_get_bookmark;
>   record_ops.to_goto_bookmark = record_goto_bookmark;
> +  record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>   record_ops.to_magic = OPS_MAGIC;
>  }
>
> @@ -1807,6 +1853,7 @@
>   /* Add bookmark target methods.  */
>   record_core_ops.to_get_bookmark = record_get_bookmark;
>   record_core_ops.to_goto_bookmark = record_goto_bookmark;
> +  record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
>   record_core_ops.to_magic = OPS_MAGIC;
>  }
>
> Index: gdb/NEWS
> ===================================================================
> --- gdb.orig/NEWS       2009-11-10 14:00:16.000000000 -0800
> +++ gdb/NEWS    2009-11-10 14:31:18.000000000 -0800
> @@ -88,6 +88,10 @@
>   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: gdb/testsuite/gdb.reverse/watch-reverse.exp
> ===================================================================
> --- gdb.orig/testsuite/gdb.reverse/watch-reverse.exp    2009-11-10
> 14:00:08.000000000 -0800
> +++ gdb/testsuite/gdb.reverse/watch-reverse.exp 2009-11-10
> 14:31:18.000000000 -0800
> @@ -38,8 +38,8 @@
>     # 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" \
>     ".*\[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: gdb/testsuite/gdb.reverse/watch-precsave.exp
> ===================================================================
> --- gdb.orig/testsuite/gdb.reverse/watch-precsave.exp   2009-11-10
> 14:00:08.000000000 -0800
> +++ gdb/testsuite/gdb.reverse/watch-precsave.exp        2009-11-10
> 14:31:18.000000000 -0800
> @@ -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" \
>     ".*\[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"
> +
>
>

Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves]

On Wednesday 11 November 2009 08:48:50, Hui Zhu wrote:

> Could some nice guy help us do some test with it patch?  If this patch
> running OK in another PC, Please ignore my trouble, this patch is OK.
> It must a fight with OS or something special for me.  :)

As I said before, I think the design of the patch is wrong ...
It is mixing up the wrong layers.

That said, I'm not sure that accounts for the troubles
you're having.

-- 
Pedro Alves