[PATCH 0/3] Prevent bad conditions from putting breakpoints into broken state

[PATCH 0/3] Prevent bad conditions from putting breakpoints into broken state

[Tankut Baris Aktemur]

Hi,

If the condition of a breakpoint is "bad" (e.g. has unresolved
symbols, syntax errors, etc.), the breakpoint goes into a broken state
where the condition string or the condition expressions are updated
inadvertently.  This small series attempts to fix that.

Regards.
Baris

Tankut Baris Aktemur (3):
  gdb/breakpoint: do not update the condition string if parsing the
    condition fails
  gdb/breakpoint: set the condition exp after parsing the condition
    successfully
  gdb/breakpoint: refactor 'set_breakpoint_condition'

 gdb/breakpoint.c                         |  70 ++++++-------
 gdb/testsuite/gdb.base/condbreak-bad.c   |  24 +++++
 gdb/testsuite/gdb.base/condbreak-bad.exp | 128 +++++++++++++++++++++++
 3 files changed, 183 insertions(+), 39 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.c
 create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.exp

-- 
2.17.1

[PATCH 1/3] gdb/breakpoint: do not update the condition string if parsing the condition fails

[Tankut Baris Aktemur]

The condition of a breakpoint can be set with the 'cond' command.  If
the condition has errors that make it problematic to evaluate, it
appears like GDB rejects the condition, but updates the breakpoint's
condition string, which causes incorrect/unintuitive behavior.

For instance:

  $ gdb ./test
  Reading symbols from ./test...
  (gdb) break 5
  Breakpoint 1 at 0x1155: file test.c, line 5.
  (gdb) cond 1 gibberish
  No symbol "gibberish" in current context.

At this point, it looks like the condition was rejected.
But "info breakpoints" shows the following:

  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   0x0000000000001155 in main at test.c:5
          stop only if gibberish

Running the code gives the following behavior, where re-insertion of
the breakpoint causes failures.

  (gdb) run
  Starting program: test
  warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
  warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
  warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
  warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
  warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
  [Inferior 1 (process 19084) exited normally]
  (gdb)

This broken behavior occurs because GDB updates the condition string
of the breakpoint *before* checking that it parses successfully.
When parsing fails, the update has already taken place.

Fix the problem by updating the condition string *after* parsing the
condition.  We get the following behavior when this patch is applied:

  $ gdb ./test
  Reading symbols from ./test...
  (gdb) break 5
  Breakpoint 1 at 0x1155: file test.c, line 5.
  (gdb) cond 1 gibberish
  No symbol "gibberish" in current context.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   0x0000000000001155 in main at test.c:5
  (gdb) run
  Starting program: test

  Breakpoint 1, main () at test.c:5
  5         a = a + 1; /* break-here */
  (gdb) c
  Continuing.
  [Inferior 1 (process 15574) exited normally]
  (gdb)

A side note: The problem does not occur if the condition is given
at the time of breakpoint definition, as in "break 5 if gibberish",
because the parsing of the condition fails during symtab-and-line
creation, before the breakpoint is created.

Finally, the code included the following comment:

  /* I don't know if it matters whether this is the string the user
     typed in or the decompiled expression.  */

This comment did not make sense to me because the condition string is
the user-typed input.  The patch updates this comment, too.

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

	* breakpoint.c (set_breakpoint_condition): Update the
	condition string after parsing the new condition successfully.

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

	* gdb.base/condbreak-bad.c: New test.
	* gdb.base/condbreak-bad.exp: New file.
---
 gdb/breakpoint.c                         | 17 +++++-----
 gdb/testsuite/gdb.base/condbreak-bad.c   | 24 ++++++++++++++
 gdb/testsuite/gdb.base/condbreak-bad.exp | 40 ++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.c
 create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6d81323dd92..1fc2d1b8966 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -834,9 +834,6 @@ void
 set_breakpoint_condition (struct breakpoint *b, const char *exp,
 			  int from_tty)
 {
-  xfree (b->cond_string);
-  b->cond_string = NULL;
-
   if (is_watchpoint (b))
     {
       struct watchpoint *w = (struct watchpoint *) b;
@@ -859,6 +856,9 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 
   if (*exp == 0)
     {
+      xfree (b->cond_string);
+      b->cond_string = nullptr;
+
       if (from_tty)
 	printf_filtered (_("Breakpoint %d now unconditional.\n"), b->number);
     }
@@ -866,11 +866,6 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
     {
       const char *arg = exp;
 
-      /* I don't know if it matters whether this is the string the user
-	 typed in or the decompiled expression.  */
-      b->cond_string = xstrdup (arg);
-      b->condition_not_parsed = 0;
-
       if (is_watchpoint (b))
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
@@ -896,6 +891,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 		error (_("Junk at end of expression"));
 	    }
 	}
+
+      /* We know that the new condition parsed successfully.  The
+	 condition string of the breakpoint can be safely updated.  */
+      xfree (b->cond_string);
+      b->cond_string = xstrdup (exp);
+      b->condition_not_parsed = 0;
     }
   mark_breakpoint_modified (b);
 
