Re: [RFA] Fix hw watchpoints in process record.

[Michael Snyder <msnyder@vmware.com>]

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"
> +
>