This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Somewhat sanitize watchpoints with conditions on local expressions
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Date: Thu, 4 Mar 2010 03:50:34 +0000
- Subject: Somewhat sanitize watchpoints with conditions on local expressions
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 <pedro@codesourcery.com>
* 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 <pedro@codesourcery.com>
* 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 <pedro@codesourcery.com>
* 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 <http://www.gnu.org/licenses/>. */
+
+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 <http://www.gnu.org/licenses/>.
+
+#
+# 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"