From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32619 invoked by alias); 4 Mar 2010 03:50:48 -0000 Received: (qmail 32607 invoked by uid 22791); 4 Mar 2010 03:50:45 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 04 Mar 2010 03:50:39 +0000 Received: (qmail 24424 invoked from network); 4 Mar 2010 03:50:36 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 4 Mar 2010 03:50:36 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Somewhat sanitize watchpoints with conditions on local expressions Date: Thu, 04 Mar 2010 03:50:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-19-generic; KDE/4.3.2; x86_64; ; ) MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201003040350.34417.pedro@codesourcery.com> 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-03/txt/msg00153.txt.bz2 Watchpoints with conditions that involve locals are busted. They never work correctly if the watchpoint triggers in some other function other than where the condition was set. Take this example: (gdb) list 1 1 int globl = 0; 2 3 int 4 func (int *foo) 5 { 6 (*foo)++; 7 globl++; 8 } 9 10 int 11 main (int argc, char **argv) 12 { 13 int val[100]; 14 int q = 0; 15 16 while (1) 17 { 18 func (&q); 19 globl++; 20 } 21 22 return 0; 23 } Let's set a watchpoint on the `globl' global, and when it triggers in `func', we make the watchpoint conditional on `*foo'. When the watchpoint triggers again on line 19, badness happens. See: (gdb) watch globl Hardware watchpoint 2: globl (gdb) c Continuing. Hardware watchpoint 2: globl Old value = 0 New value = 1 func (foo=0x7fffffffe08c) at watch_cond_local.c:8 8 } (gdb) cond *foo > 1000 Bad breakpoint argument: '*foo > 1000' (gdb) c Continuing. Hardware watchpoint 2: globl Old value = 1 New value = 2 main (argc=1, argv=0x7fffffffe178) at watch_cond_local.c:20 20 } (gdb) (gdb) p *foo No symbol "foo" in current context. (gdb) p q $1 = 1 Notice how GDB didn't complain that the condition couldn't be evaluated, yet the watchpoint triggered prematurely anyway. It would be expected for the watchpoint to trigger if the expression failed to evaluate, but that's not what happened. Sometimes it doesn show a testing error, but not always. E.g, I restarted GDB, and reran the exact same commands, and I now got: (gdb) c Continuing. Error in testing breakpoint condition: Cannot access memory at address 0x100000000 Hardware watchpoint 2: globl Old value = 1 New value = 2 main (argc=1, argv=0x7fffffffe178) at watch_cond_local.c:20 20 } (gdb) This time, from the error, the user can infer why did the watchpoint trigger, but it's still lame. What explains why sometimes GDB doesn't show an error, while other times it does, is that, is that when GDB manages to evaluate the expression in the wrong context, it peeks into the wrong random memory. Sometimes this trips unmapped, unreadable memory. Sometimes, it doesn't. The root issue, is that when GDB parses the condition expression for '*foo > 1000', at the time the watchpoint was created, it stores in the expression a reference to the symbol for `foo'. This symbol though, only makes sense to evaluate in the context of a frame that's executing `func'. The debug info for `foo' tells GDB to look for FOO at some offset from a frame running `func', and that obviously doesn't work correctly when GDB goes apply that debug info to the `main'. Depending on the phase of the moon, evaluating that condition on the wrong frame either gives a false positive or a false negative. Sometimes it throws an error. IMO, GBB could be smarted, and check if it makes sense to evaluate the condition in the current frame before actualy trying, and stop if it doesn't, with an short blurb: (gdb) c Continuing. warning: Watchpoint condition can't tested in the current scope ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Hardware watchpoint 2: globl Old value = 1 New value = 2 main (argc=1, argv=0x7fffffffe178) at watch_cond_local.c:20 20 } (gdb) This would avoid a dependency on the current lunar phase, which I think would be good. Here's another example where GDB could be smarter, but is completely broken today, using the same code as above, but doing things the other way around. This time, setting a watchpoint with a condition that only makes sense in `main', and see what happens when a function inner to `main' changes the watched memory. As you can see in the sources at the top of this letter, `q' is a local of `main'. Restart GDB, run to main, and do: (gdb) watch q if q > 10 Hardware watchpoint 2: q (gdb) c Continuing. Hardware watchpoint 2: q Old value = 0 New value = 1 func (foo=0x7fffffffe08c) at watch_cond_local.c:7 7 globl++; (gdb) I would have expected that the watchpoint would only trigger if q in `main' was > 10. Why doesn't GDB realize that there's still a frame active in `main', and that the condition could be evaluated there? Again, notice that no error was thrown, but the watchpoint still triggered prematurely. Remember that for watchpoints on local expressions, GDB switches to the frame where the watchpoint was set to evaluate the new value of the expression. But, it doesn't do this for the _condition_ expression evaluation. That's lame. Indeed, a closer look revealed that GDB did manage to evaluate the watchpoint's condition, but since the watchpoint triggered in a frame where the symbol for `q' doesn't make sense, it evaluated gawd-knows-what. IMO, the most intuitive behaviour, in this case, one that almost doesn't need explaning would be, for example: (gdb) start (...) Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe178) at watch_cond_local.c:14 14 int q = 0; (gdb) watch q if q > 10 Hardware watchpoint 2: q (gdb) c Continuing. Hardware watchpoint 2: q Old value = 10 New value = 11 func (foo=0x7fffffffe08c) at watch_cond_local.c:7 7 globl++; (gdb) As conditions with local variables don't usually work as one would expect, most people use something like this instead: (gdb) p &local (gdb) watch foo if *$ > 10 The patch below that implements what sounded least surprising to me (evaluate condition in correct frame if possible, and warn if not possible, without trying and doing undefined things), and adds tests for this, and other variants involving global expressions, and local conditions. Comments? 2010-03-04 Pedro Alves * breakpoint.c (condition_command): Handle watchpoint conditions. (is_watchpoint): New. (update_watchpoint): Don't reparse the watchpoint's condition unless necessary. (WP_IGNORE): New. (watchpoint_check): Use it. (bpstat_check_watchpoint): Handle it. (bpstat_check_breakpoint_conditions): 2010-03-04 Pedro Alves * gdb.base/watch-cond.c, gdb.base/watch-cond.exp: New. -- Pedro Alves --- gdb/breakpoint.c | 173 +++++++++++++++++++++++++++++++++++++++---------------- gdb/breakpoint.h | 17 +++-- 2 files changed, 136 insertions(+), 54 deletions(-) Index: src/gdb/breakpoint.c =================================================================== --- src.orig/gdb/breakpoint.c 2010-03-04 02:25:27.000000000 +0000 +++ src/gdb/breakpoint.c 2010-03-04 03:36:17.000000000 +0000 @@ -210,6 +210,8 @@ static void update_global_location_list_ static int is_hardware_watchpoint (struct breakpoint *bpt); +static int is_watchpoint (struct breakpoint *bpt); + static void insert_breakpoint_locations (void); static int syscall_catchpoint_p (struct breakpoint *b); @@ -640,18 +642,16 @@ condition_command (char *arg, int from_t struct bp_location *loc = b->loc; for (; loc; loc = loc->next) { - if (loc->cond) - { - xfree (loc->cond); - loc->cond = 0; - } + xfree (loc->cond); + loc->cond = NULL; } - if (b->cond_string != NULL) - xfree (b->cond_string); + xfree (b->cond_string); + b->cond_string = NULL; + xfree (b->cond_exp); + b->cond_exp = NULL; if (*p == 0) { - b->cond_string = NULL; if (from_tty) printf_filtered (_("Breakpoint %d now unconditional.\n"), bnum); } @@ -662,13 +662,26 @@ condition_command (char *arg, int from_t typed in or the decompiled expression. */ b->cond_string = xstrdup (arg); b->condition_not_parsed = 0; - for (loc = b->loc; loc; loc = loc->next) + + if (is_watchpoint (b)) { + innermost_block = NULL; arg = p; - loc->cond = - parse_exp_1 (&arg, block_for_pc (loc->address), 0); + b->cond_exp = parse_exp_1 (&arg, 0, 0); if (*arg) error (_("Junk at end of expression")); + b->cond_exp_valid_block = innermost_block; + } + else + { + for (loc = b->loc; loc; loc = loc->next) + { + arg = p; + loc->cond = + parse_exp_1 (&arg, block_for_pc (loc->address), 0); + if (*arg) + error (_("Junk at end of expression")); + } } } breakpoints_changed (); @@ -916,6 +929,13 @@ is_hardware_watchpoint (struct breakpoin || bpt->type == bp_access_watchpoint); } +static int +is_watchpoint (struct breakpoint *bpt) +{ + return (is_hardware_watchpoint (bpt) + || bpt->type == bp_watchpoint); +} + /* Find the current value of a watchpoint on EXP. Return the value in *VALP and *RESULTP and the chain of intermediate and final values in *VAL_CHAIN. RESULTP and VAL_CHAIN may be NULL if the caller does @@ -1237,14 +1257,13 @@ update_watchpoint (struct breakpoint *b, value_free (v); } - /* We just regenerated the list of breakpoint locations. - The new location does not have its condition field set to anything - and therefore, we must always reparse the cond_string, independently - of the value of the reparse flag. */ - if (b->cond_string != NULL) + /* Note that unlike with breakpoints, the watchpoint's condition + expression is stored in the breakpoint object, not in the + locations we just (re)created. */ + if (reparse && b->cond_string != NULL) { char *s = b->cond_string; - b->loc->cond = parse_exp_1 (&s, b->exp_valid_block, 0); + b->cond_exp = parse_exp_1 (&s, b->cond_exp_valid_block, 0); } } else if (!within_current_scope) @@ -1254,7 +1273,11 @@ Watchpoint %d deleted because the progra in which its expression is valid.\n"), b->number); if (b->related_breakpoint) - b->related_breakpoint->disposition = disp_del_at_next_stop; + { + b->related_breakpoint->disposition = disp_del_at_next_stop; + b->related_breakpoint->related_breakpoint = NULL; + b->related_breakpoint= NULL; + } b->disposition = disp_del_at_next_stop; } @@ -3238,6 +3261,8 @@ watchpoints_triggered (struct target_wai #define WP_VALUE_CHANGED 2 /* The value has not changed. */ #define WP_VALUE_NOT_CHANGED 3 +/* Ignore this watchpoint, no matter if the value changed or not. */ +#define WP_IGNORE 4 #define BP_TEMPFLAG 1 #define BP_HARDWAREFLAG 2 @@ -3261,7 +3286,7 @@ watchpoint_check (void *p) watchpoint frame is in scope if the current thread is the thread that was used to create the watchpoint. */ if (!watchpoint_in_thread_scope (b)) - return WP_VALUE_NOT_CHANGED; + return WP_IGNORE; if (b->exp_valid_block == NULL) within_current_scope = 1; @@ -3280,7 +3305,7 @@ watchpoint_check (void *p) even if they are in some other frame, our view of the stack is likely to be wrong and frame_find_by_id could error out. */ if (gdbarch_in_function_epilogue_p (frame_arch, frame_pc)) - return WP_VALUE_NOT_CHANGED; + return WP_IGNORE; fr = frame_find_by_id (b->watchpoint_frame); within_current_scope = (fr != NULL); @@ -3331,14 +3356,12 @@ watchpoint_check (void *p) bs->old_val = b->val; b->val = new_val; b->val_valid = 1; - /* We will stop here */ return WP_VALUE_CHANGED; } else { - /* Nothing changed, don't do anything. */ + /* Nothing changed. */ value_free_to_mark (mark); - /* We won't stop here */ return WP_VALUE_NOT_CHANGED; } } @@ -3365,7 +3388,11 @@ watchpoint_check (void *p) which its expression is valid.\n"); if (b->related_breakpoint) - b->related_breakpoint->disposition = disp_del_at_next_stop; + { + b->related_breakpoint->disposition = disp_del_at_next_stop; + b->related_breakpoint->related_breakpoint = NULL; + b->related_breakpoint = NULL; + } b->disposition = disp_del_at_next_stop; return WP_DELETED; @@ -3485,6 +3512,10 @@ bpstat_check_watchpoint (bpstat bs) bs->print_it = print_it_done; /* Stop. */ break; + case WP_IGNORE: + bs->print_it = print_it_noop; + bs->stop = 0; + break; case WP_VALUE_CHANGED: if (b->type == bp_read_watchpoint) { @@ -3604,16 +3635,24 @@ bpstat_check_breakpoint_conditions (bpst else if (bs->stop) { int value_is_zero = 0; - + struct expression *cond; + /* If this is a scope breakpoint, mark the associated watchpoint as triggered so that we will handle the out-of-scope event. We'll get to the watchpoint next iteration. */ if (b->type == bp_watchpoint_scope) b->related_breakpoint->watchpoint_triggered = watch_triggered_yes; - - if (bl->cond && bl->owner->disposition != disp_del_at_next_stop) + + if (is_watchpoint (b)) + cond = b->cond_exp; + else + cond = bl->cond; + + if (cond && bl->owner->disposition != disp_del_at_next_stop) { + int within_current_scope = 1; + /* We use value_mark and value_free_to_mark because it could be a long time before we return to the command level and call free_all_values. We can't call free_all_values @@ -3627,15 +3666,50 @@ bpstat_check_breakpoint_conditions (bpst variables when we arrive at a breakpoint at the start of the inlined function; the current frame will be the call site. */ - select_frame (get_current_frame ()); - value_is_zero - = catch_errors (breakpoint_cond_eval, (bl->cond), - "Error in testing breakpoint condition:\n", - RETURN_MASK_ALL); + if (!is_watchpoint (b) || b->cond_exp_valid_block == NULL) + select_frame (get_current_frame ()); + else + { + struct frame_info *frame; + + /* For local watchpoint expressions, which particular + instance of a local is being watched matters, so we + keep track of the frame to evaluate the expression + in. To evaluate the condition however, it doesn't + really matter which instantiation of the function + where the condition makes sense triggers the + watchpoint. This allows an expression like "watch + global if q > 10" set in `func', catch writes to + global on all threads that call `func', or catch + writes on all recursive calls of `func' by a single + thread. We simple always evaluate the condition in + the innermost frame that's executing where it makes + sense to evaluate the condition. It seems + intuitive. */ + frame = block_innermost_frame (b->cond_exp_valid_block); + if (frame != NULL) + select_frame (frame); + else + within_current_scope = 0; + } + if (within_current_scope) + value_is_zero + = catch_errors (breakpoint_cond_eval, cond, + "Error in testing breakpoint condition:\n", + RETURN_MASK_ALL); + else + { + warning (_("Watchpoint condition can't tested " + "in the current scope")); + /* If we failed to set the right context for this + watchpoint, unconditionally report it. */ + value_is_zero = 0; + } /* FIXME-someday, should give breakpoint # */ value_free_to_mark (mark); } - if (bl->cond && value_is_zero) + + if (cond && value_is_zero) { bs->stop = 0; } @@ -7283,7 +7357,7 @@ watch_command_1 (char *arg, int accessfl struct gdbarch *gdbarch = get_current_arch (); struct breakpoint *b, *scope_breakpoint = NULL; struct expression *exp; - struct block *exp_valid_block; + struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL; struct value *val, *mark; struct frame_info *frame; char *exp_start = NULL; @@ -7388,8 +7462,14 @@ watch_command_1 (char *arg, int accessfl { struct expression *cond; + innermost_block = NULL; tok = cond_start = end_tok + 1; cond = parse_exp_1 (&tok, 0, 0); + + /* The watchpoint expression may not be local, but the condition + may still be. E.g.: `watch global if local > 0'. */ + cond_exp_valid_block = innermost_block; + xfree (cond); cond_end = tok; } @@ -7430,7 +7510,7 @@ watch_command_1 (char *arg, int accessfl breakpoint at the point where we've left the scope of the watchpoint expression. Create the scope breakpoint before the watchpoint, so that we will encounter it first in bpstat_stop_status. */ - if (innermost_block && frame) + if (exp_valid_block && frame) { if (frame_id_p (frame_unwind_caller_id (frame))) { @@ -7467,6 +7547,7 @@ watch_command_1 (char *arg, int accessfl b->disposition = disp_donttouch; 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); b->val = val; b->val_valid = 1; @@ -8842,20 +8923,14 @@ delete_breakpoint (struct breakpoint *bp } free_command_lines (&bpt->commands); - if (bpt->cond_string != NULL) - xfree (bpt->cond_string); - if (bpt->addr_string != NULL) - xfree (bpt->addr_string); - if (bpt->exp != NULL) - xfree (bpt->exp); - if (bpt->exp_string != NULL) - xfree (bpt->exp_string); - if (bpt->val != NULL) - value_free (bpt->val); - if (bpt->source_file != NULL) - xfree (bpt->source_file); - if (bpt->exec_pathname != NULL) - xfree (bpt->exec_pathname); + xfree (bpt->cond_string); + xfree (bpt->cond_exp); + xfree (bpt->addr_string); + xfree (bpt->exp); + xfree (bpt->exp_string); + value_free (bpt->val); + xfree (bpt->source_file); + xfree (bpt->exec_pathname); clean_up_filters (&bpt->syscalls_to_be_caught); /* Be sure no bpstat's are pointing at it after it's been freed. */ Index: src/gdb/breakpoint.h =================================================================== --- src.orig/gdb/breakpoint.h 2010-03-04 02:25:27.000000000 +0000 +++ src/gdb/breakpoint.h 2010-03-04 02:57:38.000000000 +0000 @@ -240,11 +240,13 @@ struct bp_location than reference counting. */ struct breakpoint *owner; - /* Conditional. Break only if this expression's value is nonzero. - Unlike string form of condition, which is associated with breakpoint, - this is associated with location, since if breakpoint has several - locations, the evaluation of expression can be different for - different locations. */ + /* Conditional. Break only if this expression's value is nonzero. + Unlike string form of condition, which is associated with + breakpoint, this is associated with location, since if breakpoint + has several locations, the evaluation of expression can be + different for different locations. Only valid for real + breakpoints; a watchpoint's conditional expression is stored in + the owner breakpoint object. */ struct expression *cond; /* This location's address is in an unloaded solib, and so this @@ -439,6 +441,11 @@ struct breakpoint /* The largest block within which it is valid, or NULL if it is valid anywhere (e.g. consists just of global symbols). */ struct block *exp_valid_block; + /* The conditional expression if any. NULL if not a watchpoint. */ + struct expression *cond_exp; + /* The largest block within which it is valid, or NULL if it is + valid anywhere (e.g. consists just of global symbols). */ + struct block *cond_exp_valid_block; /* Value of the watchpoint the last time we checked it, or NULL when we do not know the value yet or the value was not readable. VAL is never lazy. */ 2010-03-04 Pedro Alves * gdb.base/watch-cond.c, gdb.base/watch-cond.exp: New. --- gdb/testsuite/gdb.base/watch-cond.c | 46 ++++++++++++++++++++ gdb/testsuite/gdb.base/watch-cond.exp | 78 ++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) Index: src/gdb/testsuite/gdb.base/watch-cond.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.base/watch-cond.c 2010-03-03 18:28:26.000000000 +0000 @@ -0,0 +1,46 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2010 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 + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +int global = 0; +int global2 = 0; + +int func(int *foo) +{ + (*foo)++; + global++; + global2++; +} + +void func2(int *foo) +{ + global2++; +} + +int main() +{ + int q = 0; + + func2 (&q); + global2++; + + while (1) + { + func(&q); + } + + return 0; +} Index: src/gdb/testsuite/gdb.base/watch-cond.exp =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.base/watch-cond.exp 2010-03-03 18:27:50.000000000 +0000 @@ -0,0 +1,78 @@ +# Copyright 2010 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 +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# +# Tests involving watchpoint conditions with local expressions. +# + +set testfile "watch-cond" +set srcfile ${testfile}.c +set binfile ${objdir}/${subdir}/${testfile} + +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } { + untested ${testfile}.exp + return -1 +} + +if ![runto_main] then { + fail "Can't run to main" + return +} + +gdb_test "watch global if q > 10" \ + "atchpoint .*: global" \ + "set write watchpoint on global variable, local condition" + +gdb_test "continue" \ + "Old value = 10.*New value = 11.*" \ + "watchpoint with global expression, local condition evaluates in correct frame" + +clean_restart ${testfile} + +if ![runto_main] then { + fail "Can't run to main" + return +} + +gdb_test "watch q if q > 10" \ + "atchpoint .*: q" \ + "set write watchpoint on local variable, local condition" + +gdb_test "continue" \ + "Old value = 10.*New value = 11.*" \ + "watchpoint with local expression, local condition evaluates in correct frame" + +clean_restart ${testfile} + +if ![runto_main] then { + fail "Can't run to main" + return +} + +gdb_test "watch global2" \ + "atchpoint.*" \ + "set write watchpoint on global2 variable" + +gdb_test "continue" \ + "Old value = 0.*New value = 1.*" \ + "watchpoint on global2 variable triggers" + +gdb_test "condition 2 *foo > 10" \ + "" \ + "condition of watchpoint 2 changes" + +gdb_test "continue" \ + ".*condition can't tested in the current scope.*Old value = 1.*New value = 2.*" \ + "watchpoint stops with untestable local expression"