RFA: implement "watch -location"

RFA: implement "watch -location"

[Tom Tromey]

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

Re: RFA: implement "watch -location"

[Michael Snyder]

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

Re: RFA: implement "watch -location"

[Tom Tromey]

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:

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

I picked that because it is what Apple did.

GDB itself is inconsistent.  The "/x" form is used (primarily? by
formats), but the "-" form is used too, e.g. "file -readnow".

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

Oops!  I'll fix this.  Thanks.

Tom

Re: RFA: implement "watch -location"

[Hui Zhu]

On Thu, Aug 12, 2010 at 06:15, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
>
> Michael> Syntax quibble.  "-location"?  Don't we traditionally use
> Michael> command option syntax such as "print /x"?  Hate to be
> Michael> inconsistant...
>
> I picked that because it is what Apple did.
>
> GDB itself is inconsistent.  The "/x" form is used (primarily? by
> formats), but the "-" form is used too, e.g. "file -readnow".
>
> Michael> Also, what about help?  Did I miss it?  I couldn't see it at a glance.
>
> Oops!  I'll fix this.  Thanks.
>
> Tom
>

Hi Tom,

This change is very cool.
I hate:
print &expr
watch *$
It is so uncool.

I am not sure about "-location" or "/x".  But if you choose
"-location", could you add a alias "-l"?

Thanks,
Hui

Re: RFA: implement "watch -location"

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Date: Wed, 11 Aug 2010 16:04:05 -0600
> 
> 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.

Thanks.

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

I suggest a slight rewording here:

   ... take the address of the result, and watch the memory at that
   address.

Please also say something about the size of the memory area watched by
such a watchpoint.

The docs are OK with those changes.

Re: RFA: implement "watch -location"

[André Pönitz]

On Thursday 12 August 2010 00:15:14 ext Tom Tromey wrote:
> >>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
> 
> Michael> Syntax quibble.  "-location"?  Don't we traditionally use
> Michael> command option syntax such as "print /x"?  Hate to be
> Michael> inconsistant...
> 
> I picked that because it is what Apple did.

Thanks for doing so.

From a debugger frontend's point of view it's nasty to have different
options for different versions of gdb requiring different code paths,
some of which can be only tested on specific platforms etc.

Andre'

Re: RFA: implement "watch -location"

[Phil Muldoon]

On 11/08/10 23:04, 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.


Nice patch!

I guess we could get around it by massaging the data in Python before
passing it as an expression to the Breakpoint API.  It would be nice,
however, to maintain parity with the command line.  I'm on the fence
too, but if you don't implement it I probably will (eventually).

Cheers,

Phil

Re: RFA: implement "watch -location"

[Jan Kratochvil]

Hi Tom,

this patch has a regression (guessing due to -lmcheck) on