diff --git a/gdb/testsuite/gdb.base/condbreak-bad.c b/gdb/testsuite/gdb.base/condbreak-bad.c
new file mode 100644
index 00000000000..58283b75ca7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/condbreak-bad.c
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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
+main ()
+{
+  int a = 10;
+  a = a + 1; /* break-here */
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/condbreak-bad.exp b/gdb/testsuite/gdb.base/condbreak-bad.exp
new file mode 100644
index 00000000000..a01ba2a9340
--- /dev/null
+++ b/gdb/testsuite/gdb.base/condbreak-bad.exp
@@ -0,0 +1,40 @@
+# Copyright 2020 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/>.
+
+# Test defining bad conditions for breakpoints.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" ${binfile} ${srcfile}]} {
+    return
+}
+
+set bp_location [gdb_get_line_number "break-here"]
+gdb_breakpoint "$bp_location"
+set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"]
+
+# Define a 'bad' condition.  The breakpoint should stay unconditional.
+gdb_test "cond $bpnum gibberish" \
+    "No symbol \"gibberish\" in current context." \
+    "attempt a bad condition"
+
+set fill "\[^\r\n\]*"
+
+gdb_test "info break" \
+    [multi_line \
+	 "Num${fill}What" \
+	 "${decimal}${fill}breakpoint${fill}keep y${fill}:${bp_location}"] \
+    "breakpoint is unconditional"
+
-- 
2.17.1

[PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Tankut Baris Aktemur]

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-06-29  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-06-29  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.base/condbreak-bad.exp: Extend the test with scenarios
	that attempt to overwrite and existing condition with a condition
	that fails parsing and also with a condition that parses fine
	but contains junk at the end.
---
 gdb/breakpoint.c                         | 46 +++++++------
 gdb/testsuite/gdb.base/condbreak-bad.exp | 88 ++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 22 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1fc2d1b8966..abda470fe21 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,9 +872,10 @@ 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
@@ -884,11 +885,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	  for (loc = b->loc; loc; loc = loc->next)
 	    {
 	      arg = exp;
-	      loc->cond =
-		parse_exp_1 (&arg, loc->address,
-			     block_for_pc (loc->address), 0);
+	      expression_up new_exp
+		= parse_exp_1 (&arg, loc->address,
+			       block_for_pc (loc->address), 0);
 	      if (*arg)
 		error (_("Junk at end of expression"));
+	      loc->cond = std::move (new_exp);
 	    }
 	}
 
diff --git a/gdb/testsuite/gdb.base/condbreak-bad.exp b/gdb/testsuite/gdb.base/condbreak-bad.exp
index a01ba2a9340..84d32a0f15d 100644
--- a/gdb/testsuite/gdb.base/condbreak-bad.exp
+++ b/gdb/testsuite/gdb.base/condbreak-bad.exp
@@ -38,3 +38,91 @@ gdb_test "info break" \
 	 "${decimal}${fill}breakpoint${fill}keep y${fill}:${bp_location}"] \
     "breakpoint is unconditional"
 
