[3/4] gdb/breakpoint: add a 'force_condition' parameter to 'create_breakpoint'

Message ID 40773bd923ed8cb1f9773ecd29a5af01e204d801.1617806598.git.tankut.baris.aktemur@intel.com
State Superseded
Headers show
Series
  • Multi-context invalid breakpoint conditions and MI
Related show

Commit Message

Mike Frysinger via Gdb-patches April 7, 2021, 2:55 p.m.
The 'create_breakpoint' function takes a 'parse_extra' argument that
determines whether the condition, thread, and force-condition
specifiers should be parsed from the extra string or be used from the
function arguments.  However, for the case when 'parse_extra' is
false, there is no way to pass the force-condition specifier.  This
patch adds it as a new argument.

Also, in the case when parse_extra is false, the current behavior is
as if the condition is being forced.  This is a bug.  The default
behavior should reject the breakpoint.  See below for a demo of this
incorrect behavior.  (The MI command '-break-insert' uses the
'create_breakpoint' function with parse_extra=0.)

  $ gdb -q --interpreter=mi2 /tmp/simple
  =thread-group-added,id="i1"
  =cmd-param-changed,param="history save",value="on"
  =cmd-param-changed,param="auto-load safe-path",value="/"
  ~"Reading symbols from /tmp/simple...\n"
  (gdb)
  -break-insert -c junk -f main
  &"warning: failed to validate condition at location 1, disabling:\n  "
  &"No symbol \"junk\" in current context.\n"
  ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main"},{number="1.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}
  (gdb)
  break main if junk
  &"break main if junk\n"
  &"No symbol \"junk\" in current context.\n"
  ^error,msg="No symbol \"junk\" in current context."
  (gdb)
  break main -force-condition if junk
  &"break main -force-condition if junk\n"
  ~"Note: breakpoint 1 also set at pc 0x114e.\n"
  &"warning: failed to validate condition at location 1, disabling:\n  "
  &"No symbol \"junk\" in current context.\n"
  ~"Breakpoint 2 at 0x114e: file simple.c, line 2.\n"
  =breakpoint-created,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main"},{number="2.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}
  ^done
  (gdb)

After applying this patch, we get the behavior below:

  (gdb)
  -break-insert -c junk -f main
  ^error,msg="No symbol \"junk\" in current context."

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

	* breakpoint.h (create_breakpoint): Add a new parameter,
	'force_condition'.
	* breakpoint.c (create_breakpoint): Use the 'force_condition'
	argument when 'parse_extra' is false to check if the condition
	is invalid at all of the breakpoint locations.
	Update the users below.
	(break_command_1)
	(dprintf_command)
	(trace_command)
	(ftrace_command)
	(strace_command)
	(create_tracepoint_from_upload): Update.
	* guile/scm-breakpoint.c (gdbscm_register_breakpoint_x): Update.
	* mi/mi-cmd-break.c (mi_cmd_break_insert_1): Update.
	* python/py-breakpoint.c (bppy_init): Update.
	* python/py-finishbreakpoint.c (bpfinishpy_init): Update.

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

	* gdb.mi/mi-break.exp: Extend with checks for invalid breakpoint
	conditions.
---
 gdb/breakpoint.c                  | 40 ++++++++++++++++++++++++++-----
 gdb/breakpoint.h                  |  6 +++++
 gdb/guile/scm-breakpoint.c        |  2 +-
 gdb/mi/mi-cmd-break.c             |  1 +
 gdb/python/py-breakpoint.c        |  2 +-
 gdb/python/py-finishbreakpoint.c  |  2 +-
 gdb/testsuite/gdb.mi/mi-break.exp | 13 ++++++++++
 7 files changed, 57 insertions(+), 9 deletions(-)

-- 
2.17.1

Comments

Mike Frysinger via Gdb-patches April 7, 2021, 10:08 p.m. | #1
On 2021-04-07 10:55 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> The 'create_breakpoint' function takes a 'parse_extra' argument that

> determines whether the condition, thread, and force-condition

> specifiers should be parsed from the extra string or be used from the

> function arguments.  However, for the case when 'parse_extra' is

> false, there is no way to pass the force-condition specifier.  This

> patch adds it as a new argument.

> 

> Also, in the case when parse_extra is false, the current behavior is

> as if the condition is being forced.  This is a bug.  The default

> behavior should reject the breakpoint.  See below for a demo of this

> incorrect behavior.  (The MI command '-break-insert' uses the

> 'create_breakpoint' function with parse_extra=0.)

> 

