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

["Aktemur, Tankut Baris via Gdb-patches" <gdb-patches@sourceware.org>]

On Friday, September 25, 2020 6:10 PM, Simon Marchi wrote:
> 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.

We can skip printing the warning but save the exception message inside the loc object.
Once we are done iterating the locations, we can go over the now-ordered list to print
the warnings together with the location numbers.  How does that sound?
 
> > 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.

OK, I'll check the GDB doc to see how it looks and will break the line
if necessary.
 
> >>>   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.

I don't have a strong preference.  The legend approach would be fine, too.
 
> > 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?

I had aimed to be consistent with the "... now unconditional" message, but I agree.
When hidden, such messages sometimes cost valuable time loss.  I'll remove the from_tty
guards I added.

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

Yes, it is exercised as one of the first tests.  The "break func if a == 10"
command goes through this path, while "cond 1 a == 10" goes through the bp_num != 0
path.

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