From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9818 invoked by alias); 12 Nov 2009 00:27:11 -0000 Received: (qmail 9806 invoked by uid 22791); 12 Nov 2009 00:27:07 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 12 Nov 2009 00:27:01 +0000 Received: (qmail 9861 invoked from network); 12 Nov 2009 00:26:58 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 12 Nov 2009 00:26:58 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA] Fix hw watchpoints in process record. Date: Thu, 12 Nov 2009 00:27:00 -0000 User-Agent: KMail/1.9.10 Cc: Michael Snyder , Hui Zhu References: <4AECE12F.3000704@vmware.com> <4AF31C1F.2000405@vmware.com> <200911051909.40796.pedro@codesourcery.com> In-Reply-To: <200911051909.40796.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911120027.03426.pedro@codesourcery.com> 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/msg00272.txt.bz2 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" +