From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1050 invoked by alias); 10 Nov 2009 23:32:27 -0000 Received: (qmail 1042 invoked by uid 22791); 10 Nov 2009 23:32:25 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 10 Nov 2009 23:32:14 +0000 Received: from mailhost2.vmware.com (mailhost2.vmware.com [10.16.67.167]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 0CE7E139E2; Tue, 10 Nov 2009 15:32:11 -0800 (PST) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by mailhost2.vmware.com (Postfix) with ESMTP id 018FA8E6DA; Tue, 10 Nov 2009 15:32:11 -0800 (PST) Message-ID: <4AF9F7A7.1030804@vmware.com> Date: Tue, 10 Nov 2009 23:32:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20090624) MIME-Version: 1.0 To: "tromey@redhat.com" CC: Hui Zhu , "gdb-patches@sourceware.org" Subject: Re: [RFA] Fix hw watchpoints in process record. References: <4AECE12F.3000704@vmware.com> <4AF31C1F.2000405@vmware.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------020401050004080504070504" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-11/txt/msg00244.txt.bz2 This is a multi-part message in MIME format. --------------020401050004080504070504 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 550 Tom Tromey wrote: >>>>>> "Michael" == Michael Snyder 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? --------------020401050004080504070504 Content-Type: text/plain; name="watchpoint.txt" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="watchpoint.txt" Content-length: 18257 2009-10-31 Michael Snyder 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" + --------------020401050004080504070504--