+# Now define a valid condition.  Attempt to override that with a 'bad'
+# condition again.  The condition should be preserved.
+with_test_prefix "with run" {
+    gdb_test_no_output "cond $bpnum a == 10"
+
+    gdb_test "cond $bpnum gibberish" \
+	"No symbol \"gibberish\" in current context." \
+	"attempt a bad condition"
+
+    gdb_test "info break" \
+	[multi_line \
+	     "Num${fill}What" \
+	     "${decimal}${fill}breakpoint${fill}keep y${fill}:${bp_location}" \
+	     "${fill}stop only if a == 10${fill}"] \
+	"breakpoint condition is preserved"
+
+    # Run the code.  We should hit the breakpoint, because the
+    # condition evaluates to true.
+
+    gdb_run_cmd
+    gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "run to the bp"
+}
+
+# Restart.  Repeat the test above after the program has started.
+# This is needed to check a scenario where the breakpoints are no
+# longer re-inserted due to solib events.  Note that runto_main
+# deletes the breakpoints.
+with_test_prefix "with continue 1" {
+    if {![runto_main]} {
+	fail "could not run to main"
+	return -1
+    }
+
+    gdb_breakpoint "$bp_location"
+    set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"]
+
+    gdb_test_no_output "cond $bpnum a == 10"
+
+    gdb_test "cond $bpnum gibberish" \
+	"No symbol \"gibberish\" in current context." \
+	"attempt a bad condition"
+
+    # Resume.  We should hit the breakpoint, because the
+    # condition evaluates to true.
+    gdb_continue_to_breakpoint "${srcfile}:${bp_location}"
+}
+
+# Repeat with a condition that evaluates to false.
+with_test_prefix "with continue 2" {
+    if {![runto_main]} {
+	fail "could not run to main"
+	return -1
+    }
+
+    gdb_breakpoint "$bp_location"
+    set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"]
+
+    gdb_test_no_output "cond $bpnum a == 999"
+    
+    gdb_test "cond $bpnum gibberish" \
+	"No symbol \"gibberish\" in current context." \
+	"attempt a bad condition"
+
+    # Resume.  We should *not* hit the breakpoint, because the
+    # condition evaluates to false.
+    gdb_continue_to_end
+}
+
+# Repeat with a condition that contains junk at the end.
+with_test_prefix "with junk" {
+    if {![runto_main]} {
+	fail "could not run to main"
+	return -1
+    }
+
+    gdb_breakpoint "$bp_location"
+    set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"]
+
+    gdb_test_no_output "cond $bpnum a == 999"
+
+    gdb_test "cond $bpnum a == 10 if" \
+	"Junk at end of expression" \
+	"attempt a bad condition"
+
+    # Resume.  We should *not* hit the breakpoint, because the
+    # condition evaluates to false.
+    gdb_continue_to_end
+}
-- 
2.17.1

[PATCH 3/3] gdb/breakpoint: refactor 'set_breakpoint_condition'

[Tankut Baris Aktemur]

Apply minor refactoring to 'set_breakpoint_condition'.

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

	* breakpoint.c (set_breakpoint_condition): Do minor refactoring.
---
 gdb/breakpoint.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index abda470fe21..aebc123f86f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -840,16 +840,10 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
       b->cond_string = nullptr;
 
       if (is_watchpoint (b))
