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

Simon Marchi
2020-09-25T16:10:17Z

On 2020-09-25 11:49 a.m., Aktemur, Tankut Baris via Gdb-patches wrote:
> 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.

Ok, it would be nicer if we could refer to location numbers at the point
where we validate the conditions, it would make a more consistent
experience, but that works for now.

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

I don't mind... as long as it's clear that it's one message broken on
two lines, not two message.

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

Ok. Honestly, I find it a bit a bit cryptic, but I don't see a better
way without being overly verbose. Perhaps a legend like we have on info
shared would help?

(*): Shared library is missing debugging information.

> Done, with a small change:
>
> Breakpoint 1's condition is invalid at location 2, cannot enable.
>
> OK with this?

Sounds good!

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

Sounds good too.

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

Do you see any reason for that? Even if these commands are executed in
a script, I'd like to be notified about these changes. What do you
think?

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

Ok. And is this warning (when bp_num is 0) exercised in the tests? I
don't remember seeing it as a test expected output, but maybe I just
missed it.

Simon