Somewhat sanitize watchpoints with conditions on local expressions

Somewhat sanitize watchpoints with conditions on local expressions

[Pedro Alves]

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"

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Joel Brobecker]

>  (gdb) cond *foo > 1000
>  Bad breakpoint argument: '*foo > 1000'

GDB does not have bugs, GDB is perfect. Only the operator has bugs.
You forgot the breakpoint number in this example.

(sorry, just couldn't resist)


> 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
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That, or maybe just disable the watchpoint and keep going? It's unclear
what the user intent is, but maybe he really meant for the watchpoint
to be local to the scope where the condition applies?  Maybe your approach
is safer, though - perhaps it's better to over hit a watchpoint rather
than miss some hits...

I agree with your second case.

> 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.

I only skimmed through the patch, just noticed a couple of nits.

> +	      /* 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
                             ^^^^^^
                             simply?
> +	      warning (_("Watchpoint condition can't tested "
                                               ^^^^^^^^^^^^
                                               cannot be tested
                                                
> +			 "in the current scope"));
                                            
Note: I personally would prefer if we kept contractions away from
our error messages (hence the suggestion to use "cannot"), but I'm OK
if others think it's fine.

-- 
Joel

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Thu, 4 Mar 2010 03:50:34 +0000
> 
> 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?

I like it.  One question:

> +static int
> +is_watchpoint (struct breakpoint *bpt)
> +{
> +  return (is_hardware_watchpoint (bpt)
> +	  || bpt->type == bp_watchpoint);
> +}

Does this catch read and access watchpoints?  Should it?

Thanks.

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Eli Zaretskii]

> Date: Thu, 4 Mar 2010 09:59:26 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > 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
> >  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> That, or maybe just disable the watchpoint and keep going?

Personally, I hate it when GDB disables watchpoints automatically,
since more often than not, it does the Wrong Thing.

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Pedro Alves]

On Thursday 04 March 2010 05:59:26, Joel Brobecker wrote:
> >  (gdb) cond *foo > 1000
> >  Bad breakpoint argument: '*foo > 1000'
> 
> GDB does not have bugs, GDB is perfect. Only the operator has bugs.
> You forgot the breakpoint number in this example.
>
> (sorry, just couldn't resist)

Heh!  Thanks, it's official, I'm dumb.  :-)
What I was trying to show does happen, but of course, silly
mistakes happen when trying to show pretty examples.  :-)
If will not-error on non-MMU targets much more easily.

> 
> 
> > 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
> >  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> That, or maybe just disable the watchpoint and keep going? 
> It's unclear
> what the user intent is, but maybe he really meant for the watchpoint
> to be local to the scope where the condition applies?  Maybe your approach
> is safer, though - perhaps it's better to over hit a watchpoint rather
> than miss some hits...

Yeah, one can't really tell, so IMO better stop (which leaves the option
of simply continuing), than not stopping and leave the user wondering
what's changing the memory, and why isn't GDB helping.  It's a one line
change to change this behaviour, so, if usage in the field indicates the
other way around would be better, it's easy to change.

> 
> I agree with your second case.
> 
> > 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.
> 
> I only skimmed through the patch, just noticed a couple of nits.
> 
> > +	      /* 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
>                              ^^^^^^
>                              simply?
> > +	      warning (_("Watchpoint condition can't tested "
>                                                ^^^^^^^^^^^^
>                                                cannot be tested
>                                                 
> > +			 "in the current scope"));
>                                             
> Note: I personally would prefer if we kept contractions away from
> our error messages (hence the suggestion to use "cannot"), but I'm OK
> if others think it's fine.

I'm a convert.  I fully agree.  Both fixed locally.  Thanks!

-- 
Pedro Alves

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Pedro Alves]

On Thursday 04 March 2010 07:55:26, Eli Zaretskii wrote:

> > +static int
> > +is_watchpoint (struct breakpoint *bpt)
> > +{
> > +  return (is_hardware_watchpoint (bpt)
> > +	  || bpt->type == bp_watchpoint);
> > +}
> 
> Does this catch read and access watchpoints?  Should it?

It does, and it should.  I've added the below to the patch.

-- 
Pedro Alves

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c   2010-03-04 16:03:30.000000000 +0000
+++ src/gdb/breakpoint.c        2010-03-04 16:03:09.000000000 +0000
@@ -921,6 +921,8 @@ insert_catchpoint (struct ui_out *uo, vo
   b->ops->insert (b);
 }

+/* Return true if BPT is of any hardware watchpoint kind.  */
+
 static int
 is_hardware_watchpoint (struct breakpoint *bpt)
 {
@@ -929,6 +931,9 @@ is_hardware_watchpoint (struct breakpoin
          || bpt->type == bp_access_watchpoint);
 }

+/* Return true if BPT is of any watchpoint kind, hardware or
+   software.  */
+
 static int
 is_watchpoint (struct breakpoint *bpt)
 {

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Pedro Alves]

On Thursday 04 March 2010 16:00:07, Pedro Alves wrote:
> > That, or maybe just disable the watchpoint and keep going? 
> > It's unclear

> > what the user intent is, but maybe he really meant for the watchpoint
> > to be local to the scope where the condition applies?  

One extra comment.  I though of making the watchpoint trigger only
exactly in the frame where the condition was set, so to
conditionalize on a particular local instance, but I thought that
if the user wants that behavior, he can emulate it by:

 (gdb) p &local
 (gdb) watch global if *$ > 10.

But there's no way to emulate what the patch allows, and
it seemed intuitive, so went with it, and so the user can
have it both ways.

-- 
Pedro Alves

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Pedro Alves]

