Re: [PATCH v2 1/2] gdb/breakpoint: disable a bp location if condition is invalid at that location

[Simon Marchi <simark@simark.ca>]

Hi Baris,

I don't have much time right now, but I also don't want to delay looking
at this further, so here are a few comments.  Overall, I think that this
is very well thought through and I like the behavior.

On 2020-08-20 5:24 p.m., Tankut Baris Aktemur via Gdb-patches wrote:
> Currently, for a conditional breakpoint, GDB checks if the condition
> can be evaluated in the context of the first symtab and line (SAL).
> In case of an error, defining the conditional breakpoint is aborted.
> This prevents having a conditional breakpoint whose condition may
> actually be meaningful for some of the location contexts.  This patch
> makes it possible to define conditional BPs by checking all location
> contexts.  If the condition is meaningful for even one context, the
> breakpoint is defined.  The locations for which the condition gives
> errors are disabled.
>
> The bp_location struct is introduced a new field, 'disabled_by_cond'.
> This field denotes whether the location is disabled automatically
> because the condition was non-evaluatable.  Disabled-by-cond locations
> cannot be enabled by the user.  But locations that are not
> disabled-by-cond can be enabled/disabled by the user manually as
> before.
>
> For a concrete example, consider 3 contexts; func1, func2, and func3.
> Each context contains instructions coming from the same source.
> For instance:
>
>   void
>   func1 (int *value)
>   {
>     int a = 10;
>     value += a;
>     int b = 1;
>   #include "included.c"
>   }
>
>   void
>   func2 (int *value)
>   {
>     int b = 2;
>   #include "included.c"
>   }
>
>   void
>   func3 (int *value)
>   {
>     int c = 30;
>     value += c;
>     int b = 3;
>   #include "included.c"
>   }

A C++ example with overloaded functions would be simpler to understand,
I think.

>
> Note that
>
> * the variable 'a' is defined only in the context defined by func1.
> * the variable 'c' is defined only in the context defined by func3.
> * the variable 'b' is defined in all the three contexts.
>
> With the existing GDB, it's not possible to define a conditional
> breakpoint at "included.c:1" if the condition refers to 'a' or 'c':
>
>   (gdb) break included.c:1 if a == 10
>   No symbol "a" in current context.
>   (gdb) break included.c:1 if c == 30
>   No symbol "c" in current context.
>   (gdb) info breakpoints
>   No breakpoints or watchpoints.
>
> With this patch, it becomes possible:
>
>   (gdb) break included.c:1 if a == 10
>   warning: disabling breakpoint location 2: No symbol "a" in current context.
>   warning: disabling breakpoint location 3: No symbol "a" in current context.

Though it would make the message a bit longer, I think it would be
important to mention in it that the location is disabled because GDB
failed to resolve (is the right word?) the condition at this location.

Most of the time it will be obvious, but I'm sure that in some cases it
won't be, and the user will be wondering what that error on the right is
about.  Just one example, if the condition expression involves a macro
that hides some identifiers:

  #define SOME_EXPR s

and

  (gdb) b bar if SOME_EXPR
  warning: disabling breakpoint location 2: No symbol "s" in current context.
  Breakpoint 1 at 0x1120: bar. (2 locations)
  (gdb) i b
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
          stop only if SOME_EXPR
  1.1                         n   0x0000000000001120 in bar(int) at test.c:11
  1.2                         y   0x000000000000112b in bar(char const*) at test.c:15

I'm not sure what format is better:

  warning: failed to resolve condition at location 2, disabing: No symbol "a" in current context.

vs

  disabling breakpoint location 2: failed to resolve condition: No symbol "a" in current context.

vs

  warning: failed to resolve condition at location 2: No symbol "a" in current context.
  warning: disabling breakpoint location 2.

It's a bit annoying that the exception message is capitalized and has a
period, otherwise this would have been a good candidate:

  warning: failed to resolve condition at location 2 (no symbol "a" in current context), disabling.

>   Breakpoint 1 at 0x117d: included.c:1. (3 locations)
>   (gdb) break included.c:1 if c == 30
>   Note: breakpoint 1 also set at pc 0x117d.
>   warning: disabling breakpoint location 1: No symbol "c" in current context.
>   Note: breakpoint 1 also set at pc 0x119c.
>   warning: disabling breakpoint location 2: No symbol "c" in current context.
>   Note: breakpoint 1 also set at pc 0x11cf.
>   Breakpoint 2 at 0x117d: included.c:1. (3 locations)
>   (gdb) info break
>   Num     Type           Disp Enb Address            What
>   1       breakpoint     keep y   <MULTIPLE>
>           stop only if a == 10
>   1.1                         y   0x000000000000117d in func1 at included.c:1
>   1.2                         n   0x000000000000119c in func2 at included.c:1
>   1.3                         n   0x00000000000011cf in func3 at included.c:1
>   2       breakpoint     keep y   <MULTIPLE>
>           stop only if c == 30
>   2.1                         n   0x000000000000117d in func1 at included.c:1
>   2.2                         n   0x000000000000119c in func2 at included.c:1
>   2.3                         y   0x00000000000011cf in func3 at included.c:1

Should we somehow show in the listing that the locations disabled
because of the condition are disabled and can't be enabled?  For
example, a capital N in the "Enb" column?

>
> Executing the code hits the breakpoints 1.1 and 2.3 as expected.
>
> Defining a condition on an unconditional breakpoint gives the same
> behavior above:
>
>   (gdb) break included.c:1
>   Breakpoint 1 at 0x117d: included.c:1. (3 locations)
>   (gdb) cond 1 a == 10
>   warning: disabling breakpoint 1.2: No symbol "a" in current context.
>   warning: disabling breakpoint 1.3: No symbol "a" in current context.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   1       breakpoint     keep y   <MULTIPLE>
>           stop only if a == 10
>   1.1                         y   0x000000000000117d in func1 at included.c:1
>   1.2                         n   0x000000000000119c in func2 at included.c:1
>   1.3                         n   0x00000000000011cf in func3 at included.c:1
>
> Locations that are disabled because of a condition cannot be enabled
> by the user:
>
>   ...
>   (gdb) enable 1.2
>   Location is disabled because of the condition; cannot enable manually.

"because of the condition" does not sound clear to me.  It does not say
what about the condition prevents the location from getting enabled.
What about something like "Breakpoint condition is invalid at location
1.2, cannot enable".  Ideally, we should make sure that the messages we
output at different times should use the same vocabulary.

>
> Resetting the condition enables the locations back:
>
>   ...
>   (gdb) cond 1
>   Breakpoint 1.2 is now enabled.
>   Breakpoint 1.3 is now enabled.

Likewise, this doesn't say why these locations suddenly get enabled.
Should it?  Something like "Breakpoint condition now resolves at
location 1.2, enabling.".  Or is it obvious, because the user is already
using the "condition" command?

>   Breakpoint 1 now unconditional.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   1       breakpoint     keep y   <MULTIPLE>
>   1.1                         y   0x000000000000117d in func1 at included.c:1
>   1.2                         y   0x000000000000119c in func2 at included.c:1
>   1.3                         y   0x00000000000011cf in func3 at included.c:1
>
> If a location is disabled by the user, a condition can still be defined
> but the location will remain disabled even if the condition is meaningful
> for the disabled location:
>
>   ...
>   (gdb) disable 1.1
>   (gdb) cond 1 a == 10
>   warning: disabling breakpoint 1.2: No symbol "a" in current context.
>   warning: disabling breakpoint 1.3: No symbol "a" in current context.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   1       breakpoint     keep y   <MULTIPLE>
>           stop only if a == 10
>   1.1                         n   0x000000000000117d in func1 at included.c:1
>   1.2                         n   0x000000000000119c in func2 at included.c:1
>   1.3                         n   0x00000000000011cf in func3 at included.c:1

If a location is already user-disabled and we install a condition that
condition-disables it, we print a "disabling" message:

    (gdb) b bar
    Breakpoint 1 at 0x1120: bar. (2 locations)
    (gdb) disable 1.1 1.2
    (gdb) i b
    Num     Type           Disp Enb Address            What
    1       breakpoint     keep y   <MULTIPLE>
    1.1                         n   0x0000000000001120 in bar(int) at test.c:9
    1.2                         n   0x000000000000112b in bar(char const*) at test.c:13
    (gdb) cond 1 s
    warning: disabling breakpoint 1.1: No symbol "s" in current context.
    (gdb) i b
    Num     Type           Disp Enb Address            What
    1       breakpoint     keep y   <MULTIPLE>
            stop only if s
    1.1                         n   0x0000000000001120 in bar(int) at test.c:9
    1.2                         n   0x000000000000112b in bar(char const*) at test.c:13

Should the "disabling breakpoint" message not be printed here, since it
doesn't really make sense to disable an already disabled breakpoint?  If
the user tries to enable it back, then they'll get the "can't enable"
message.

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 977599db1db..7abfd510abc 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -830,6 +830,48 @@ get_first_locp_gte_addr (CORE_ADDR address)
>    return locp_found;
>  }
>
> +/* Parse COND_STRING in the context of LOC and set as the condition
> +   expression of LOC.  BP_NUM is the number of LOC's owner, LOC_NUM is
> +   the number of LOC within its owner.  In case of parsing error, mark
> +   LOC as DISABLED_BY_COND.  In case of success, unset DISABLED_BY_COND.  */
> +
> +static void
> +set_breakpoint_location_condition (const char *cond_string, bp_location *loc,
> +				   int bp_num, int loc_num)
> +{
> +  bool has_junk = false;
> +  try
> +    {
> +      expression_up new_exp = parse_exp_1 (&cond_string, loc->address,
> +					   block_for_pc (loc->address), 0);
> +      if (*cond_string != 0)
> +	has_junk = true;
> +      else
> +	{
> +	  loc->cond = std::move (new_exp);
> +	  if (loc->disabled_by_cond && loc->enabled)
> +	    printf_filtered (_("Breakpoint %d.%d is now enabled.\n"),
> +			     bp_num, loc_num);
> +
> +	  loc->disabled_by_cond = false;
> +	}
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      if (bp_num != 0)
> +	warning (_("disabling breakpoint %d.%d: %s"),
> +		 bp_num, loc_num, e.what ());
> +      else
> +	warning (_("disabling breakpoint location %d: %s"),
> +		 loc_num, e.what ());

When is bp_num 0?

> @@ -14157,6 +14258,10 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable)
>    struct bp_location *loc = find_location_by_number (bp_num, loc_num);
>    if (loc != NULL)
>      {
> +      if (loc->disabled_by_cond && enable)
> +	error(_("Location is disabled because of the condition; "

Space after "error".

Simon