gdb/breakpoint: set the condition exp after parsing the condition successfully

In 'set_breakpoint_condition', GDB resets the condition expressions
before parsing the condition input by the user.  This leads to the
problem of losing the condition expressions if the new condition
does not parse successfully and is thus rejected.

For instance:

  $ gdb ./test
  Reading symbols from ./test...
  (gdb) start
  Temporary breakpoint 1 at 0x114e: file test.c, line 4.
  Starting program: test

  Temporary breakpoint 1, main () at test.c:4
  4         int a = 10;
  (gdb) break 5
  Breakpoint 2 at 0x555555555155: file test.c, line 5.

Now define a condition that would evaluate to false.  Next, attempt
to overwrite that with an invalid condition:

  (gdb) cond 2 a == 999
  (gdb) cond 2 gibberish
  No symbol "gibberish" in current context.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   0x0000555555555155 in main at test.c:5
          stop only if a == 999

It appears as if the bad condition is successfully rejected.  But if we
resume the program, we see that we hit the breakpoint although the condition
would evaluate to false.

  (gdb) continue
  Continuing.

  Breakpoint 2, main () at test.c:5
  5         a = a + 1; /* break-here */

Fix the problem by not resetting the condition expressions before
parsing the condition input.

Suppose the fix is applied.  A similar problem could occur if the
condition is valid, but has "junk" at the end.  In this case, parsing
succeeds, but an error is raised immediately after.  It is too late,
though; the condition expression is already updated.

For instance:

  $ gdb ./test
  Reading symbols from ./test...
  (gdb) start
  Temporary breakpoint 1 at 0x114e: file test.c, line 4.
  Starting program: test

  Temporary breakpoint 1, main () at test.c:4
  4         int a = 10;
  (gdb) break 5
  Breakpoint 2 at 0x555555555155: file test.c, line 5.
  (gdb) cond 2 a == 999
  (gdb) cond 2 a == 10 if
  Junk at end of expression
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   0x0000555555555155 in main at test.c:5
          stop only if a == 999
  (gdb) c
  Continuing.

  Breakpoint 2, main () at test.c:5
  5         a = a + 1; /* break-here */
  (gdb)

We should not have hit the breakpoint because the condition would
evaluate to false.

Fix this problem by updating the condition expression of the breakpoint
after parsing the input successfully and checking that there is no
remaining junk.

gdb/ChangeLog:
2020-07-30  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* breakpoint.c (set_breakpoint_condition): Update the condition
	expressions after checking that the input condition string parses
	successfully and does not contain junk at the end.

gdb/testsuite/ChangeLog:
2020-07-30  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.base/condbreak-bad.exp: Extend the test with scenarios
	that attempt to overwrite an existing condition with a condition
	that fails parsing and also with a condition that parses fine
	but contains junk at the end.

1 files changed, 38 insertions, 21 deletions

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a3a7c17..7e020c5 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -834,30 +834,30 @@ void
 set_breakpoint_condition (struct breakpoint *b, const char *exp,
 			  int from_tty)
 {
-  if (is_watchpoint (b))
-    {
-      struct watchpoint *w = (struct watchpoint *) b;
-
-      w->cond_exp.reset ();
-    }
-  else
+  if (*exp == 0)
     {
-      struct bp_location *loc;
+      xfree (b->cond_string);
+      b->cond_string = nullptr;
 
-      for (loc = b->loc; loc; loc = loc->next)
+      if (is_watchpoint (b))
 	{
-	  loc->cond.reset ();
+	  struct watchpoint *w = (struct watchpoint *) b;
 
-	  /* No need to free the condition agent expression
-	     bytecode (if we have one).  We will handle this
-	     when we go through update_global_location_list.  */
+	  w->cond_exp.reset ();
 	}
-    }
+      else
+	{
+	  struct bp_location *loc;
 
-  if (*exp == 0)
-    {
-      xfree (b->cond_string);
-      b->cond_string = nullptr;
+	  for (loc = b->loc; loc; loc = loc->next)
+	    {
+	      loc->cond.reset ();
+
+	      /* No need to free the condition agent expression
+		 bytecode (if we have one).  We will handle this
+		 when we go through update_global_location_list.  */
+	    }
+	}
 
       if (from_tty)
 	printf_filtered (_("Breakpoint %d now unconditional.\n"), b->number);
@@ -872,23 +872,40 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 
 	  innermost_block_tracker tracker;
 	  arg = exp;
-	  w->cond_exp = parse_exp_1 (&arg, 0, 0, 0, &tracker);
+	  expression_up new_exp = parse_exp_1 (&arg, 0, 0, 0, &tracker);
 	  if (*arg)
 	    error (_("Junk at end of expression"));
+	  w->cond_exp = std::move (new_exp);
 	  w->cond_exp_valid_block = tracker.block ();
 	}
       else
 	{
 	  struct bp_location *loc;
 
+	  /* Parse and set condition expressions.  We make two passes.
+	     In the first, we parse the condition string to see if it
+	     is valid in all locations.  If so, the condition would be
+	     accepted.  So we go ahead and set the locations'
+	     conditions.  In case a failing case is found, we throw
+	     the error and the condition string will be rejected.
+	     This two-pass approach is taken to avoid setting the
+	     state of locations in case of a reject.  */
+	  for (loc = b->loc; loc; loc = loc->next)
+	    {
+	      arg = exp;
+	      parse_exp_1 (&arg, loc->address,
+			   block_for_pc (loc->address), 0);
+	      if (*arg != 0)
+		error (_("Junk at end of expression"));
+	    }
+
+	  /* If we reach here, the condition is valid at all locations.  */
 	  for (loc = b->loc; loc; loc = loc->next)
 	    {
 	      arg = exp;
 	      loc->cond =
 		parse_exp_1 (&arg, loc->address,
 			     block_for_pc (loc->address), 0);
-	      if (*arg)
-		error (_("Junk at end of expression"));
 	    }
 	}