This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Somewhat sanitize watchpoints with conditions on local expressions


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"


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]