From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18872 invoked by alias); 11 Aug 2010 22:11:22 -0000 Received: (qmail 18859 invoked by uid 22791); 11 Aug 2010 22:11:19 -0000 X-SWARE-Spam-Status: No, hits=-4.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 11 Aug 2010 22:11:14 +0000 Received: from mailhost3.vmware.com (mailhost3.vmware.com [10.16.27.45]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 9ACA74B061; Wed, 11 Aug 2010 15:11:12 -0700 (PDT) Received: from msnyder-server.eng.vmware.com (promd-2s-dhcp138.eng.vmware.com [10.20.124.138]) by mailhost3.vmware.com (Postfix) with ESMTP id 92BA4CD951; Wed, 11 Aug 2010 15:11:12 -0700 (PDT) Message-ID: <4C632000.2060606@vmware.com> Date: Wed, 11 Aug 2010 22:11:00 -0000 From: Michael Snyder User-Agent: Thunderbird 2.0.0.22 (X11/20090609) MIME-Version: 1.0 To: Tom Tromey CC: "gdb-patches@sourceware.org" Subject: Re: RFA: implement "watch -location" References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-08/txt/msg00163.txt.bz2 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 > > * 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 > > * gdb.texinfo (Set Watchpoints): Document -location option. > > b/gdb/testsuite/ChangeLog: > 2010-08-11 Tom Tromey > > * 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); > } > > > 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