Re: [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output

Joel Brobecker
2018-06-28T17:42:08Z

Hi Pedro,

> While experimenting with the previous patch, I noticed this inconsistency
> in GDB's output:
[...]
> When we run to the breakpoint, we present the stop at the same line
> number, and correctly show "func1" as the function name (2).
>
> But in "info break" output (3), notice that we say "in main", not "in
> func1".

> (It seems odd to me that block_linkage_function says "the CONTAINING
> function will be returned", and then block_containing_function says it
> returns "the closest enclosing function". Something seems reversed
> here. Still, I've kept the same nomenclature and copied the comments,
> so that at least there's consistency. Maybe we should fix that up
> somehow.)

That seems opposite to me as well...

> Then I wondered, why make print_breakpoint_location look up the symbol
> every time it is called, instead of just always storing the symbol
> when the location is created, since the location already stores the
> symbol in some cases.

Agreed.

> So to find which cases might be missing setting
> the symbol in the sal which is used to create the breakpoint location,
> I added an assertion to print_breakpoint_location, and ran the
> testsuite. That caught a few places, unsurprisingly:
>
> - setting a breakpoint by line number
> - setting a breapoint by address
> - ifunc resolving
>
> Those are all fixed by this commit.

Nice approach!

> I decided not to add the
> assertion to block_linkage_function and leave the existing "if (sym)"
> check in place, because it's plausible that we have symtabs with line
> info but no symbols. I.e., that would not be a GDB bug, but
> a peculiarity of debug info input.

Agreed as well.

> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * blockframe.c (find_pc_sect_containing_function): New function.
> * breakpoint.c (print_breakpoint_location): Don't call
> find_pc_sect_function.
> * linespec.c (create_sals_line_offset): Record the location's
> symbol in the sal.
> * linespec.c (convert_address_location_to_sals): Fill in sal's
> symbol with find_pc_sect_containing_function.
> * symtab.c (find_function_start_sal): Rename to ...
> (find_function_start_sal_1): ... this.
> (find_function_start_sal): Reimplement as wrapper around
> find_function_start_sal_1, and use
> find_pc_sect_containing_function to fill in the sal's symbol.
> (find_function_start_sal(symbol*, bool)): Adjust.
> * symtab.h (find_pc_function, find_pc_sect_function): Adjust
> comments.
> (find_pc_sect_containing_function): Declare.

I know it might be considered a small and trivial part, but I really
appreciate the attention to the function's comment description.

>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * gdb.opt/inline-break.exp (line number, address): Add "info
> break" tests.

I went through the patch and it looks good.

Thanks again!
--
Joel