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

[Joel Brobecker <brobecker@adacore.com>]

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