[RFC,2/2] gdb/breakpoint: add a '-force' flag to the 'condition' command

Message ID 94a5e4dc4f144cc0f9c170bdabeec7c0cb4fda4d.1596209606.git.tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • Breakpoint conditions at locations with differing contexts
Related show

Commit Message

Hannes Domani via Gdb-patches July 31, 2020, 3:42 p.m.
The previous patch made it possible to define a condition if it's
valid at some locations.  If the condition is invalid at all of the
locations, it's rejected.  However, there may be cases where the user
knows the condition will be valid at a location in the *future*,
e.g. due to a shared library load.

To make it possible that such condition is defined, this patch
adds an optional '-force' flag to the 'condition' command.  When this
command is passed, the condition is not rejected even when it is
invalid for all the current locations (note that all the locations
would be internally disabled in this case).

For instance:

  (gdb) break test.c:5
  Breakpoint 1 at 0x1155: file test.c, line 5.
  (gdb) cond 1 foo == 42
  No symbol "foo" in current context.

Defining the condition was not possible because 'foo' is not
available.  The user can override this behavior with the '-force'
flag:

  (gdb) cond -force 1 foo == 42
  warning: disabling breakpoint 1.1: No symbol "foo" in current context.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
          stop only if foo == 42
  1.1                         n   0x0000000000001155 in main at test.c:5

Now the condition is accepted, but the location is automatically
disabled.  If a future location has a context in which 'foo' is
available, that location would be enabled.

This is an RFC.  If the idea of having this '-force' flag makes sense,
I could also work on the same feature for the 'break' command, write
test cases, and submit as a full patch.

Another potentially interesting approach is to query the user.
Something along the following idea:

  (gdb) cond 1 foo == 42
  Condition expression is invalid at all locations of breakpoint 1.
  Do you want force GDB to nevertheless define the condition, but
  disable the current locations? [y/N] y
  warning: disabling breakpoint 1.1: No symbol "foo" in current context.

This query approach may break some scripts because it is changing the
existing behavior.

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

	* breakpoint.h (set_breakpoint_condition): Add a new bool parameter.
	* breakpoint.c (set_breakpoint_condition): Take a new bool parameter
	to control whether condition definition should be forced even when
	the condition expression is invalid in all of the current locations.
	(condition_command): Update the call to 'set_breakpoint_condition'.
	* ada-lang.c (create_ada_exception_catchpoint): Ditto.
	* guile/scm-breakpoint.c (gdbscm_set_breakpoint_condition_x): Ditto.
	* python/py-breakpoint.c (bppy_set_condition): Ditto.

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

	* gdb.texinfo: Document the '-force' flag of the 'condition' command.
---
 gdb/ada-lang.c             |  2 +-
 gdb/breakpoint.c           | 28 ++++++++++++++++++++++------
 gdb/breakpoint.h           |  6 ++++--
 gdb/doc/gdb.texinfo        |  6 ++++++
 gdb/guile/scm-breakpoint.c |  2 +-
 gdb/python/py-breakpoint.c |  2 +-
 6 files changed, 35 insertions(+), 11 deletions(-)

-- 
2.17.1

Comments

Andrew Burgess Aug. 3, 2020, 10:28 a.m. | #1
* Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> [2020-07-31 17:42:29 +0200]:

> The previous patch made it possible to define a condition if it's

> valid at some locations.  If the condition is invalid at all of the

> locations, it's rejected.  However, there may be cases where the user

> knows the condition will be valid at a location in the *future*,

> e.g. due to a shared library load.

> 

> To make it possible that such condition is defined, this patch

> adds an optional '-force' flag to the 'condition' command.  When this

> command is passed, the condition is not rejected even when it is

> invalid for all the current locations (note that all the locations

> would be internally disabled in this case).

> 

> For instance:

> 

>   (gdb) break test.c:5

>   Breakpoint 1 at 0x1155: file test.c, line 5.

>   (gdb) cond 1 foo == 42

>   No symbol "foo" in current context.

> 

> Defining the condition was not possible because 'foo' is not

> available.  The user can override this behavior with the '-force'

> flag:

> 

>   (gdb) cond -force 1 foo == 42

>   warning: disabling breakpoint 1.1: No symbol "foo" in current context.

>   (gdb) info breakpoints

>   Num     Type           Disp Enb Address            What

>   1       breakpoint     keep y   <MULTIPLE>

>           stop only if foo == 42

>   1.1                         n   0x0000000000001155 in main at test.c:5

> 

> Now the condition is accepted, but the location is automatically

> disabled.  If a future location has a context in which 'foo' is

> available, that location would be enabled.

> 

> This is an RFC.  If the idea of having this '-force' flag makes sense,