>   $ gdb -q --interpreter=mi2 /tmp/simple

>   =thread-group-added,id="i1"

>   =cmd-param-changed,param="history save",value="on"

>   =cmd-param-changed,param="auto-load safe-path",value="/"

>   ~"Reading symbols from /tmp/simple...\n"

>   (gdb)

>   -break-insert -c junk -f main

>   &"warning: failed to validate condition at location 1, disabling:\n  "

>   &"No symbol \"junk\" in current context.\n"

>   ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main"},{number="1.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}

>   (gdb)

>   break main if junk

>   &"break main if junk\n"

>   &"No symbol \"junk\" in current context.\n"

>   ^error,msg="No symbol \"junk\" in current context."

>   (gdb)

>   break main -force-condition if junk

>   &"break main -force-condition if junk\n"

>   ~"Note: breakpoint 1 also set at pc 0x114e.\n"

>   &"warning: failed to validate condition at location 1, disabling:\n  "

>   &"No symbol \"junk\" in current context.\n"

>   ~"Breakpoint 2 at 0x114e: file simple.c, line 2.\n"

>   =breakpoint-created,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main"},{number="2.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}

>   ^done

>   (gdb)

> 

> After applying this patch, we get the behavior below:

> 

>   (gdb)

>   -break-insert -c junk -f main

>   ^error,msg="No symbol \"junk\" in current context."


You can also mention that this restores the previous behavior (present
in existing releases).

> diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp

> index 9d3d7ade6dc..ac13e4d1e09 100644

> --- a/gdb/testsuite/gdb.mi/mi-break.exp

> +++ b/gdb/testsuite/gdb.mi/mi-break.exp

