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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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
>