> I could also work on the same feature for the 'break' command, write

> test cases, and submit as a full patch.


I'd be happy to see the `-force` flag added, I think adding flags like
this makes perfect sense.  There's already many flags like this for
the break command so I don't see why adding one more should be a
problem.

I'd be tempted to name the flag for `break` as `-force-condition` to
make it clear what's being forced.  The flags already accept unique
sub-strings, so `-force` would still work, plus there's always
tab-completion.

> 

> Another potentially interesting approach is to query the user.

> Something along the following idea:

> 

>   (gdb) cond 1 foo == 42

>   Condition expression is invalid at all locations of breakpoint 1.

>   Do you want force GDB to nevertheless define the condition, but

>   disable the current locations? [y/N] y

>   warning: disabling breakpoint 1.1: No symbol "foo" in current context.

> 

> This query approach may break some scripts because it is changing the

> existing behavior.


Unless the script is parsing the exact output of GDB this should be a
problem.  When GDB is run in an non-interactive manor the default
answer is assumed to prompts like this, so in the above case the user
would still get the `N` behaviour.

But I wouldn't rush to add more interactive prompts to GDB personally.

These are just my thoughts.  Others might feel differently.

Thanks,
Andrew


> 

> gdb/ChangeLog:

> 2020-07-31  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	* breakpoint.h (set_breakpoint_condition): Add a new bool parameter.

> 	* breakpoint.c (set_breakpoint_condition): Take a new bool parameter

> 	to control whether condition definition should be forced even when

> 	the condition expression is invalid in all of the current locations.

> 	(condition_command): Update the call to 'set_breakpoint_condition'.

> 	* ada-lang.c (create_ada_exception_catchpoint): Ditto.

> 	* guile/scm-breakpoint.c (gdbscm_set_breakpoint_condition_x): Ditto.

> 	* python/py-breakpoint.c (bppy_set_condition): Ditto.

> 

> gdb/doc/ChangeLog:

> 2020-07-31  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	* gdb.texinfo: Document the '-force' flag of the 'condition' command.

> ---

>  gdb/ada-lang.c             |  2 +-

>  gdb/breakpoint.c           | 28 ++++++++++++++++++++++------

>  gdb/breakpoint.h           |  6 ++++--

>  gdb/doc/gdb.texinfo        |  6 ++++++

>  gdb/guile/scm-breakpoint.c |  2 +-

>  gdb/python/py-breakpoint.c |  2 +-

>  6 files changed, 35 insertions(+), 11 deletions(-)

> 

> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c

> index 29951528e5e..69b3c3c63cd 100644

> --- a/gdb/ada-lang.c

> +++ b/gdb/ada-lang.c

> @@ -12702,7 +12702,7 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,

>    c->excep_string = excep_string;

>    create_excep_cond_exprs (c.get (), ex_kind);

>    if (!cond_string.empty ())

> -    set_breakpoint_condition (c.get (), cond_string.c_str (), from_tty);

> +    set_breakpoint_condition (c.get (), cond_string.c_str (), from_tty, false);

>    install_breakpoint (0, std::move (c), 1);

>  }

>  

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c

> index 7abfd510abc..5fe9358b381 100644

> --- a/gdb/breakpoint.c

> +++ b/gdb/breakpoint.c

> @@ -874,7 +874,7 @@ set_breakpoint_location_condition (const char *cond_string, bp_location *loc,

>  

>  void

>  set_breakpoint_condition (struct breakpoint *b, const char *exp,

> -			  int from_tty)

> +			  int from_tty, bool force)

>  {

>    if (*exp == 0)

>      {

> @@ -941,9 +941,11 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,

>  	      catch (const gdb_exception_error &e)

>  		{

>  		  /* Condition string is invalid.  If this happens to

> -		     be the last loc, abandon.  */

> +		     be the last loc, abandon (if not forced) or continue

> +		     (if forced).  */

>  		  if (loc->next == nullptr)

> -		    throw;

> +		    if (!force)

> +		      throw;

>  		}

>  	    }

>  

> @@ -1023,6 +1025,18 @@ condition_command (const char *arg, int from_tty)

>      error_no_arg (_("breakpoint number"));

>  

>    p = arg;

> +

> +  /* Check if the "-force" flag was passed.  */

> +  bool force = false;

> +  const char *tok = skip_spaces (p);

> +  const char *end_tok = skip_to_space (tok);

> +  int toklen = end_tok - tok;

> +  if (toklen >= 1 && strncmp (tok, "-force", toklen) == 0)

> +    {

> +      force = true;

> +      p = end_tok + 1;

> +    }

> +

>    bnum = get_number (&p);

>    if (bnum == 0)

>      error (_("Bad breakpoint argument: '%s'"), arg);

> @@ -1042,7 +1056,7 @@ condition_command (const char *arg, int from_tty)

>  		     " a %s stop condition defined for this breakpoint."),