-	{
-	  struct watchpoint *w = (struct watchpoint *) b;
-
-	  w->cond_exp.reset ();
-	}
+	static_cast<watchpoint *> (b)->cond_exp.reset ();
       else
 	{
-	  struct bp_location *loc;
-
-	  for (loc = b->loc; loc; loc = loc->next)
+	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
 	    {
 	      loc->cond.reset ();
 
@@ -864,31 +858,26 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
     }
   else
     {
-      const char *arg = exp;
-
       if (is_watchpoint (b))
 	{
-	  struct watchpoint *w = (struct watchpoint *) b;
-
 	  innermost_block_tracker tracker;
-	  arg = exp;
+	  const char *arg = exp;
 	  expression_up new_exp = parse_exp_1 (&arg, 0, 0, 0, &tracker);
-	  if (*arg)
+	  if (*arg != 0)
 	    error (_("Junk at end of expression"));
+	  watchpoint *w = static_cast<watchpoint *> (b);
 	  w->cond_exp = std::move (new_exp);
 	  w->cond_exp_valid_block = tracker.block ();
 	}
       else
 	{
-	  struct bp_location *loc;
-
-	  for (loc = b->loc; loc; loc = loc->next)
+	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
 	    {
-	      arg = exp;
+	      const char *arg = exp;
 	      expression_up new_exp
 		= parse_exp_1 (&arg, loc->address,
 			       block_for_pc (loc->address), 0);
-	      if (*arg)
+	      if (*arg != 0)
 		error (_("Junk at end of expression"));
 	      loc->cond = std::move (new_exp);
 	    }
-- 
2.17.1

Re: [PATCH 0/3] Prevent bad conditions from putting breakpoints into broken state

[Tankut Baris Aktemur]

> Hi,
>
> If the condition of a breakpoint is "bad" (e.g. has unresolved
> symbols, syntax errors, etc.), the breakpoint goes into a broken state
> where the condition string or the condition expressions are updated
> inadvertently.  This small series attempts to fix that.


Kindly pinging for

  https://sourceware.org/pipermail/gdb-patches/2020-June/169954.html

Thanks.
Baris

Re: [PATCH 0/3] Prevent bad conditions from putting breakpoints into broken state

[Tankut Baris Aktemur]

> Hi,
>
> If the condition of a breakpoint is "bad" (e.g. has unresolved
> symbols, syntax errors, etc.), the breakpoint goes into a broken state
> where the condition string or the condition expressions are updated
> inadvertently.  This small series attempts to fix that.


Kindly pinging for

  https://sourceware.org/pipermail/gdb-patches/2020-June/169954.html

Thanks.
Baris

Re: [PATCH 1/3] gdb/breakpoint: do not update the condition string if parsing the condition fails

[Simon Marchi]

On 2020-06-29 9:48 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> The condition of a breakpoint can be set with the 'cond' command.  If
> the condition has errors that make it problematic to evaluate, it
> appears like GDB rejects the condition, but updates the breakpoint's
> condition string, which causes incorrect/unintuitive behavior.
> 
> For instance:
> 
>   $ gdb ./test
>   Reading symbols from ./test...
>   (gdb) break 5
>   Breakpoint 1 at 0x1155: file test.c, line 5.
>   (gdb) cond 1 gibberish
>   No symbol "gibberish" in current context.
> 
> At this point, it looks like the condition was rejected.
> But "info breakpoints" shows the following:
> 
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   1       breakpoint     keep y   0x0000000000001155 in main at test.c:5
>           stop only if gibberish
> 
> Running the code gives the following behavior, where re-insertion of
> the breakpoint causes failures.
> 
>   (gdb) run
>   Starting program: test
>   warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
>   warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
>   warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
>   warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
>   warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context.
>   [Inferior 1 (process 19084) exited normally]
>   (gdb)
> 
> This broken behavior occurs because GDB updates the condition string
> of the breakpoint *before* checking that it parses successfully.
> When parsing fails, the update has already taken place.
> 
> Fix the problem by updating the condition string *after* parsing the
> condition.  We get the following behavior when this patch is applied:
> 
>   $ gdb ./test
>   Reading symbols from ./test...
>   (gdb) break 5
>   Breakpoint 1 at 0x1155: file test.c, line 5.
>   (gdb) cond 1 gibberish
>   No symbol "gibberish" in current context.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   1       breakpoint     keep y   0x0000000000001155 in main at test.c:5
>   (gdb) run
>   Starting program: test
> 
>   Breakpoint 1, main () at test.c:5
>   5         a = a + 1; /* break-here */
>   (gdb) c
>   Continuing.
>   [Inferior 1 (process 15574) exited normally]
>   (gdb)
> 
> A side note: The problem does not occur if the condition is given
> at the time of breakpoint definition, as in "break 5 if gibberish",
> because the parsing of the condition fails during symtab-and-line
> creation, before the breakpoint is created.
> 
> Finally, the code included the following comment:
> 
>   /* I don't know if it matters whether this is the string the user
>      typed in or the decompiled expression.  */
> 
> This comment did not make sense to me because the condition string is
> the user-typed input.  The patch updates this comment, too.
> 
> gdb/ChangeLog:
> 2020-06-29  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* breakpoint.c (set_breakpoint_condition): Update the
> 	condition string after parsing the new condition successfully.
> 
> gdb/testsuite/ChangeLog:
> 2020-06-29  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* gdb.base/condbreak-bad.c: New test.
> 	* gdb.base/condbreak-bad.exp: New file.
> ---
>  gdb/breakpoint.c                         | 17 +++++-----
>  gdb/testsuite/gdb.base/condbreak-bad.c   | 24 ++++++++++++++
>  gdb/testsuite/gdb.base/condbreak-bad.exp | 40 ++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.c
>  create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.exp
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 6d81323dd92..1fc2d1b8966 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -834,9 +834,6 @@ void
>  set_breakpoint_condition (struct breakpoint *b, const char *exp,
>  			  int from_tty)
>  {
> -  xfree (b->cond_string);
> -  b->cond_string = NULL;
> -
>    if (is_watchpoint (b))
>      {
>        struct watchpoint *w = (struct watchpoint *) b;
> @@ -859,6 +856,9 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
>  
>    if (*exp == 0)
>      {
> +      xfree (b->cond_string);
> +      b->cond_string = nullptr;
> +
>        if (from_tty)
>  	printf_filtered (_("Breakpoint %d now unconditional.\n"), b->number);
>      }
> @@ -866,11 +866,6 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
>      {
>        const char *arg = exp;
>  
> -      /* I don't know if it matters whether this is the string the user
> -	 typed in or the decompiled expression.  */
> -      b->cond_string = xstrdup (arg);
> -      b->condition_not_parsed = 0;
> -
>        if (is_watchpoint (b))
>  	{
>  	  struct watchpoint *w = (struct watchpoint *) b;
> @@ -896,6 +891,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
>  		error (_("Junk at end of expression"));
>  	    }
>  	}
> +
> +      /* We know that the new condition parsed successfully.  The
> +	 condition string of the breakpoint can be safely updated.  */
> +      xfree (b->cond_string);
> +      b->cond_string = xstrdup (exp);
> +      b->condition_not_parsed = 0;
>      }
>    mark_breakpoint_modified (b);
>  
> diff --git a/gdb/testsuite/gdb.base/condbreak-bad.c b/gdb/testsuite/gdb.base/condbreak-bad.c
> new file mode 100644
> index 00000000000..58283b75ca7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/condbreak-bad.c
> @@ -0,0 +1,24 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 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
> +main ()
> +{
> +  int a = 10;
> +  a = a + 1; /* break-here */
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/condbreak-bad.exp b/gdb/testsuite/gdb.base/condbreak-bad.exp
> new file mode 100644
> index 00000000000..a01ba2a9340
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/condbreak-bad.exp
> @@ -0,0 +1,40 @@
> +# Copyright 2020 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/>.
> +
> +# Test defining bad conditions for breakpoints.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" ${binfile} ${srcfile}]} {
> +    return
> +}
> +
> +set bp_location [gdb_get_line_number "break-here"]
> +gdb_breakpoint "$bp_location"
> +set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"]
> +
> +# Define a 'bad' condition.  The breakpoint should stay unconditional.
> +gdb_test "cond $bpnum gibberish" \
> +    "No symbol \"gibberish\" in current context." \
> +    "attempt a bad condition"
> +
> +set fill "\[^\r\n\]*"
> +
> +gdb_test "info break" \
> +    [multi_line \
> +	 "Num${fill}What" \
> +	 "${decimal}${fill}breakpoint${fill}keep y${fill}:${bp_location}"] \
> +    "breakpoint is unconditional"
> +

Here, could you also test that when a valid condition exists and we try to change
it for a bad one, the previous condition is still there after the failed attempt
to change it?

Simon

Re: [PATCH 1/3] gdb/breakpoint: do not update the condition string if parsing the condition fails

[Simon Marchi]

On 2020-07-22 9:12 a.m., Simon Marchi wrote:
> Here, could you also test that when a valid condition exists and we try to change
> it for a bad one, the previous condition is still there after the failed attempt
> to change it?
> 
> Simon
> 

Ah sorry, I read patches in a very linear way, so I did not realize this was added
in the next patch.

Simon

Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Simon Marchi]

On 2020-06-29 9:48 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> 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-06-29  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-06-29  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* gdb.base/condbreak-bad.exp: Extend the test with scenarios
> 	that attempt to overwrite and existing condition with a condition
> 	that fails parsing and also with a condition that parses fine
> 	but contains junk at the end.
> ---
>  gdb/breakpoint.c                         | 46 +++++++------
>  gdb/testsuite/gdb.base/condbreak-bad.exp | 88 ++++++++++++++++++++++++
>  2 files changed, 112 insertions(+), 22 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 1fc2d1b8966..abda470fe21 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.  */
> +	    }
> +	}

Since you touch this, might as well declare the `bp_location *loc` in the for loop
and use `loc != nullptr`.

Otherwise, this LGTM.

Simon

Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Simon Marchi]