Here's the current patch with fixes and comments incorporated.

2010-03-04  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.c (condition_command): Handle watchpoint conditions.
	(is_hardware_watchpoint): Add comment.
	(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): Evaluate watchpoint local
	conditions in a frame where it makes sense.
	(watch_command_1): Store the innermost block of the condition
	expression.
	(delete_breakpoint): Delete the watchpoint condition expression.
	* breakpoint.h (struct bp_location) <cond>: Update comment.
	(struct breakpoint): New field `cond_exp_valid_block'.

	gdb/testsuite/
	* gdb.base/watch-cond.c, gdb.base/watch-cond.exp: New.

-- 
Pedro Alves

---
 gdb/breakpoint.c |  178 +++++++++++++++++++++++++++++++++++++++----------------
 gdb/breakpoint.h |   17 +++--
 2 files changed, 141 insertions(+), 54 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2010-03-04 15:54:08.000000000 +0000
+++ src/gdb/breakpoint.c	2010-03-04 16:03:09.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 ();
@@ -908,6 +921,8 @@ insert_catchpoint (struct ui_out *uo, vo
   b->ops->insert (b);
 }
 
+/* Return true if BPT is of any hardware watchpoint kind.  */
+
 static int
 is_hardware_watchpoint (struct breakpoint *bpt)
 {
@@ -916,6 +931,16 @@ is_hardware_watchpoint (struct breakpoin
 	  || bpt->type == bp_access_watchpoint);
 }
 
+/* Return true if BPT is of any watchpoint kind, hardware or
+   software.  */
+
+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
@@ -1250,14 +1275,13 @@ update_watchpoint (struct breakpoint *b,
 	  b->loc->watchpoint_type = -1;
 	}
 
-      /* 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)
@@ -1267,7 +1291,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;
     }
 
@@ -3251,6 +3279,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
@@ -3274,7 +3304,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;
@@ -3293,7 +3323,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);
@@ -3344,14 +3374,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;
 	}
     }
@@ -3378,7 +3406,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;
@@ -3498,6 +3530,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)
 		{
@@ -3617,16 +3653,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
@@ -3640,15 +3684,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 simply 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 cannot be 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;
 	}
@@ -7303,7 +7382,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;
@@ -7408,8 +7487,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;
     }
@@ -7450,7 +7535,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)))
 	{
@@ -7487,6 +7572,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;
@@ -8862,20 +8948,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 15:54:09.000000000 +0000
+++ src/gdb/breakpoint.h	2010-03-04 15:59:24.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"

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Pedro Alves]

On Thursday 04 March 2010 16:18:59, Pedro Alves wrote:
> +    ".*condition can't tested in the current scope.*Old value = 1.*New value = 2.*" \

Sigh.  I've updated this string as well to go along the suggested change,
but posted the old test patch...

-- 
Pedro Alves

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Joel Brobecker]

> 2010-03-04  Pedro Alves  <pedro@codesourcery.com>
> 
> 	gdb/
> 	* breakpoint.c (condition_command): Handle watchpoint conditions.
> 	(is_hardware_watchpoint): Add comment.
> 	(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): Evaluate watchpoint local
> 	conditions in a frame where it makes sense.
> 	(watch_command_1): Store the innermost block of the condition
> 	expression.
> 	(delete_breakpoint): Delete the watchpoint condition expression.
> 	* breakpoint.h (struct bp_location) <cond>: Update comment.
> 	(struct breakpoint): New field `cond_exp_valid_block'.
> 
> 	gdb/testsuite/
> 	* gdb.base/watch-cond.c, gdb.base/watch-cond.exp: New.

FWIW, this looks fine to me.  And after discussion, I agree it seems
much better to always stop rather than disable the watchpoint when
the condition can no longer be evaluated in the current scope.

-- 
Joel

Re: Somewhat sanitize watchpoints with conditions on local expressions

[Pedro Alves]

On Thursday 04 March 2010 16:18:59, Pedro Alves wrote:
> 	gdb/
> 	* breakpoint.c (condition_command): Handle watchpoint conditions.
> 	(is_hardware_watchpoint): Add comment.
> 	(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): Evaluate watchpoint local
> 	conditions in a frame where it makes sense.
> 	(watch_command_1): Store the innermost block of the condition
> 	expression.
> 	(delete_breakpoint): Delete the watchpoint condition expression.
> 	* breakpoint.h (struct bp_location) <cond>: Update comment.
> 	(struct breakpoint): New field `cond_exp_valid_block'.
> 
> 	gdb/testsuite/
> 	* gdb.base/watch-cond.c, gdb.base/watch-cond.exp: New.

I've committed these now.

-- 
Pedro Alves