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

Message ID a84581d9c13ee6753ad1535fb7cfecbd3a961287.1593438119.git.tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • Prevent bad conditions from putting breakpoints into broken state
Related show

Commit Message

Jose E. Marchesi via Gdb-patches June 29, 2020, 1:48 p.m.
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

-- 
2.17.1

Comments

Simon Marchi July 22, 2020, 1:12 p.m. | #1
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
Simon Marchi July 22, 2020, 1:15 p.m. | #2
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

Patch

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"
+