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

Andrew Burgess
2020-08-03T10:28:30Z

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