From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26547 invoked by alias); 20 Nov 2009 18:11:02 -0000 Received: (qmail 26524 invoked by uid 22791); 20 Nov 2009 18:10:59 -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; Fri, 20 Nov 2009 18:09:55 +0000 Received: from jupiter.vmware.com (mailhost5.vmware.com [10.16.68.131]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id ACA4713B5C; Fri, 20 Nov 2009 10:09:53 -0800 (PST) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by jupiter.vmware.com (Postfix) with ESMTP id A26E6DC05D; Fri, 20 Nov 2009 10:09:53 -0800 (PST) Message-ID: <4B06DAB2.7090409@vmware.com> Date: Fri, 20 Nov 2009 18:11:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20090624) MIME-Version: 1.0 To: Pedro Alves CC: "gdb-patches@sourceware.org" , Hui Zhu Subject: Re: [RFA] Fix hw watchpoints in process record. References: <4AECE12F.3000704@vmware.com> <4AF31C1F.2000405@vmware.com> <200911051909.40796.pedro@codesourcery.com> <200911120027.03426.pedro@codesourcery.com> In-Reply-To: <200911120027.03426.pedro@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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/msg00443.txt.bz2 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 > 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 > > > > (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" > + >