On 2020-07-22 9:21 a.m., Simon Marchi wrote:
> On 2020-06-29 9:48 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
>> 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-06-29  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-06-29  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
>>
>> 	* gdb.base/condbreak-bad.exp: Extend the test with scenarios
>> 	that attempt to overwrite and existing condition with a condition
>> 	that fails parsing and also with a condition that parses fine
>> 	but contains junk at the end.
>> ---
>>  gdb/breakpoint.c                         | 46 +++++++------
>>  gdb/testsuite/gdb.base/condbreak-bad.exp | 88 ++++++++++++++++++++++++
>>  2 files changed, 112 insertions(+), 22 deletions(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 1fc2d1b8966..abda470fe21 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.  */
>> +	    }
>> +	}
> 
> Since you touch this, might as well declare the `bp_location *loc` in the for loop
> and use `loc != nullptr`.

Again, this is taken care of in the next patch, so forget it :).

Although, in the breakpoint case, when we have:

	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
	    {
	      const char *arg = exp;
	      expression_up new_exp
		= parse_exp_1 (&arg, loc->address,
			       block_for_pc (loc->address), 0);
	      if (*arg != 0)
		error (_("Junk at end of expression"));
	      loc->cond = std::move (new_exp);
	    }

Doesn't that mean that if the expression succeeds to parse for one location and then
fails to parse for another location, we'll have updated one location and not the other?

