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-10-13T12:25:16Z

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