>  		   ext_lang_capitalized_name (extlang));

>  	  }

> -	set_breakpoint_condition (b, p, from_tty);

> +	set_breakpoint_condition (b, p, from_tty, force);

>  

>  	if (is_breakpoint (b))

>  	  update_global_location_list (UGLL_MAY_INSERT);

> @@ -15576,8 +15590,10 @@ then no output is printed when it is hit, except what the commands print."));

>  

>    c = add_com ("condition", class_breakpoint, condition_command, _("\

>  Specify breakpoint number N to break only if COND is true.\n\

> -Usage is `condition N COND', where N is an integer and COND is an\n\

> -expression to be evaluated whenever breakpoint N is reached."));

> +Usage is `condition [-force] N COND', where N is an integer and COND\n\

> +is an expression to be evaluated whenever breakpoint N is reached.\n\

> +With the `-force` flag, the condition is defined even when it is\n\

> +invalid for all current locations."));

>    set_cmd_completer (c, condition_completer);

>  

>    c = add_com ("tbreak", class_breakpoint, tbreak_command, _("\

> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h

> index 145bc8a397a..6acec428a9a 100644

> --- a/gdb/breakpoint.h

> +++ b/gdb/breakpoint.h

> @@ -1617,9 +1617,11 @@ extern int breakpoints_should_be_inserted_now (void);

>     in our opinion won't ever trigger.  */

>  extern void breakpoint_retire_moribund (void);

>  

> -/* Set break condition of breakpoint B to EXP.  */

> +/* Set break condition of breakpoint B to EXP.

> +   If FORCE, define the condition even if it is invalid in

> +   all of the breakpoint locations.  */

>  extern void set_breakpoint_condition (struct breakpoint *b, const char *exp,

> -				      int from_tty);

> +				      int from_tty, bool force);

>  

>  /* Checks if we are catching syscalls or not.

>     Returns 0 if not, greater than 0 if we are.  */

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo

> index 1b9f76573b9..a81689dab21 100644

> --- a/gdb/doc/gdb.texinfo

> +++ b/gdb/doc/gdb.texinfo

> @@ -5453,6 +5453,12 @@ prints an error message:

>  No symbol "foo" in current context.

>  @end smallexample

>  

> +@item condition -force @var{bnum} @var{expression}

> +When the @code{-force} flag is used, define the condition even if

> +@var{expression} is invalid in all the current locations of breakpoint

> +@var{bpnum}.  This is useful if it is known that @var{expression} will

> +be valid for a future shared library location.

> +

>  @noindent

>  @value{GDBN} does

>  not actually evaluate @var{expression} at the time the @code{condition}

> diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c

> index 96b6ca91f8d..7c9707235ec 100644

> --- a/gdb/guile/scm-breakpoint.c

> +++ b/gdb/guile/scm-breakpoint.c

> @@ -905,7 +905,7 @@ gdbscm_set_breakpoint_condition_x (SCM self, SCM newvalue)

>  	   ? nullptr

>  	   : gdbscm_scm_to_c_string (newvalue));

>  

> -      set_breakpoint_condition (bp_smob->bp, exp ? exp.get () : "", 0);

> +      set_breakpoint_condition (bp_smob->bp, exp ? exp.get () : "", 0, false);

>  

>        return SCM_UNSPECIFIED;

>      });

> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c

> index 11345ad3052..56640421994 100644

> --- a/gdb/python/py-breakpoint.c

> +++ b/gdb/python/py-breakpoint.c

> @@ -461,7 +461,7 @@ bppy_set_condition (PyObject *self, PyObject *newvalue, void *closure)

>  

>    try

>      {

> -      set_breakpoint_condition (self_bp->bp, exp, 0);

> +      set_breakpoint_condition (self_bp->bp, exp, 0, false);

>      }

>    catch (gdb_exception &ex)