{x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu for:
 PASS: gdb.base/default.exp: up-silently
-PASS: gdb.base/default.exp: watch
+ERROR: Process no longer exists
+UNRESOLVED: gdb.base/default.exp: watch
[...]

There are some freed VALUEs in use.

While thanks for this killer patch I do not like:

Num     Type           Disp Enb Address    What
1       watchpoint     keep y              gdb_stdin
2       watchpoint     keep y              * (struct ui_file * *) 0x0000000001bf84d0

What does the watchpoint 2 watch?  Is this add-on patch OK?

Num     Type           Disp Enb Address    What
1       watchpoint     keep y              gdb_stdin
2       watchpoint     keep y              -location: gdb_stdin


Thanks,
Jan


gdb/
2010-08-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (update_watchpoint): Add source empty line.  Prefer
	EXP_STRING_REPARSE to EXP_STRING.
	(watch_command_1): Set also EXP_STRING_REPARSE.
	(delete_breakpoint): Free also EXP_STRING_REPARSE.
	* breakpoint.h (struct breakpoint): New field exp_string_reparse.
	Update comment for exp_string.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1339,12 +1339,13 @@ update_watchpoint (struct breakpoint *b, int reparse)
   if (within_current_scope && reparse)
     {
       char *s;
+
       if (b->exp)
 	{
 	  xfree (b->exp);
 	  b->exp = NULL;
 	}
-      s = b->exp_string;
+      s = b->exp_string_reparse ? b->exp_string_reparse : b->exp_string;
       b->exp = parse_exp_1 (&s, b->exp_valid_block, 0);
       /* If the meaning of expression itself changed, the old value is
 	 no longer relevant.  We don't want to report a watchpoint hit
@@ -8211,10 +8212,13 @@ watch_command_1 (char *arg, int accessflag, int from_tty, int just_location)
       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));
+      b->exp_string_reparse = xstrprintf ("* (%s *) %s", name,
+					  core_addr_to_string (addr));
       xfree (name);
 
+      b->exp_string = xstrprintf ("-location: %.*s",
+				  (int) (exp_end - exp_start), exp_start);
+
       /* The above expression is in C.  */
       b->language = language_c;
     }
@@ -9644,6 +9648,7 @@ delete_breakpoint (struct breakpoint *bpt)
   xfree (bpt->addr_string);
   xfree (bpt->exp);
   xfree (bpt->exp_string);
+  xfree (bpt->exp_string_reparse);
   value_free (bpt->val);
   xfree (bpt->source_file);
   xfree (bpt->exec_pathname);
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -450,8 +450,11 @@ struct breakpoint
     /* String form of the breakpoint condition (malloc'd), or NULL if there
        is no condition.  */
     char *cond_string;
-    /* String form of exp (malloc'd), or NULL if none.  */
+    /* String form of exp to use for displaying to the user (malloc'd), or
+       NULL if none.  */
     char *exp_string;
+    /* String form to use for reparsing of EXP (malloc'd) or NULL.  */
+    char *exp_string_reparse;
 
     /* The expression we are watching, or NULL if not a watchpoint.  */
     struct expression *exp;

Re: RFA: implement "watch -location"

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This patch steals an idea from Apple's gdb fork, namely "watch -location".
Tom> I wrote this from scratch but in the end it looks pretty similar to what
Tom> they did.

Here is a new version of the patch.  I have addressed most comments.

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

I kept this as-is.

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

I updated the help and added "usage" lines as well.

Hui> I am not sure about "-location" or "/x".  But if you choose
Hui> "-location", could you add a alias "-l"?

I added this.

Eli> I suggest a slight rewording here:
Eli>    ... take the address of the result, and watch the memory at that
Eli>    address.
Eli> Please also say something about the size of the memory area watched by
Eli> such a watchpoint.

I did these.

Phil> I guess we could get around it by massaging the data in Python before
Phil> passing it as an expression to the Breakpoint API.  It would be nice,
Phil> however, to maintain parity with the command line.  I'm on the fence
Phil> too, but if you don't implement it I probably will (eventually).

I didn't do this.

Jan> this patch has a regression (guessing due to -lmcheck) on

The bug was that watch_maybe_just_location did not check for arg==NULL.
So, a plain "watch" crashed gdb.

Tom

b/gdb/ChangeLog:
2010-08-13  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.
	(check_for_argument): New function.
	(_initialize_breakpoint): Update help text.

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..a58a9bb 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,73 @@ 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 an argument at the start of a
+   string.  The argument must also either be at the end of the string,
+   or be followed by whitespace.  Returns 1 if it finds the argument,
+   0 otherwise.  If the argument is found, it updates *STR.  */
+
+static int
+check_for_argument (char **str, char *arg, int arg_len)
+{
+  if (strncmp (*str, arg, arg_len) == 0
+      && ((*str)[arg_len] == '\0' || isspace ((*str)[arg_len])))
+    {
+      *str += arg_len;
+      return 1;
+    }
+  return 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 (arg
+      && (check_for_argument (&arg, "-location", sizeof ("-location") - 1)
+	  || check_for_argument (&arg, "-l", sizeof ("-l") - 1)))
+    {
+      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
 
@@ -11823,20 +11877,29 @@ With an argument, catch only exceptions with the given name."),
 
   c = add_com ("watch", class_breakpoint, watch_command, _("\
 Set a watchpoint for an expression.\n\
+Usage: watch [-l|-location] EXPRESSION\n\
 A watchpoint stops execution of your program whenever the value of\n\
-an expression changes."));
+an expression changes.\n\
+If -l or -location is given, this evaluates EXPRESSION and watches\n\
+the memory to which it refers."));
   set_cmd_completer (c, expression_completer);
 
   c = add_com ("rwatch", class_breakpoint, rwatch_command, _("\
 Set a read watchpoint for an expression.\n\
+Usage: rwatch [-l|-location] EXPRESSION\n\
 A watchpoint stops execution of your program whenever the value of\n\
-an expression is read."));
+an expression is read.\n\
+If -l or -location is given, this evaluates EXPRESSION and watches\n\
+the memory to which it refers."));
   set_cmd_completer (c, expression_completer);
 
   c = add_com ("awatch", class_breakpoint, awatch_command, _("\
 Set a watchpoint for an expression.\n\
+Usage: awatch [-l|-location] EXPRESSION\n\
 A watchpoint stops execution of your program whenever the value of\n\
-an expression is either read or written."));
+an expression is either read or written.\n\
+If -l or -location is given, this evaluates EXPRESSION and watches\n\
+the memory to which it refers."));
   set_cmd_completer (c, expression_completer);
 
   add_info ("watchpoints", watchpoints_info, _("\
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7abb9ed..d4249c8 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{[}-l@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,22 @@ 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 the memory at that address.  The type of the result is used
+to determine the size of the watched 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{[}-l@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{[}-l@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

Re: RFA: implement "watch -location"

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> 2       watchpoint     keep y              * (struct ui_file * *) 0x0000000001bf84d0

Jan> What does the watchpoint 2 watch?  Is this add-on patch OK?

It seems ok to me, but please wait a short interval to see if there are
other comments.

I'm not going to check my patch in until next week sometime, anyway.

thanks,
Tom

Re: RFA: implement "watch -location"

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 13 Aug 2010 12:25:14 -0600
> 
> Here is a new version of the patch.  I have addressed most comments.

The doco part is fine with me.  Thanks.

Re: RFA: implement "watch -location"

[Jan Kratochvil]

On Fri, 13 Aug 2010 20:25:14 +0200, Tom Tromey wrote:
> Jan> this patch has a regression (guessing due to -lmcheck) on
> 
> The bug was that watch_maybe_just_location did not check for arg==NULL.
> So, a plain "watch" crashed gdb.

There is still a crash:
(gdb) watch -location gdb_stdin
== Invalid read of size 4
==    at 0x6420E3: can_use_hardware_watchpoint (breakpoint.c:8295)
==    by 0x641AE2: watch_command_1 (breakpoint.c:8140)
==    by 0x642342: watch_maybe_just_location (breakpoint.c:8376)
==    by 0x64236C: watch_command (breakpoint.c:8382)
==    by 0x5F0B04: do_cfunc (cli-decode.c:67)
==    by 0x5F3B72: cmd_func (cli-decode.c:1771)
==    by 0x48A9ED: execute_command (top.c:422)
==  Address 0xcee60a0 is 0 bytes inside a block of size 144 free'd
==    at 0x4C25D72: free (vg_replace_malloc.c:325)
==    by 0x48E6D3: xfree (utils.c:1505)
==    by 0x6554CD: value_free (value.c:687)
==    by 0x655503: value_free_to_mark (value.c:701)
==    by 0x641950: watch_command_1 (breakpoint.c:8101)
==    by 0x642342: watch_maybe_just_location (breakpoint.c:8376)
==    by 0x64236C: watch_command (breakpoint.c:8382)
==    by 0x5F0B04: do_cfunc (cli-decode.c:67)
==    by 0x5F3B72: cmd_func (cli-decode.c:1771)
==    by 0x48A9ED: execute_command (top.c:422)

The problem is:
	release_value (val);
	value_free_to_mark (mark);
should also have some:
	val->next = NULL;
as later
	mem_cnt = can_use_hardware_watchpoint (val);
will otherwise jump into the stale value_next pointer.
Just it cannot be implemented this way due to:
	breakpoint.c:8101:10: error: dereferencing pointer to incomplete type

While trying to fix it one could simplify the code as since
	cc7cc38174cfc79e75ea92a00b48591f90c83ea5
	http://sourceware.org/ml/gdb-cvs/2009-11/msg00184.html

the result of can_use_hardware_watchpoint() mostly gets immediately dropped as
it all gets recalculated in update_watchpoint again.  Still
bp_read_watchpoint/bp_access_watchpoint are not checked in update_watchpoint
so one cannot just drop the can_use_hardware_watchpoint call from
watch_command_1.


Thanks,
Jan

Re: RFA: implement "watch -location"

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> There is still a crash:
[... explanation ]

Thanks.  I'll fix this on Monday.

Tom

Re: RFA: implement "watch -location"

[Tom Tromey]

Tom> Thanks.  I'll fix this on Monday.

This version fixes the problem for me.

Built & regtested on x86-64 (compile farm).
I also looked with valgrind.

I am going to check this in soon.

Tom

b/gdb/ChangeLog:
2010-08-16  Tom Tromey  <tromey@redhat.com>

	* value.c (release_value): Clear 'next' pointer.
	* 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.
	(check_for_argument): New function.
	(_initialize_breakpoint): Update help text.

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-16  Tom Tromey  <tromey@redhat.com>

	* gdb.base/help.exp: Update.
	* 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..a58a9bb 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,73 @@ 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 an argument at the start of a
+   string.  The argument must also either be at the end of the string,
+   or be followed by whitespace.  Returns 1 if it finds the argument,
+   0 otherwise.  If the argument is found, it updates *STR.  */
+
+static int
+check_for_argument (char **str, char *arg, int arg_len)
+{
+  if (strncmp (*str, arg, arg_len) == 0
+      && ((*str)[arg_len] == '\0' || isspace ((*str)[arg_len])))
+    {
+      *str += arg_len;
+      return 1;
+    }
+  return 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 (arg
+      && (check_for_argument (&arg, "-location", sizeof ("-location") - 1)
+	  || check_for_argument (&arg, "-l", sizeof ("-l") - 1)))
+    {
+      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
 
@@ -11823,20 +11877,29 @@ With an argument, catch only exceptions with the given name."),
 
   c = add_com ("watch", class_breakpoint, watch_command, _("\
 Set a watchpoint for an expression.\n\
+Usage: watch [-l|-location] EXPRESSION\n\
 A watchpoint stops execution of your program whenever the value of\n\
-an expression changes."));
+an expression changes.\n\
+If -l or -location is given, this evaluates EXPRESSION and watches\n\
+the memory to which it refers."));
   set_cmd_completer (c, expression_completer);
 
   c = add_com ("rwatch", class_breakpoint, rwatch_command, _("\
 Set a read watchpoint for an expression.\n\
+Usage: rwatch [-l|-location] EXPRESSION\n\
 A watchpoint stops execution of your program whenever the value of\n\
-an expression is read."));
+an expression is read.\n\
+If -l or -location is given, this evaluates EXPRESSION and watches\n\
+the memory to which it refers."));
   set_cmd_completer (c, expression_completer);
 
   c = add_com ("awatch", class_breakpoint, awatch_command, _("\
 Set a watchpoint for an expression.\n\
+Usage: awatch [-l|-location] EXPRESSION\n\
 A watchpoint stops execution of your program whenever the value of\n\
-an expression is either read or written."));
+an expression is either read or written.\n\
+If -l or -location is given, this evaluates EXPRESSION and watches\n\
+the memory to which it refers."));
   set_cmd_completer (c, expression_completer);
 
   add_info ("watchpoints", watchpoints_info, _("\
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7abb9ed..d4249c8 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{[}-l@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,22 @@ 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 the memory at that address.  The type of the result is used
+to determine the size of the watched 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{[}-l@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{[}-l@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/help.exp b/gdb/testsuite/gdb.base/help.exp
index 3346f51..6b0777b 100644
--- a/gdb/testsuite/gdb.base/help.exp
+++ b/gdb/testsuite/gdb.base/help.exp
@@ -669,7 +669,7 @@ test_class_help "user-defined" {
     "Use the \"define\" command to define a command\.\[\r\n\]+"
 }
 # test help watch
-gdb_test "help watch" "Set a watchpoint for an expression\.\[\r\n\]+A watchpoint stops execution of your program whenever the value of\[\r\n\]+an expression changes\." "help watch"
+gdb_test "help watch" "Set a watchpoint for an expression\.\[\r\n\]+Usage: watch .-l.-location. EXPRESSION\[\r\n\]+A watchpoint stops execution of your program whenever the value of\[\r\n\]+an expression changes\.\[\r\n\]+If -l or -location is given, this evaluates EXPRESSION and watches\[\r\n\]+the memory to which it refers\." "help watch"
 # test help whatis
 gdb_test "help whatis" "Print data type of expression EXP\." "help whatis"
 # test help where
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
diff --git a/gdb/value.c b/gdb/value.c
index d552402..d938fef 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -747,6 +747,7 @@ release_value (struct value *val)
   if (all_values == val)
     {
       all_values = val->next;
+      val->next = NULL;
       return;
     }
 
@@ -755,6 +756,7 @@ release_value (struct value *val)
       if (v->next == val)
 	{
 	  v->next = val->next;
+	  val->next = NULL;
 	  break;
 	}
     }

Re: RFA: implement "watch -location"

[Jan Kratochvil]

On Mon, 16 Aug 2010 20:12:48 +0200, Tom Tromey wrote:
> This version fixes the problem for me.

I agree.


Thanks,
Jan

Re: RFA: implement "watch -location"

[Jan Kratochvil]

On Fri, 13 Aug 2010 20:27:31 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> 2       watchpoint     keep y              * (struct ui_file * *) 0x0000000001bf84d0
> 
> Jan> What does the watchpoint 2 watch?  Is this add-on patch OK?
> 
> It seems ok to me, but please wait a short interval to see if there are
> other comments.
> 
> I'm not going to check my patch in until next week sometime, anyway.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-08/msg00083.html


Thanks,
Jan