This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Fix hw watchpoints in process record.
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"
+