* [RFA] Fix hw watchpoints in process record.
@ 2009-11-01 1:23 Michael Snyder
2009-11-04 3:01 ` Hui Zhu
0 siblings, 1 reply; 28+ messages in thread
From: Michael Snyder @ 2009-11-01 1:23 UTC (permalink / raw)
To: gdb-patches, Hui Zhu
[-- 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, ¬used);
+}
+
+/* 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;
}
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-01 1:23 [RFA] Fix hw watchpoints in process record Michael Snyder
@ 2009-11-04 3:01 ` Hui Zhu
2009-11-04 17:51 ` Michael Snyder
2009-11-05 18:41 ` Michael Snyder
0 siblings, 2 replies; 28+ messages in thread
From: Hui Zhu @ 2009-11-04 3:01 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
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, ¬used);
> +}
> +
> +/* 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;
> }
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-04 3:01 ` Hui Zhu
@ 2009-11-04 17:51 ` Michael Snyder
2009-11-05 1:41 ` Hui Zhu
2009-11-05 18:41 ` Michael Snyder
1 sibling, 1 reply; 28+ messages in thread
From: Michael Snyder @ 2009-11-04 17:51 UTC (permalink / raw)
To: Hui Zhu; +Cc: gdb-patches
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?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-04 17:51 ` Michael Snyder
@ 2009-11-05 1:41 ` Hui Zhu
0 siblings, 0 replies; 28+ messages in thread
From: Hui Zhu @ 2009-11-05 1:41 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
[-- 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);
}
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-04 3:01 ` Hui Zhu
2009-11-04 17:51 ` Michael Snyder
@ 2009-11-05 18:41 ` Michael Snyder
2009-11-05 19:09 ` Pedro Alves
` (2 more replies)
1 sibling, 3 replies; 28+ messages in thread
From: Michael Snyder @ 2009-11-05 18:41 UTC (permalink / raw)
To: Hui Zhu; +Cc: gdb-patches
[-- 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, ¬used);
+}
+
+/* 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"
+
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-05 18:41 ` Michael Snyder
@ 2009-11-05 19:09 ` Pedro Alves
2009-11-12 0:27 ` Pedro Alves
2009-11-09 3:18 ` Hui Zhu
2009-11-09 17:48 ` Tom Tromey
2 siblings, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2009-11-05 19:09 UTC (permalink / raw)
To: gdb-patches; +Cc: Michael Snyder, Hui Zhu
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-05 18:41 ` Michael Snyder
2009-11-05 19:09 ` Pedro Alves
@ 2009-11-09 3:18 ` Hui Zhu
2009-11-09 21:20 ` Michael Snyder
2009-11-09 17:48 ` Tom Tromey
2 siblings, 1 reply; 28+ messages in thread
From: Hui Zhu @ 2009-11-09 3:18 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
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, ¬used);
> +}
> +
> +/* 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"
> +
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-05 18:41 ` Michael Snyder
2009-11-05 19:09 ` Pedro Alves
2009-11-09 3:18 ` Hui Zhu
@ 2009-11-09 17:48 ` Tom Tromey
2009-11-10 23:32 ` Michael Snyder
2 siblings, 1 reply; 28+ messages in thread
From: Tom Tromey @ 2009-11-09 17:48 UTC (permalink / raw)
To: Michael Snyder; +Cc: Hui Zhu, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-09 3:18 ` Hui Zhu
@ 2009-11-09 21:20 ` Michael Snyder
2009-11-10 7:39 ` Hui Zhu
0 siblings, 1 reply; 28+ messages in thread
From: Michael Snyder @ 2009-11-09 21:20 UTC (permalink / raw)
To: Hui Zhu; +Cc: gdb-patches
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)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-09 21:20 ` Michael Snyder
@ 2009-11-10 7:39 ` Hui Zhu
0 siblings, 0 replies; 28+ messages in thread
From: Hui Zhu @ 2009-11-10 7:39 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
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)
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-09 17:48 ` Tom Tromey
@ 2009-11-10 23:32 ` Michael Snyder
2009-11-11 8:49 ` Hui Zhu
0 siblings, 1 reply; 28+ messages in thread
From: Michael Snyder @ 2009-11-10 23:32 UTC (permalink / raw)
To: tromey; +Cc: Hui Zhu, gdb-patches
[-- 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, ¬used);
+}
+
+/* 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"
+
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-10 23:32 ` Michael Snyder
@ 2009-11-11 8:49 ` Hui Zhu
2009-11-12 0:05 ` Pedro Alves
0 siblings, 1 reply; 28+ messages in thread
From: Hui Zhu @ 2009-11-11 8:49 UTC (permalink / raw)
To: Michael Snyder; +Cc: tromey, gdb-patches
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, ¬used);
> +}
> +
> +/* 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"
> +
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-11 8:49 ` Hui Zhu
@ 2009-11-12 0:05 ` Pedro Alves
0 siblings, 0 replies; 28+ messages in thread
From: Pedro Alves @ 2009-11-12 0:05 UTC (permalink / raw)
To: gdb-patches; +Cc: Hui Zhu, Michael Snyder, tromey
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-05 19:09 ` Pedro Alves
@ 2009-11-12 0:27 ` Pedro Alves
2009-11-20 18:11 ` Michael Snyder
0 siblings, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2009-11-12 0:27 UTC (permalink / raw)
To: gdb-patches; +Cc: Michael Snyder, Hui Zhu
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"
+
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-12 0:27 ` Pedro Alves
@ 2009-11-20 18:11 ` Michael Snyder
2009-11-21 9:41 ` Hui Zhu
0 siblings, 1 reply; 28+ messages in thread
From: Michael Snyder @ 2009-11-20 18:11 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, Hui Zhu
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"
> +
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-20 18:11 ` Michael Snyder
@ 2009-11-21 9:41 ` Hui Zhu
2009-11-21 12:05 ` Pedro Alves
2009-11-21 19:50 ` Michael Snyder
0 siblings, 2 replies; 28+ messages in thread
From: Hui Zhu @ 2009-11-21 9:41 UTC (permalink / raw)
To: Michael Snyder, Pedro Alves; +Cc: gdb-patches
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"
>> +
>>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-21 9:41 ` Hui Zhu
@ 2009-11-21 12:05 ` Pedro Alves
2009-11-21 19:50 ` Michael Snyder
1 sibling, 0 replies; 28+ messages in thread
From: Pedro Alves @ 2009-11-21 12:05 UTC (permalink / raw)
To: Hui Zhu; +Cc: Michael Snyder, gdb-patches
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-21 9:41 ` Hui Zhu
2009-11-21 12:05 ` Pedro Alves
@ 2009-11-21 19:50 ` Michael Snyder
2009-11-22 2:11 ` Hui Zhu
1 sibling, 1 reply; 28+ messages in thread
From: Michael Snyder @ 2009-11-21 19:50 UTC (permalink / raw)
To: Hui Zhu; +Cc: Pedro Alves, gdb-patches
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...
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-21 19:50 ` Michael Snyder
@ 2009-11-22 2:11 ` Hui Zhu
2009-11-22 12:56 ` Pedro Alves
2009-11-22 20:21 ` Michael Snyder
0 siblings, 2 replies; 28+ messages in thread
From: Hui Zhu @ 2009-11-22 2:11 UTC (permalink / raw)
To: Michael Snyder, Pedro Alves; +Cc: gdb-patches
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...
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 2:11 ` Hui Zhu
@ 2009-11-22 12:56 ` Pedro Alves
2009-11-22 15:45 ` Pedro Alves
2009-11-22 15:50 ` Pedro Alves
2009-11-22 20:21 ` Michael Snyder
1 sibling, 2 replies; 28+ messages in thread
From: Pedro Alves @ 2009-11-22 12:56 UTC (permalink / raw)
To: Hui Zhu; +Cc: Michael Snyder, gdb-patches
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 12:56 ` Pedro Alves
@ 2009-11-22 15:45 ` Pedro Alves
2009-11-22 19:22 ` Eli Zaretskii
2009-11-24 1:58 ` Pedro Alves
2009-11-22 15:50 ` Pedro Alves
1 sibling, 2 replies; 28+ messages in thread
From: Pedro Alves @ 2009-11-22 15:45 UTC (permalink / raw)
To: gdb-patches; +Cc: Hui Zhu, Michael Snyder
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;
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 12:56 ` Pedro Alves
2009-11-22 15:45 ` Pedro Alves
@ 2009-11-22 15:50 ` Pedro Alves
1 sibling, 0 replies; 28+ messages in thread
From: Pedro Alves @ 2009-11-22 15:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Hui Zhu, Michael Snyder
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 */
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 15:45 ` Pedro Alves
@ 2009-11-22 19:22 ` Eli Zaretskii
2009-11-24 1:58 ` Pedro Alves
1 sibling, 0 replies; 28+ messages in thread
From: Eli Zaretskii @ 2009-11-22 19:22 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, teawater, msnyder
> 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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 2:11 ` Hui Zhu
2009-11-22 12:56 ` Pedro Alves
@ 2009-11-22 20:21 ` Michael Snyder
2009-11-22 23:03 ` Pedro Alves
1 sibling, 1 reply; 28+ messages in thread
From: Michael Snyder @ 2009-11-22 20:21 UTC (permalink / raw)
To: Hui Zhu; +Cc: Pedro Alves, gdb-patches
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...
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 20:21 ` Michael Snyder
@ 2009-11-22 23:03 ` Pedro Alves
2009-11-23 3:17 ` Hui Zhu
0 siblings, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2009-11-22 23:03 UTC (permalink / raw)
To: Michael Snyder; +Cc: Hui Zhu, gdb-patches
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 23:03 ` Pedro Alves
@ 2009-11-23 3:17 ` Hui Zhu
0 siblings, 0 replies; 28+ messages in thread
From: Hui Zhu @ 2009-11-23 3:17 UTC (permalink / raw)
To: Pedro Alves, Michael Snyder; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-22 15:45 ` Pedro Alves
2009-11-22 19:22 ` Eli Zaretskii
@ 2009-11-24 1:58 ` Pedro Alves
2009-11-26 2:28 ` Hui Zhu
1 sibling, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2009-11-24 1:58 UTC (permalink / raw)
To: gdb-patches; +Cc: Hui Zhu, Michael Snyder
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;
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFA] Fix hw watchpoints in process record.
2009-11-24 1:58 ` Pedro Alves
@ 2009-11-26 2:28 ` Hui Zhu
0 siblings, 0 replies; 28+ messages in thread
From: Hui Zhu @ 2009-11-26 2:28 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, Michael Snyder
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;
>
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2009-11-26 2:28 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-01 1:23 [RFA] Fix hw watchpoints in process record Michael Snyder
2009-11-04 3:01 ` Hui Zhu
2009-11-04 17:51 ` Michael Snyder
2009-11-05 1:41 ` Hui Zhu
2009-11-05 18:41 ` Michael Snyder
2009-11-05 19:09 ` Pedro Alves
2009-11-12 0:27 ` Pedro Alves
2009-11-20 18:11 ` Michael Snyder
2009-11-21 9:41 ` Hui Zhu
2009-11-21 12:05 ` Pedro Alves
2009-11-21 19:50 ` Michael Snyder
2009-11-22 2:11 ` Hui Zhu
2009-11-22 12:56 ` Pedro Alves
2009-11-22 15:45 ` Pedro Alves
2009-11-22 19:22 ` Eli Zaretskii
2009-11-24 1:58 ` Pedro Alves
2009-11-26 2:28 ` Hui Zhu
2009-11-22 15:50 ` Pedro Alves
2009-11-22 20:21 ` Michael Snyder
2009-11-22 23:03 ` Pedro Alves
2009-11-23 3:17 ` Hui Zhu
2009-11-09 3:18 ` Hui Zhu
2009-11-09 21:20 ` Michael Snyder
2009-11-10 7:39 ` Hui Zhu
2009-11-09 17:48 ` Tom Tromey
2009-11-10 23:32 ` Michael Snyder
2009-11-11 8:49 ` Hui Zhu
2009-11-12 0:05 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox