disable breakpoints with invalid condition (Re: [Patch] Cannot set pending bp if condition set explicitly)

[Pedro Alves <palves@redhat.com>]

On 07/30/2012 04:18 PM, Pedro Alves wrote:
>>> >> This currently works:
>>> >>
>>> >>  (gdb) b main if 1 == 1 thread 1
>>> >>  Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29.
>>> >>
>>> >> But this does not:
>>> >>
>>> >>  (gdb) condition 2 1 == 1 thread 1
>>> >>  Junk at end of expression
>> > 
>> > On a side-note, I see that although there is an error printed and 
>> > set_breakpoint_condition() is aborted, the bad condition is not
>> > cleaned-up, as shown here:
>> > 
>> > (gdb) info b
>> > Num     Type           Disp Enb Address    What
>> > 1       breakpoint     keep y   0x0804851d in main at ../../../src/gdb/testsuite/gdb.base/pending.c:26
>> >         stop only if k==1 thread 1
>> > 
>> > Shouldn't there be some kind of cleanup in set_breakpoint_condition()?
> Indeed.  Hmm, that, or disable the location, with a warning.  We parse the condition in the
> context of each location.  A condition might well make sense and parse okay
> for a location, but not for another (e.g., different locals/parameters).  Disabling
> is also what we do if we fail to parse a location later on with "break foo if bar".
> 

Here's a patch implementing this idea.  Examples (currently, the condition text
is left stale, and the breakpoint becomes uncondition):

(gdb) b main
Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29.
(gdb) condition 2 foo bar
warning: Breakpoint 2.1 disabled: No symbol "foo" in current context.
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   <MULTIPLE>
        stop only if foo bar
2.1                         n     0x00000000004572bb in main at ../../src/gdb/gdb.c:29
(gdb) condition 2
Breakpoint 2 now unconditional.
(gdb) enable 2.1
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29

(gdb) watch args
Hardware watchpoint 3: args
(gdb) condition 3
Breakpoint 3 now unconditional.
(gdb) condition 3 foo bar
warning: Watchpoint 3 disabled: No symbol "foo" in current context.
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29
3       hw watchpoint  keep n                      args
        stop only if foo bar
(gdb) condition 3 1 == 1 thread 1
warning: Watchpoint 3 disabled - junk at end of expression: thread 1
(gdb)

There are several little details we get wrong that I haven't addressed.
For instance, in this case, the re-enabling of a watchpoint is prevented:

(gdb) condition 3 asdf
warning: Watchpoint 3 disabled: No symbol "asdf" in current context.
(gdb) info breakpoints 3
Num     Type           Disp Enb Address            What
3       hw watchpoint  keep n                      args
        stop only if asdf
        breakpoint already hit 1 time
(gdb) enable 3
Cannot enable watchpoint 3: No symbol "asdf" in current context.

While this doesn't:

(gdb) condition 3 1 == 1 thread 1
warning: Watchpoint 3 disabled - junk at end of expression: thread 1
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29
3       hw watchpoint  keep n                      args
        stop only if 1 == 1 thread 1
        breakpoint already hit 1 time
(gdb) enable 3
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29
3       hw watchpoint  keep y                      args
        stop only if 1 == 1 thread 1
        breakpoint already hit 1 time
(gdb)

For regular breakpoints, you can always "enable" them, but when you let
the program run, you'll find they are unconditional, and if something causes
a re-set, they get disabled again:

warning: failed to reevaluate condition for breakpoint 4: blah blah.


I'd appreciate hearing opinions on whether this change is a good direction or not.

macscp.exp had a case of an invalid condition being set on a breakpoint, and then
the test assuming the breakpoint would be hit.

2012-08-02  Pedro Alves  <palves@redhat.com>

	gdb/
	* breakpoint.c (set_breakpoint_condition): If condition fails to
	parse, disable the watchpoint or breakpoint location, and warn.

	gdb/doc/
	* gdb.texinfo (condition command): Mention that locations are
	disabled when the condition has a problem.

	gdb/testsuite/
	* gdb.base/condbreak.exp: New tests for invalid conditions with
	"condition" command.
	* gdb.base/macscp.exp: After setting an invalid condition to a
	breakpoint, make the breakpoint unconditional and re-enable it.

---

 gdb/testsuite/gdb.base/condbreak.exp |    9 +++++++++
 gdb/testsuite/gdb.base/macscp.exp    |    7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 03719d4..b8484a7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -956,26 +956,59 @@ set_breakpoint_condition (struct breakpoint *b, char *exp,
       if (is_watchpoint (b))
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
+	  volatile struct gdb_exception e;

 	  innermost_block = NULL;
 	  arg = exp;
-	  w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
-	  if (*arg)
-	    error (_("Junk at end of expression"));
-	  w->cond_exp_valid_block = innermost_block;
+	  TRY_CATCH (e, RETURN_MASK_ERROR)
+	    {
+	      w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
+	    }
+	  if (e.reason < 0)
+	    {
+	      b->enable_state = bp_disabled;
+	      warning (_("Watchpoint %d disabled: %s"),
+		     b->number, e.message);
+	    }
+	  else if (*arg)
+	    {
+	      b->enable_state = bp_disabled;
+	      warning (_("Watchpoint %d disabled - "
+		       "junk at end of expression: %s"),
+		     b->number, arg);
+	    }
+	  else
+	    w->cond_exp_valid_block = innermost_block;
 	}
       else
 	{
 	  struct bp_location *loc;
+	  int loc_n;

-	  for (loc = b->loc; loc; loc = loc->next)
+	  for (loc = b->loc, loc_n = 1; loc; loc = loc->next, loc_n++)
 	    {
+	      volatile struct gdb_exception e;
+
 	      arg = exp;
-	      loc->cond =
-		parse_exp_1 (&arg, loc->address,
-			     block_for_pc (loc->address), 0);
-	      if (*arg)
-		error (_("Junk at end of expression"));
+
+	      TRY_CATCH (e, RETURN_MASK_ERROR)
+		{
+		  loc->cond = parse_exp_1 (&arg, loc->address,
+					   block_for_pc (loc->address), 0);
+		}
+	      if (e.reason < 0)
+		{
+		  loc->enabled = 0;
+		  warning (_("Breakpoint %d.%d disabled: %s"),
+			   b->number, loc_n, e.message);
+		}
+	      else if (*arg)
+		{
+		  loc->enabled = 0;
+		  warning (_("Breakpoint %d.%d disabled - "
+			     "junk at end of expression: %s"),
+			   b->number, loc_n, arg);
+		}
 	    }
 	}
     }
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index facca8f..8518d2f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -4487,11 +4487,12 @@ breakpoint @var{bnum} stops your program only if the value of
 @code{condition}, @value{GDBN} checks @var{expression} immediately for
 syntactic correctness, and to determine whether symbols in it have
 referents in the context of your breakpoint.  If @var{expression} uses
-symbols not referenced in the context of the breakpoint, @value{GDBN}
-prints an error message:
+symbols not referenced in the context any of the breakpoint's
+locations, @value{GDBN} prints a warning message and disables the
+corresponding location:

 @smallexample
-No symbol "foo" in current context.
+warning: Breakpoint 1.1 disabled: No symbol "foo" in current context.
 @end smallexample

 @noindent
diff --git a/gdb/testsuite/gdb.base/condbreak.exp b/gdb/testsuite/gdb.base/condbreak.exp
index e8e0d84..9d91fab 100644
--- a/gdb/testsuite/gdb.base/condbreak.exp
+++ b/gdb/testsuite/gdb.base/condbreak.exp
@@ -266,3 +266,12 @@ gdb_test "complete cond 1" "cond 1"
 gdb_test "set variable \$var = 1"
 gdb_test "complete cond \$v" "cond \\\$var"
 gdb_test "complete cond 1 values\[0\].a" "cond 1 values.0..a_field"
+
+gdb_test "cond 1 foo bar" ".*" "set invalid condition"
+
+gdb_test "info break 1" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+1\[\t \]+breakpoint     keep y.* <MULTIPLE>.*
+\[\t \]+stop only if foo bar.*
+1.1\[\t \]+n\[\t \]+.*in main at .*$srcfile:$bp_location6.*" \
+    "location with invalid condition is disabled"
diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
index 1f995d5..ad18245 100644
--- a/gdb/testsuite/gdb.base/macscp.exp
+++ b/gdb/testsuite/gdb.base/macscp.exp
@@ -444,6 +444,13 @@ gdb_test "cond \$bpnum foo == MACRO_TO_EXPAND" \
   "No symbol \"MACRO_TO_EXPAND\" in current context\." \
   "macro MACRO_TO_EXPAND not in scope at breakpoint"

+gdb_test "cond \$bpnum" \
+  "Breakpoint .* now unconditional\." \
+  "make breakpoint unconditional"
+
+gdb_test_no_output "enable \$bpnum.1" \
+  "re-enable breakpoint"
+
 # Note that we choose the condition so that this breakpoint never
 # stops.
 set l2 [gdb_get_line_number "set second breakpoint here"]