How does that work (or should work) when we have a multi-location breakpoint and the
condition only makes sense in one of the locations?

Simon

RE: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Aktemur, Tankut Baris]

On Wednesday, July 22, 2020 3:28 PM, Simon Marchi wrote:
> Although, in the breakpoint case, when we have:
> 
> 	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
> 	    {
> 	      const char *arg = exp;
> 	      expression_up new_exp
> 		= parse_exp_1 (&arg, loc->address,
> 			       block_for_pc (loc->address), 0);
> 	      if (*arg != 0)
> 		error (_("Junk at end of expression"));
> 	      loc->cond = std::move (new_exp);
> 	    }
> 
> Doesn't that mean that if the expression succeeds to parse for one location and then
> fails to parse for another location, we'll have updated one location and not the other?

Ahh, yes.  The diff for the part above should have been:

          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"));
            }

> How does that work (or should work) when we have a multi-location breakpoint and the
> condition only makes sense in one of the locations?

I'm in fact working on a follow-up patch on this topic, where the two-pass approach above
is used (hence I forgot to include it in this series).

Currently, GDB expects the condition to be valid at all locations.  The patch that I'll
soon post proposes to accept the condition if there exist locations where it's valid.
The locations where the condition is invalid are disabled.  But in the current state, the
condition has to make sense at all locations.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Simon Marchi]

On 2020-07-22 11:29 a.m., Aktemur, Tankut Baris wrote:
> On Wednesday, July 22, 2020 3:28 PM, Simon Marchi wrote:
>> Although, in the breakpoint case, when we have:
>>
>> 	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
>> 	    {
>> 	      const char *arg = exp;
>> 	      expression_up new_exp
>> 		= parse_exp_1 (&arg, loc->address,
>> 			       block_for_pc (loc->address), 0);
>> 	      if (*arg != 0)
>> 		error (_("Junk at end of expression"));
>> 	      loc->cond = std::move (new_exp);
>> 	    }
>>
>> Doesn't that mean that if the expression succeeds to parse for one location and then
>> fails to parse for another location, we'll have updated one location and not the other?
> 
> Ahh, yes.  The diff for the part above should have been:
> 
>           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"));
>             }
> 
>> How does that work (or should work) when we have a multi-location breakpoint and the
>> condition only makes sense in one of the locations?
> 
> I'm in fact working on a follow-up patch on this topic, where the two-pass approach above
> is used (hence I forgot to include it in this series).
> 
> Currently, GDB expects the condition to be valid at all locations.  The patch that I'll
> soon post proposes to accept the condition if there exist locations where it's valid.
> The locations where the condition is invalid are disabled.  But in the current state, the
> condition has to make sense at all locations.

Ok, so do you want to wait and post everything together, or do still want to consider
merging this one on its own, since it's still a step forward?