> @@ -224,6 +224,19 @@ proc test_error {} {

>      mi_gdb_test "-break-insert -c i==4 \"callme if i < 4\"" \

>          ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \

>          "conditional breakpoint with garbage after location"

> +

> +    # Try using an invalid condition.

> +    mi_gdb_test "-break-insert -c bad callme" \

> +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \

> +        "breakpoint with bad condition"

> +

> +    mi_gdb_test "-dprintf-insert -c bad callme 123" \

> +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \

> +        "dprintf with bad condition"

> +

> +    mi_gdb_test "-break-condition 5 bad" \

> +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \

> +        "invalid condition"


Here, does the 5 refer to an existing breakpoint number created by
another test function?  It's a bit annoying when test functions rely on
the state left by other test functions, that makes them harder to debug
in isolation.  In my ideal world, each test_* function in this file
(except test_break, which is just the driver) would spawn a fresh GDB,
such that if you need to debug one of them, you can comment out all the
other ones.

The patch is still OK though, and thanks for taking the time to write a
test.

Simon
Mike Frysinger via Gdb-patches April 8, 2021, 7:44 a.m. | #2
On Thursday, April 8, 2021 12:09 AM, Simon Marchi wrote:
> On 2021-04-07 10:55 a.m., Tankut Baris Aktemur via Gdb-patches wrote:

> > diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp

> > index 9d3d7ade6dc..ac13e4d1e09 100644

> > --- a/gdb/testsuite/gdb.mi/mi-break.exp

> > +++ b/gdb/testsuite/gdb.mi/mi-break.exp

> > @@ -224,6 +224,19 @@ proc test_error {} {

> >      mi_gdb_test "-break-insert -c i==4 \"callme if i < 4\"" \

> >          ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \

> >          "conditional breakpoint with garbage after location"

> > +

> > +    # Try using an invalid condition.

> > +    mi_gdb_test "-break-insert -c bad callme" \

> > +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \

> > +        "breakpoint with bad condition"

> > +

> > +    mi_gdb_test "-dprintf-insert -c bad callme 123" \

> > +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \

> > +        "dprintf with bad condition"

> > +

> > +    mi_gdb_test "-break-condition 5 bad" \

> > +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \

> > +        "invalid condition"

> 

> Here, does the 5 refer to an existing breakpoint number created by

> another test function?  It's a bit annoying when test functions rely on

> the state left by other test functions, that makes them harder to debug

> in isolation.  In my ideal world, each test_* function in this file

> (except test_break, which is just the driver) would spawn a fresh GDB,

> such that if you need to debug one of them, you can comment out all the

> other ones.


Ack.  The other test functions also refer to hardcoded breakpoint numbers.
Fiddling with the breakpoint list by deleting/adding new breakpoints
caused the other tests to fail, and thus I simply opted for the easier
solution of hardcoding.

> The patch is still OK though, and thanks for taking the time to write a

> test.

> 

> Simon


Thanks.
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Mike Frysinger via Gdb-patches April 8, 2021, 1:59 p.m. | #3
On 2021-04-08 3:44 a.m., Aktemur, Tankut Baris wrote:
> Ack.  The other test functions also refer to hardcoded breakpoint numbers.

> Fiddling with the breakpoint list by deleting/adding new breakpoints

> caused the other tests to fail, and thus I simply opted for the easier

> solution of hardcoding.


And I don't want to put on you the burden of doing this change, so I
think your patch is OK.  But this would be an item on the wishlist, and
I mentioned it because it can perhaps influence the future tests you
write from scratch (if you agree with the idea).

Simon
Mike Frysinger via Gdb-patches April 8, 2021, 2:19 p.m. | #4
On Thursday, April 8, 2021 4:00 PM, Simon Marchi wrote:
> On 2021-04-08 3:44 a.m., Aktemur, Tankut Baris wrote:

> > Ack.  The other test functions also refer to hardcoded breakpoint numbers.

> > Fiddling with the breakpoint list by deleting/adding new breakpoints

> > caused the other tests to fail, and thus I simply opted for the easier

> > solution of hardcoding.

> 

> And I don't want to put on you the burden of doing this change, so I

> think your patch is OK.  But this would be an item on the wishlist, and

> I mentioned it because it can perhaps influence the future tests you

> write from scratch (if you agree with the idea).

> 

> Simon


Thank you for your empathy.  Yes, I agree with the idea -- it's hard
to argue against it :)

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 16cf7977b62..45a09719392 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9456,7 +9456,7 @@  create_breakpoint (struct gdbarch *gdbarch,
 		   struct event_location *location,
 		   const char *cond_string,
 		   int thread, const char *extra_string,
-		   int parse_extra,
+		   bool force_condition, int parse_extra,
 		   int tempflag, enum bptype type_wanted,
 		   int ignore_count,
 		   enum auto_boolean pending_break_support,
@@ -9554,6 +9554,33 @@  create_breakpoint (struct gdbarch *gdbarch,
 	      && extra_string != NULL && *extra_string != '\0')
 		error (_("Garbage '%s' at end of location"), extra_string);
 
+	  /* Check the validity of the condition.  We should error out
+	     if the condition is invalid at all of the locations and
+	     if it is not forced.  In the PARSE_EXTRA case above, this
+	     check is done when parsing the EXTRA_STRING.  */
+	  if (cond_string != nullptr && !force_condition)
+	    {
+	      int num_failures = 0;
+	      const linespec_sals &lsal = canonical.lsals[0];
+	      for (auto &sal : lsal.sals)
+		{
+		  const char *cond = cond_string;
+		  try
+		    {
+		      parse_exp_1 (&cond, sal.pc, block_for_pc (sal.pc), 0);
+		      /* One success is sufficient to keep going.  */
+		      break;
+		    }
+		  catch (const gdb_exception_error &)
+		    {
+		      num_failures++;
+		      /* If this is the last sal, error out.  */
+		      if (num_failures == lsal.sals.size ())
+			throw;
+		    }
+		}
+	    }
+
 	  /* Create a private copy of condition string.  */
 	  if (cond_string)
 	    cond_string_copy.reset (xstrdup (cond_string));
@@ -9632,7 +9659,7 @@  break_command_1 (const char *arg, int flag, int from_tty)
 
   create_breakpoint (get_current_arch (),
 		     location.get (),
-		     NULL, 0, arg, 1 /* parse arg */,
+		     NULL, 0, arg, false, 1 /* parse arg */,
 		     tempflag, type_wanted,
 		     0 /* Ignore count */,
 		     pending_break_support,
@@ -9819,7 +9846,7 @@  dprintf_command (const char *arg, int from_tty)
 
   create_breakpoint (get_current_arch (),
 		     location.get (),
-		     NULL, 0, arg, 1 /* parse arg */,
+		     NULL, 0, arg, false, 1 /* parse arg */,
 		     0, bp_dprintf,
 		     0 /* Ignore count */,
 		     pending_break_support,
@@ -14717,7 +14744,7 @@  trace_command (const char *arg, int from_tty)
 
   create_breakpoint (get_current_arch (),
 		     location.get (),
-		     NULL, 0, arg, 1 /* parse arg */,
+		     NULL, 0, arg, false, 1 /* parse arg */,
 		     0 /* tempflag */,
 		     bp_tracepoint /* type_wanted */,
 		     0 /* Ignore count */,
@@ -14735,7 +14762,7 @@  ftrace_command (const char *arg, int from_tty)
 							 current_language);
   create_breakpoint (get_current_arch (),
 		     location.get (),
-		     NULL, 0, arg, 1 /* parse arg */,
+		     NULL, 0, arg, false, 1 /* parse arg */,
 		     0 /* tempflag */,
 		     bp_fast_tracepoint /* type_wanted */,
 		     0 /* Ignore count */,
@@ -14769,7 +14796,7 @@  strace_command (const char *arg, int from_tty)
 
   create_breakpoint (get_current_arch (),
 		     location.get (),
-		     NULL, 0, arg, 1 /* parse arg */,
+		     NULL, 0, arg, false, 1 /* parse arg */,
 		     0 /* tempflag */,
 		     bp_static_tracepoint /* type_wanted */,
 		     0 /* Ignore count */,
@@ -14839,6 +14866,7 @@  create_tracepoint_from_upload (struct uploaded_tp *utp)
   if (!create_breakpoint (get_current_arch (),
 			  location.get (),
 			  utp->cond_string.get (), -1, addr_str,
+			  false /* force_condition */,
 			  0 /* parse cond/thread */,
 			  0 /* tempflag */,
 			  utp->type /* type_wanted */,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index c30656971bb..ded498f5562 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1409,6 +1409,11 @@  enum breakpoint_create_flags
    the condition, thread, and extra string from EXTRA_STRING, ignoring
    the similarly named parameters.
 
+   If FORCE_CONDITION is true, the condition is accepted even when it is
+   invalid at all of the locations.  However, if PARSE_EXTRA is non-zero,
+   the FORCE_CONDITION parameter is ignored and the corresponding argument
+   is parsed from EXTRA_STRING.
+
    If INTERNAL is non-zero, the breakpoint number will be allocated
    from the internal breakpoint count.
 
@@ -1418,6 +1423,7 @@  extern int create_breakpoint (struct gdbarch *gdbarch,
 			      struct event_location *location,
 			      const char *cond_string, int thread,
 			      const char *extra_string,
+			      bool force_condition,
 			      int parse_extra,
 			      int tempflag, enum bptype wanted_type,
 			      int ignore_count,
diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index 99098e7e155..af63893461b 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -440,7 +440,7 @@  gdbscm_register_breakpoint_x (SCM self)
 	    const breakpoint_ops *ops =
 	      breakpoint_ops_for_event_location (eloc.get (), false);
 	    create_breakpoint (get_current_arch (),
-			       eloc.get (), NULL, -1, NULL,
+			       eloc.get (), NULL, -1, NULL, false,
 			       0,
 			       0, bp_breakpoint,
 			       0,
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 1c3e9209a74..5a4a62ce8c3 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -353,6 +353,7 @@  mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
 
   create_breakpoint (get_current_arch (), location.get (), condition, thread,
 		     extra_string.c_str (),
+		     false,
 		     0 /* condition and thread are valid.  */,
 		     temp_p, type_wanted,
 		     ignore_count,
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 3fbb1c633ff..9650bd023b5 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -835,7 +835,7 @@  bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	      breakpoint_ops_for_event_location (location.get (), false);
 
 	    create_breakpoint (python_gdbarch,
-			       location.get (), NULL, -1, NULL,
+			       location.get (), NULL, -1, NULL, false,
 			       0,
 			       temporary_bp, type,
 			       0,
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index d01b84273fc..38b4cc67901 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -294,7 +294,7 @@  bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
       event_location_up location
 	= new_address_location (get_frame_pc (prev_frame), NULL, 0);
       create_breakpoint (python_gdbarch,
-			 location.get (), NULL, thread, NULL,
+			 location.get (), NULL, thread, NULL, false,
 			 0,
 			 1 /*temp_flag*/,
 			 bp_breakpoint,
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index 9d3d7ade6dc..ac13e4d1e09 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -224,6 +224,19 @@  proc test_error {} {
     mi_gdb_test "-break-insert -c i==4 \"callme if i < 4\"" \
         ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \
         "conditional breakpoint with garbage after location"
+
+    # Try using an invalid condition.
+    mi_gdb_test "-break-insert -c bad callme" \
+        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \
+        "breakpoint with bad condition"
+
+    mi_gdb_test "-dprintf-insert -c bad callme 123" \
+        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \
+        "dprintf with bad condition"
+
+    mi_gdb_test "-break-condition 5 bad" \
+        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \
+        "invalid condition"
 }
 
 proc test_disabled_creation {} {