Re: RFA: implement "watch -location"

[Michael Snyder <msnyder@vmware.com>]

Syntax quibble.  "-location"?  Don't we traditionally use command option
syntax such as "print /x"?  Hate to be inconsistant...

Also, what about help?  Did I miss it?  I couldn't see it at a glance.


Tom Tromey wrote:
> This patch steals an idea from Apple's gdb fork, namely "watch -location".
> I wrote this from scratch but in the end it looks pretty similar to what
> they did.
> 
> In my experience, I generally don't want gdb's scope-tracking logic for
> watchpoint expressions.  Instead, I'm usually just interested in some
> bit of memory.  I find myself typing this a lot:
> 
> print &expr
> watch *$
> 
> This patch adds an option to "watch" to let me do this more directly.
> (FWIW, this is a frequently-asked question on #gdb.)
> 
> I did not add an MI option for this (something Apple did do).
> It didn't seem necessary to me.
> 
> I also didn't add Python API for this.  I'm on the fence about whether
> it is needed.
> 
> A doc review is needed, and of course I'd appreciate other comments.
> 
> Built and regtested on x86-64 (compile farm).
> New tests included.
> 
> Tom
> 
> b/gdb/ChangeLog:
> 2010-08-11  Tom Tromey  <tromey@redhat.com>
> 
> 	* breakpoint.c (watch_command_1): Add 'just_location' argument.
> 	(watch_command_wrapper): Update.
> 	(watch_maybe_just_location): New function.
> 	(watch_command): Update.
> 	(rwatch_command_wrapper): Update.
> 	(rwatch_command): Update.
> 	(awatch_command_wrapper): Update.
> 	(awatch_command): Update.
> 
> b/gdb/doc/ChangeLog:
> 2010-08-11  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.texinfo (Set Watchpoints): Document -location option.
> 
> b/gdb/testsuite/ChangeLog:
> 2010-08-11  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.base/watchpoint.exp (test_watchpoint_and_breakpoint): Delete
> 	watchpoint.
> 	(test_watch_location): New proc.
> 	(test_watchpoint_in_big_blob): Delete watchpoint.
> 	* gdb.base/watchpoint.c (func5): New function.
> 	(main): Call it.
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 4affe0a..11c88b6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -98,8 +98,6 @@ static void clear_command (char *, int);
>  
>  static void catch_command (char *, int);
>  
> -static void watch_command (char *, int);
> -
>  static int can_use_hardware_watchpoint (struct value *);
>  
>  static void break_command_1 (char *, int, int);
> @@ -173,12 +171,6 @@ static void hbreak_command (char *, int);
>  
>  static void thbreak_command (char *, int);
>  
> -static void watch_command_1 (char *, int, int);
> -
> -static void rwatch_command (char *, int);
> -
> -static void awatch_command (char *, int);
> -
>  static void do_enable_breakpoint (struct breakpoint *, enum bpdisp);
>  
>  static void stop_command (char *arg, int from_tty);
> @@ -7981,7 +7973,7 @@ watchpoint_exp_is_const (const struct expression *exp)
>                  hw_read:   watch read, 
>  		hw_access: watch access (read or write) */
>  static void
> -watch_command_1 (char *arg, int accessflag, int from_tty)
> +watch_command_1 (char *arg, int accessflag, int from_tty, int just_location)
>  {
>    struct breakpoint *b, *scope_breakpoint = NULL;
>    struct expression *exp;
> @@ -8086,7 +8078,15 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
>    exp_valid_block = innermost_block;
>    mark = value_mark ();
>    fetch_subexp_value (exp, &pc, &val, NULL, NULL);
> -  if (val != NULL)
> +
> +  if (just_location)
> +    {
> +      exp_valid_block = NULL;
> +      val = value_addr (val);
> +      release_value (val);
> +      value_free_to_mark (mark);
> +    }
> +  else if (val != NULL)
>      release_value (val);
>  
>    tok = arg;
> @@ -8188,7 +8188,24 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
>    b->exp = exp;
>    b->exp_valid_block = exp_valid_block;
>    b->cond_exp_valid_block = cond_exp_valid_block;
> -  b->exp_string = savestring (exp_start, exp_end - exp_start);
> +  if (just_location)
> +    {
> +      struct type *t = value_type (val);
> +      CORE_ADDR addr = value_as_address (val);
> +      char *name;
> +
> +      t = check_typedef (TYPE_TARGET_TYPE (check_typedef (t)));
> +      name = type_to_string (t);
> +
> +      b->exp_string = xstrprintf ("* (%s *) %s", name,
> +				  core_addr_to_string (addr));
> +      xfree (name);
> +
> +      /* The above expression is in C.  */
> +      b->language = language_c;
> +    }
> +  else
> +    b->exp_string = savestring (exp_start, exp_end - exp_start);
>    b->val = val;
>    b->val_valid = 1;
>    if (cond_start)
> @@ -8215,7 +8232,8 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
>        scope_breakpoint->related_breakpoint = b;
>      }
>  
> -  value_free_to_mark (mark);
> +  if (!just_location)
> +    value_free_to_mark (mark);
>  
>    /* Finally update the new watchpoint.  This creates the locations
>       that should be inserted.  */
> @@ -8305,37 +8323,59 @@ can_use_hardware_watchpoint (struct value *v)
>  void
>  watch_command_wrapper (char *arg, int from_tty)
>  {
> -  watch_command (arg, from_tty);
> +  watch_command_1 (arg, hw_write, from_tty, 0);
> +}
> +
> +/* A helper function that looks for the "-location" argument and then
> +   calls watch_command_1.  */
> +
> +static void
> +watch_maybe_just_location (char *arg, int accessflag, int from_tty)
> +{
> +  int just_location = 0;
> +  if (strncmp (arg, "-location", sizeof ("-location") - 1) == 0)
> +    {
> +      arg += sizeof ("-location") - 1;
> +      /* Handle "watch -location" as if there were an expression, and
> +	 let watch_command_1 issue an error.  */
> +      if (*arg == '\0' || isspace (*arg))
> +	{
> +	  ep_skip_leading_whitespace (&arg);
> +	  just_location = 1;
> +	}
> +    }
> +
> +  watch_command_1 (arg, accessflag, from_tty, just_location);
>  }
>  
>  static void
>  watch_command (char *arg, int from_tty)
>  {
> -  watch_command_1 (arg, hw_write, from_tty);
> +  watch_maybe_just_location (arg, hw_write, from_tty);
>  }
>  
>  void
>  rwatch_command_wrapper (char *arg, int from_tty)
>  {
> -  rwatch_command (arg, from_tty);
> +  watch_command_1 (arg, hw_read, from_tty, 0);
>  }
>  
>  static void
>  rwatch_command (char *arg, int from_tty)
>  {
> -  watch_command_1 (arg, hw_read, from_tty);
> +  watch_maybe_just_location (arg, hw_read, from_tty);
>  }
>  
>  void
>  awatch_command_wrapper (char *arg, int from_tty)
>  {
> -  awatch_command (arg, from_tty);
> +  watch_command_1 (arg, hw_access, from_tty, 0);
>  }
>  
>  static void
>  awatch_command (char *arg, int from_tty)
>  {
> -  watch_command_1 (arg, hw_access, from_tty);
> +  watch_maybe_just_location (arg, hw_access, from_tty);
>  }
>  \f
>  
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 7abb9ed..5d2815f 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -3699,7 +3699,7 @@ watchpoints, which do not slow down the running of your program.
>  
>  @table @code
>  @kindex watch
> -@item watch @var{expr} @r{[}thread @var{threadnum}@r{]}
> +@item watch @r{[}-location@r{]} @var{expr} @r{[}thread @var{threadnum}@r{]}
>  Set a watchpoint for an expression.  @value{GDBN} will break when the
>  expression @var{expr} is written into by the program and its value
>  changes.  The simplest (and the most popular) use of this command is
> @@ -3716,13 +3716,20 @@ change the value of @var{expr}, @value{GDBN} will not break.  Note
>  that watchpoints restricted to a single thread in this way only work
>  with Hardware Watchpoints.
>  
> +Ordinarily a watchpoint respects the scope of variables in @var{expr}
> +(see below).  The @code{-location} argument tells @value{GDBN} to
> +instead watch the memory referred to by @var{expr}.  In this case,
> +@value{GDBN} will evaluate @var{expr}, take the address of the result,
> +and watch that memory.  If the expression's result does not have an
> +address, then @value{GDBN} will print an error.
> +
>  @kindex rwatch
> -@item rwatch @var{expr} @r{[}thread @var{threadnum}@r{]}
> +@item rwatch @r{[}-location@r{]} @var{expr} @r{[}thread @var{threadnum}@r{]}
>  Set a watchpoint that will break when the value of @var{expr} is read
>  by the program.
>  
>  @kindex awatch
> -@item awatch @var{expr} @r{[}thread @var{threadnum}@r{]}
> +@item awatch @r{[}-location@r{]} @var{expr} @r{[}thread @var{threadnum}@r{]}
>  Set a watchpoint that will break when @var{expr} is either read from
>  or written into by the program.
>  
> diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
> index 8c212c1..9ef9253 100644
> --- a/gdb/testsuite/gdb.base/watchpoint.c
> +++ b/gdb/testsuite/gdb.base/watchpoint.c
> @@ -126,6 +126,17 @@ func4 ()
>    global_ptr++;
>  }
>  
> +void
> +func5 ()
> +{
> +  int val = 0, val2 = 23;
> +  int *x = &val;
> +
> +  /* func5 breakpoint here */
> +  x = &val2;
> +  val = 27;
> +}
> +
>  int main ()
>  {
>  #ifdef usestubs
> @@ -203,5 +214,7 @@ int main ()
>  
>    func4 ();
>  
> +  func5 ();
> +
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
> index 6029b5b..edc7ea0 100644
> --- a/gdb/testsuite/gdb.base/watchpoint.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint.exp
> @@ -615,6 +615,8 @@ proc test_watchpoint_and_breakpoint {} {
>  		kfail "gdb/38" "next after watch x"
>  	    }
>  	}
> +
> +	gdb_test_no_output "delete \$bpnum" "delete watch x"
>      }
>  }
>  
> @@ -628,6 +630,19 @@ proc test_constant_watchpoint {} {
>      gdb_test_no_output "delete \$bpnum" "delete watchpoint `7 + count'"
>  }
>  
> +proc test_watch_location {} {
> +    gdb_breakpoint [gdb_get_line_number "func5 breakpoint here"]
> +    gdb_continue_to_breakpoint "func5 breakpoint here"
> +
> +    gdb_test "watch -location *x" "atchpoint .*: .*" "watch -location .x"
> +
> +    gdb_test "continue" \
> +	"Continuing.*\[Ww\]atchpoint .*: .*New value = 27.*" \
> +	"continue with watch -location"
> +
> +    gdb_test_no_output "delete \$bpnum" "delete watch -location"
> +}
> +
>  proc test_inaccessible_watchpoint {} {
>      global gdb_prompt
>  
> @@ -678,6 +693,8 @@ proc test_watchpoint_in_big_blob {} {
>  
>      gdb_test "watch buf" ".*atchpoint \[0-9\]+: buf"
>      gdb_test "cont" "Continuing.*atchpoint \[0-9\]+: buf\r\n\r\nOld value = .*testte\".*" "watchpoint on buf hit"
> +
> +    gdb_test_no_output "delete \$bpnum" "delete watch buf"
>  }
>  
>  # Start with a fresh gdb.
> @@ -853,6 +870,8 @@ if [initialize] then {
>      }
>  
>      test_constant_watchpoint
> +
> +    test_watch_location
>  }
>  
>  # Restore old timeout