>      {

> -- 

> 2.17.1

>

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 29951528e5e..69b3c3c63cd 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12702,7 +12702,7 @@  create_ada_exception_catchpoint (struct gdbarch *gdbarch,
   c->excep_string = excep_string;
   create_excep_cond_exprs (c.get (), ex_kind);
   if (!cond_string.empty ())
-    set_breakpoint_condition (c.get (), cond_string.c_str (), from_tty);
+    set_breakpoint_condition (c.get (), cond_string.c_str (), from_tty, false);
   install_breakpoint (0, std::move (c), 1);
 }
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7abfd510abc..5fe9358b381 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -874,7 +874,7 @@  set_breakpoint_location_condition (const char *cond_string, bp_location *loc,
 
 void
 set_breakpoint_condition (struct breakpoint *b, const char *exp,
-			  int from_tty)
+			  int from_tty, bool force)
 {
   if (*exp == 0)
     {
@@ -941,9 +941,11 @@  set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	      catch (const gdb_exception_error &e)
 		{
 		  /* Condition string is invalid.  If this happens to
-		     be the last loc, abandon.  */
+		     be the last loc, abandon (if not forced) or continue
+		     (if forced).  */
 		  if (loc->next == nullptr)
-		    throw;
+		    if (!force)
+		      throw;
 		}
 	    }
 
@@ -1023,6 +1025,18 @@  condition_command (const char *arg, int from_tty)
     error_no_arg (_("breakpoint number"));
 
   p = arg;
+
+  /* Check if the "-force" flag was passed.  */
+  bool force = false;
+  const char *tok = skip_spaces (p);
+  const char *end_tok = skip_to_space (tok);
+  int toklen = end_tok - tok;
+  if (toklen >= 1 && strncmp (tok, "-force", toklen) == 0)
+    {
+      force = true;
+      p = end_tok + 1;
+    }
+
   bnum = get_number (&p);
   if (bnum == 0)
     error (_("Bad breakpoint argument: '%s'"), arg);
@@ -1042,7 +1056,7 @@  condition_command (const char *arg, int from_tty)
 		     " a %s stop condition defined for this breakpoint."),
 		   ext_lang_capitalized_name (extlang));
 	  }
-	set_breakpoint_condition (b, p, from_tty);
+	set_breakpoint_condition (b, p, from_tty, force);
 
 	if (is_breakpoint (b))
 	  update_global_location_list (UGLL_MAY_INSERT);
@@ -15576,8 +15590,10 @@  then no output is printed when it is hit, except what the commands print."));
 
   c = add_com ("condition", class_breakpoint, condition_command, _("\
 Specify breakpoint number N to break only if COND is true.\n\
-Usage is `condition N COND', where N is an integer and COND is an\n\
-expression to be evaluated whenever breakpoint N is reached."));
+Usage is `condition [-force] N COND', where N is an integer and COND\n\
+is an expression to be evaluated whenever breakpoint N is reached.\n\
+With the `-force` flag, the condition is defined even when it is\n\
+invalid for all current locations."));
   set_cmd_completer (c, condition_completer);
 
   c = add_com ("tbreak", class_breakpoint, tbreak_command, _("\
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 145bc8a397a..6acec428a9a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1617,9 +1617,11 @@  extern int breakpoints_should_be_inserted_now (void);
    in our opinion won't ever trigger.  */
 extern void breakpoint_retire_moribund (void);
 
-/* Set break condition of breakpoint B to EXP.  */
+/* Set break condition of breakpoint B to EXP.
+   If FORCE, define the condition even if it is invalid in
+   all of the breakpoint locations.  */
 extern void set_breakpoint_condition (struct breakpoint *b, const char *exp,
-				      int from_tty);
+				      int from_tty, bool force);
 
 /* Checks if we are catching syscalls or not.
    Returns 0 if not, greater than 0 if we are.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1b9f76573b9..a81689dab21 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -5453,6 +5453,12 @@  prints an error message:
 No symbol "foo" in current context.
 @end smallexample
 
+@item condition -force @var{bnum} @var{expression}
+When the @code{-force} flag is used, define the condition even if
+@var{expression} is invalid in all the current locations of breakpoint
+@var{bpnum}.  This is useful if it is known that @var{expression} will
+be valid for a future shared library location.
+
 @noindent
 @value{GDBN} does
 not actually evaluate @var{expression} at the time the @code{condition}
diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index 96b6ca91f8d..7c9707235ec 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -905,7 +905,7 @@  gdbscm_set_breakpoint_condition_x (SCM self, SCM newvalue)
 	   ? nullptr
 	   : gdbscm_scm_to_c_string (newvalue));
 
-      set_breakpoint_condition (bp_smob->bp, exp ? exp.get () : "", 0);
+      set_breakpoint_condition (bp_smob->bp, exp ? exp.get () : "", 0, false);
 
       return SCM_UNSPECIFIED;
     });
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 11345ad3052..56640421994 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -461,7 +461,7 @@  bppy_set_condition (PyObject *self, PyObject *newvalue, void *closure)
 
   try
     {
-      set_breakpoint_condition (self_bp->bp, exp, 0);
+      set_breakpoint_condition (self_bp->bp, exp, 0, false);
     }
   catch (gdb_exception &ex)
     {