Re: RFA: implement "watch -location"

[Tom Tromey <tromey at redhat dot com>]

>>>>> "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);
 }
 
 
@@ -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