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

[Simon Marchi <simark@simark.ca>]

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