Simon

Re: [PATCH 0/3] Prevent bad conditions from putting breakpoints into broken state

[Pedro Alves]

On 7/21/20 10:08 AM, Tankut Baris Aktemur via Gdb-patches wrote:
>> Hi,
>>
>> If the condition of a breakpoint is "bad" (e.g. has unresolved
>> symbols, syntax errors, etc.), the breakpoint goes into a broken state
>> where the condition string or the condition expressions are updated
>> inadvertently.  This small series attempts to fix that.
> 
> 
> Kindly pinging for
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-June/169954.html

I'm confused -- are you proposing to merge patch #2 as it was,
or with the two passes?

RE: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Aktemur, Tankut Baris]

On Wednesday, July 22, 2020 6:06 PM, Simon Marchi wrote:
> On 2020-07-22 11:29 a.m., Aktemur, Tankut Baris wrote:
> > On Wednesday, July 22, 2020 3:28 PM, Simon Marchi wrote:
> >> Although, in the breakpoint case, when we have:
> >>
> >> 	  for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next)
> >> 	    {
> >> 	      const char *arg = exp;
> >> 	      expression_up new_exp
> >> 		= parse_exp_1 (&arg, loc->address,
> >> 			       block_for_pc (loc->address), 0);
> >> 	      if (*arg != 0)
> >> 		error (_("Junk at end of expression"));
> >> 	      loc->cond = std::move (new_exp);
> >> 	    }
> >>
> >> Doesn't that mean that if the expression succeeds to parse for one location and then
> >> fails to parse for another location, we'll have updated one location and not the other?
> >
> > Ahh, yes.  The diff for the part above should have been:
> >
> >           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"));
> >             }
> >
> >> How does that work (or should work) when we have a multi-location breakpoint and the
> >> condition only makes sense in one of the locations?
> >
> > I'm in fact working on a follow-up patch on this topic, where the two-pass approach above
> > is used (hence I forgot to include it in this series).
> >
> > Currently, GDB expects the condition to be valid at all locations.  The patch that I'll
> > soon post proposes to accept the condition if there exist locations where it's valid.
> > The locations where the condition is invalid are disabled.  But in the current state, the
> > condition has to make sense at all locations.
> 
> Ok, so do you want to wait and post everything together, or do still want to consider
> merging this one on its own, since it's still a step forward?

I'd like to merge this on its own.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 0/3] Prevent bad conditions from putting breakpoints into broken state

[Aktemur, Tankut Baris]

On Wednesday, July 22, 2020 8:24 PM, Pedro Alves wrote:
> On 7/21/20 10:08 AM, Tankut Baris Aktemur via Gdb-patches wrote:
> >> Hi,
> >>
> >> If the condition of a breakpoint is "bad" (e.g. has unresolved
> >> symbols, syntax errors, etc.), the breakpoint goes into a broken state
> >> where the condition string or the condition expressions are updated
> >> inadvertently.  This small series attempts to fix that.
> >
> >
> > Kindly pinging for
> >
> >   https://sourceware.org/pipermail/gdb-patches/2020-June/169954.html
> 
> I'm confused -- are you proposing to merge patch #2 as it was,
> or with the two passes?

Sorry for the confusion.  Please consider the two-pass version.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Aktemur, Tankut Baris]

On Thursday, July 23, 2020 9:11 AM, Aktemur, Tankut Baris wrote:
> On Wednesday, July 22, 2020 6:06 PM, Simon Marchi wrote:
> > Ok, so do you want to wait and post everything together, or do still want to consider
> > merging this one on its own, since it's still a step forward?
> 
> I'd like to merge this on its own.

Hi Simon,

OK to push the two-pass version?

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Simon Marchi]

On 2020-07-30 6:56 a.m., Aktemur, Tankut Baris wrote:
> On Thursday, July 23, 2020 9:11 AM, Aktemur, Tankut Baris wrote:
>> On Wednesday, July 22, 2020 6:06 PM, Simon Marchi wrote:
>>> Ok, so do you want to wait and post everything together, or do still want to consider
>>> merging this one on its own, since it's still a step forward?
>>
>> I'd like to merge this on its own.
> 
> Hi Simon,
> 
> OK to push the two-pass version?
> 
> Thanks
> -Baris

Yes, go ahead.

Simon