Re: [PATCH] gdb: ensure bp_location::section is set correct to avoid an assert

[Simon Marchi <simark@simark.ca>]

On 2025-09-04 16:16, Andrew Burgess wrote:
> While reviewing and testing another patch I set a breakpoint on an
> gnu ifunc function, then restarted the inferior, and this assert
> triggered:
> 
>   ../../src/gdb/breakpoint.c:14747: internal-error: breakpoint_free_objfile: Assertion `loc->symtab == nullptr' failed.
> 
> The backtrace at the time of the assert is:
> 
>   #6  0x00000000005ffee0 in breakpoint_free_objfile (objfile=0x4064b30) at ../../src/gdb/breakpoint.c:14747
>   #7  0x0000000000c33ff2 in objfile::~objfile (this=0x4064b30, __in_chrg=<optimized out>) at ../../src/gdb/objfiles.c:478
>   #8  0x0000000000c38da6 in std::default_delete<objfile>::operator() (this=0x7ffc1a49d538, __ptr=0x4064b30) at /usr/include/c++/9/bits/unique_ptr.h:81
>   #9  0x0000000000c3782a in std::unique_ptr<objfile, std::default_delete<objfile> >::~unique_ptr (this=0x7ffc1a49d538, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/unique_ptr.h:292
>   #10 0x0000000000caf1bd in owning_intrusive_list<objfile, intrusive_base_node<objfile> >::erase (this=0x3790d68, i=...) at ../../src/gdb/../gdbsupport/owning_intrusive_list.h:111
>   #11 0x0000000000cacd0c in program_space::remove_objfile (this=0x3790c80, objfile=0x4064b30) at ../../src/gdb/progspace.c:192
>   #12 0x0000000000c33e1c in objfile::unlink (this=0x4064b30) at ../../src/gdb/objfiles.c:408
>   #13 0x0000000000c34fb9 in objfile_purge_solibs (pspace=0x3790c80) at ../../src/gdb/objfiles.c:729
>   #14 0x0000000000edf6f7 in no_shared_libraries (pspace=0x3790c80) at ../../src/gdb/solib.c:1359
>   #15 0x0000000000fb3f6c in target_pre_inferior () at ../../src/gdb/target.c:2466
>   #16 0x0000000000a724d7 in run_command_1 (args=0x0, from_tty=0, run_how=RUN_NORMAL) at ../../src/gdb/infcmd.c:390
>   #17 0x0000000000a72a97 in run_command (args=0x0, from_tty=0) at ../../src/gdb/infcmd.c:514
>   #18 0x00000000006bbb3d in do_simple_func (args=0x0, from_tty=0, c=0x39124b0) at ../../src/gdb/cli/cli-decode.c:95
>   #19 0x00000000006c1021 in cmd_func (cmd=0x39124b0, args=0x0, from_tty=0) at ../../src/gdb/cli/cli-decode.c:2827
> 
> The function breakpoint_free_objfile is being called when an objfile
> representing a shared library is being unloaded ahead of the inferior
> being restarted, the function is trying to remove references to
> anything that could itself reference the objfile that is being
> deleted.
> 
> The assert is making the claim that, for a bp_location, which has a
> single address, the objfile of the symtab associated with the location
> will be the same as the objfile associated with the section of the
> location.
> 
> This seems reasonable to me now, as it did when I added the assert in
> commit:
> 
>   commit 5066f3680667ec0f2d1745847a2372d85973a1e7
>   Date:   Mon Nov 11 21:45:17 2024 +0000
> 
>       gdb: do better in breakpoint_free_objfile
> 
> The bp_location::section is maintained, according to the comments in
> breakpoint.h, to aid overlay debugging (is that even used any more),
> and looking at the code, this does appear to be the case.
> 
> The problem in the above case arises when we are dealing with an ifunc
> function.  What happens is that we end up with a section from one
> objfile, but a symtab from a different objfile.
> 
> This problem originates from minsym_found (in linespec.c).  The user
> asked for 'break gnu_ifunc' where 'gnu_ifunc' is an ifunc function.
> What this means is that gnu_ifunc is actually a resolver function that
> returns the address of the actual function to use.
> 
> In this particular test case, the resolver function is in a shared
> library, and the actual function to use is in the main executable.
> 
> So, when GDB looks for 'gnu_ifunc' is finds the minimal_symbol with
> that name, and spots that this has type mst_text_gnu_ifunc.  GDB then
> uses this to figure out the actual address of the function that will
> be run.
> 
> GDB then creates the symtab_and_line using the _real_ address and the
> symtab in which that address lies, in our case this will all be
> related to the main executable objfile.
> 
> But, finally, in minsym_found, GDB fills in the symtab_and_line's
> section field, and this is done using the section containing the
> original minimal_symbol, which is from the shared library objfile.
> 
> The minimal symbol and section are then use to initialise the
> bp_location object, and this is how we end up in, what I think, is an
> unexpected state.
> 
> So what to do about this?
> 
> The symtab_and_line::msymbol field is _only_ set within minsym_found,
> and is then _only_ used to initialise the bp_location::msymbol field.
> 
> The bp_location::msymbol field is _only_ used in the function
> set_breakpoint_location_function, and we only really care about the
> msymbol type, we check to see if it's an ifunc symbol or not.  This
> allows us to set the name of the function correctly.
> 
> The bp_location::section is used, as far as I can tell, extensively
> for overlay handling.  It would seem to me, that this section should
> be the section containing the actual breakpoint address.  If the
> question we're asking is, is this breakpoint mapped in or not?  Then
> surely we need to ask about the section holding the breakpoint's
> address, and not the section holding some other code (e.g. the
> resolver function).  In fact, in a memory constrained environment,
> you'd expect the resolver functions to get mapped out pretty early on,
> but while the actual functions might still be mapped in.
> 
> Finally, symtab_and_line::section.  This is mostly set using calls to
> find_pc_overlay.  The minsym_found function is one of the few places
> where we do things differently.  In the places where the section is
> used, it is (almost?) always used in conjunction with the
> symtab_and_line::pc to lookup information, e.g. calls to
> block_for_pc_sect, or find_pc_sect_containing_function.  In all these
> cases, it appears to me that the assumption is that the section will
> be the section that contains the address.
> 
> So, where does this leave us?
> 
> I think what we need to do is update minsym_found to just use
> find_pc_overlay, which is how the symtab_and_line::section is set in
> most other cases.  What this actually means in practise is that the
> section field will be set to NULL (see find_pc_overlay in symfile.c).
> But given that this is how the section is computed in most other
> cases, I don't see why it should be especially problematic for this
> case.  In reality, I think this just means that the section is
> calculated via a call to find_pc_section when it's needed, as an
> example, see lookup_minimal_symbol_by_pc_section (minsyms.c).
> 
> I do wonder if we should be doing better when creating the
> symtab_and_line, and insist that the section be calculated correctly
> at that point, but I really don't want to open that can of worms right
> now, so I think just changing minsym_found to "do it just like
> everyone else" should be good enough.
> 
> I've extended the existing ifunc test to expose this issue, the
> updated test fails without this patch, and passes with.

The explanation makes sense to me, and I confirm that the test fails
without the fix.  So I guess:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon