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

Aktemur, Tankut Baris via Gdb-patches
2020-09-25T15:50:13Z

On Saturday, September 19, 2020 5:06 AM, Simon Marchi wrote:
> 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.

I changed the example in the commit message and the test to use the following:

class Base
{
public:
int b = 20;

void func () {}
};

class A : public Base
{
public:
int a = 10;

void func () {}
};

class C : public Base
{
public:
int c = 30;

void func () {}
};

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

While revising the code, I noticed that when the breakpoint is being defined for
the first time using "break" command, the locations are re-ordered according to
their addresses. So, tracking and reporting the location number as we iterate over
SALs is useless. Instead, we can report the location address.

Based on this, how about the first option you gave above, but using "validate" instead
of "resolve"? For the "break" command, it reports the address in hex:

warning: failed to validate condition at location 0x1120, disabling: No symbol "a" in current context.

But for the "cond" command, the location number is used because it's stable.

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

Perhaps we can break the message at the comma to avoid this long line.

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

I like the capital N notation. Patch is updated.

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

Done, with a small change:

Breakpoint 1's condition is invalid at location 2, cannot enable.

OK with this?

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

I think it's useful to say a bit more because the user may have forgotten about
the condition. To be consistent with the message above, how about this:

Breakpoint 1's condition is now valid at location 2, enabling.

One additional note: I noticed that the existing "Breakpoint N is now unconditional"
message is guarded by 'from_tty'. I added the same guard to the "...enabling"
messages, too.

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

OK, sure. That makes sense.

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

It's 0 if the breakpoint is being defined for the first time using the
"break ... if ..." command. It's non-zero if defined previously and is now
becoming conditional.

> > @@ -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".

Fixed.

Thank you.
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928