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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Friday, September 25, 2020 8:15 PM, Aktemur, Tankut Baris wrote:
> 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?

In the next revision (v4), I moved the part that sets the location's condition out of the
loop that iterates the `sals`, and added a second loop over the locations.  This way, we
will be iterating a stabilized list of locations where the positional numbers are correct.
So, the next revision does not print hex addresses anymore.
 
> > > 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.

It did not look nice in gdb.pdf.  I broke the line and indented the error
message.  It looks like this in v4:

  warning: failed to validate condition at location 2, disabling:
    No symbol "a" in current context.
 
> > >>>   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.

I went with this legend approach in v4.
 
> > > 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.

from_tty is removed in v4.
 
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