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

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

[Andrew Burgess]

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.
---
 gdb/linespec.c                       | 7 ++++++-
 gdb/testsuite/gdb.base/gnu-ifunc.exp | 5 +++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index cefee026d92..e634bf22f3c 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4113,7 +4113,12 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
       sal.pspace = current_program_space;
     }
 
-  sal.section = msymbol->obj_section (objfile);
+  /* Don't use the section from the msymbol, the code above might have
+     adjusted FUNC_ADDR, in which case the msymbol's section might not be
+     the section containing FUNC_ADDR.  It might not even be in the same
+     objfile.  As the section is primarily to assist with overlay
+     debugging, it should reflect the SAL's pc value.  */
+  sal.section = find_pc_overlay (sal.pc);
 
   if (self->maybe_add_address (objfile->pspace (), sal.pc))
     add_sal_to_sals (self, result, &sal, msymbol->natural_name (), false);
diff --git a/gdb/testsuite/gdb.base/gnu-ifunc.exp b/gdb/testsuite/gdb.base/gnu-ifunc.exp
index c7afbe5fc59..20a09973425 100644
--- a/gdb/testsuite/gdb.base/gnu-ifunc.exp
+++ b/gdb/testsuite/gdb.base/gnu-ifunc.exp
@@ -185,6 +185,11 @@ proc_with_prefix set-break {resolver_attr resolver_debug final_debug} {
 	# other two locations.
 	gdb_test "info breakpoints" "$location\r\n.*$location\r\n$location"
     }
+
+    # At one point a bug existed such that GDB would trigger an assert
+    # while restarting the inferior with ifunc breakpoints set.
+    gdb_run_cmd
+    gdb_test "" "Breakpoint $::decimal,.*final \\(\[^\r\n\]*\\).*" "restart, run until breakpoint"
 }
 
 # Misc GNU ifunc tests.  For the description of RESOLVER_ATTR,

base-commit: 7bdcd19cc6d8137ecdca83571946d6ffb7b4be26
-- 
2.47.1

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

[Simon Marchi]

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

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

[Sébastien Darche]

[-- Attachment #1: Type: text/plain, Size: 3014 bytes --]

Hi Andrew,

We've noticed a regression caused by this patch on one of the amdgpu
tests (namely gdb.rocm/displaced-stepping.exp). This test attempts
to insert a breakpoint at the beginning of a GPU kernel, step, then
proceed to the end of the test. It would appear the breakpoint is
inserted but not hit by the GPU.


Upon investigating why the breakpoint is not set the way it should be,
I've found that code_breakpoint::add_location relies on the section
being set to deduce the proper gdbarch, by calling get_sal_arch
(breakpoint.c:8615). If the sal section is not set, then the gdbarch is
set by default to the breakpoint gdbarch, which in our case is the host's
gdbarch. This causes problem down the road leading to the breakpoint not
being hit.


A quick fix to the problem would be to call find_pc_section() to assign
the proper section to the pc, *after* checking for find_pc_overlay :


diff --git a/gdb/linespec.c b/gdb/linespec.c
index e634bf22f3c..4a4d178085c 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4120,6 +4120,10 @@ minsym_found (struct linespec_state *self, struct 
objfile *objfile,
       debugging, it should reflect the SAL's pc value.  */
    sal.section = find_pc_overlay (sal.pc);

+  if (sal.section == nullptr) {
+    sal.section = find_pc_section (sal.pc);
+  }
+
    if (self->maybe_add_address (objfile->pspace (), sal.pc))
      add_sal_to_sals (self, result, &sal, msymbol->natural_name (), 
false);
  }


This ensures that minsym_found() finds the proper pc (instead of the
resolver, for ifuncs, just like your patch intended), then assigns
the section corresponding to that pc. Ensuring that sal.section is
consistent to the pc is, in my opinion, a more appropriate solution
than leaving its contents empty for the time being. Modifying the
other places where the section is needed (or expected to be null) may
require some further investigation.

> 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.

Admittedly, this means thinking about *when* sections are stored
in the sal. Let me know what you think about this problem and if
you think of another approach.


Best,
Sébastien


[-- Attachment #2: Type: text/html, Size: 5015 bytes --]

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

[Andrew Burgess]

Sébastien Darche <sdarche@efficios.com> writes:

> Hi Andrew,
>
> We've noticed a regression caused by this patch on one of the amdgpu
> tests (namely gdb.rocm/displaced-stepping.exp). This test attempts
> to insert a breakpoint at the beginning of a GPU kernel, step, then
> proceed to the end of the test. It would appear the breakpoint is
> inserted but not hit by the GPU.
>
>
> Upon investigating why the breakpoint is not set the way it should be,
> I've found that code_breakpoint::add_location relies on the section
> being set to deduce the proper gdbarch, by calling get_sal_arch
> (breakpoint.c:8615). If the sal section is not set, then the gdbarch is
> set by default to the breakpoint gdbarch, which in our case is the host's
> gdbarch. This causes problem down the road leading to the breakpoint not
> being hit.
>
>
> A quick fix to the problem would be to call find_pc_section() to assign
> the proper section to the pc, *after* checking for find_pc_overlay :
>
>
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index e634bf22f3c..4a4d178085c 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -4120,6 +4120,10 @@ minsym_found (struct linespec_state *self, struct 
> objfile *objfile,
>        debugging, it should reflect the SAL's pc value.  */
>     sal.section = find_pc_overlay (sal.pc);
>
> +  if (sal.section == nullptr) {
> +    sal.section = find_pc_section (sal.pc);
> +  }
> +
>     if (self->maybe_add_address (objfile->pspace (), sal.pc))
>       add_sal_to_sals (self, result, &sal, msymbol->natural_name (), 
> false);
>   }
>
>
> This ensures that minsym_found() finds the proper pc (instead of the
> resolver, for ifuncs, just like your patch intended), then assigns
> the section corresponding to that pc. Ensuring that sal.section is
> consistent to the pc is, in my opinion, a more appropriate solution
> than leaving its contents empty for the time being. Modifying the
> other places where the section is needed (or expected to be null) may
> require some further investigation.
>
>> 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.
>
> Admittedly, this means thinking about *when* sections are stored
> in the sal. Let me know what you think about this problem and if
> you think of another approach.

First, I don't really object to the fix you propose.  There's already
code like this in GDB, see the find_pc_overlay call in blockframe.c.

However.

I think the real fix here would be to fold the find_pc_section call into
find_pc_overlay, like:

  struct obj_section *
  find_pc_overlay (CORE_ADDR pc)
  {
    struct obj_section *best_match = NULL;
  
    if (overlay_debugging)
      {
        ... existing code here ...
      }
    else
      best_match = find_pc_section (pc);
  
    return best_match;
  }

Note that the function names might not be ideal after this change, but
they'll do for now.

Almost all calls to find_pc_overlay are related to setting the section
within a SAL, so I think we can be comfortable in those cases that this
change is OK (I say that without testing this at all).

Then there's a find_pc_overlay call in blockframe.c where we already
have a find_pc_section call, so I think that case is fine.

There's a call in find_pc_line, this one appears at a glance to be
interesting as we'd now be passing a non NULL section to
find_pc_sect_line in more cases, which, following the code through,
would appear as if we might trigger some additional cases.

And there's a call inside memory_xfer_partial_1, but this is already
guarded by an overlay_debugging check, so in this case the change I
propose would have no impact.

I'd definitely be interested in seeing if the above change throws up any
problems in testing.

Part of the reason I'd push for a wider fix is that there are lots
different linespec formats, and I don't think they all pass through
minsym_found (or maybe they do?).  If they don't then it feels like you
should be able to adjust your original test such that minsym_found isn't
called, and you'll have the same incorrect gdbarch problem.  So then
you'll have to add a find_pc_section in _another_ place....

Anyway, the above isn't a requirement, just my thoughts.

Thanks,
Andrew

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

[Simon Marchi]

On 9/25/25 1:56 PM, Andrew Burgess wrote:
> Part of the reason I'd push for a wider fix is that there are lots
> different linespec formats, and I don't think they all pass through
> minsym_found (or maybe they do?).

It's certainly possble to create SALs without going through
minsym_found.

> If they don't then it feels like you should be able to adjust your
> original test such that minsym_found isn't called, and you'll have the
> same incorrect gdbarch problem.  So then you'll have to add a
> find_pc_section in _another_ place....

If the test was compiled with DWARF info, the SAL would be created from
a full (DWARF) symbol, and we'd go through another path that would cause
the section field to be set.  I think find_function_start_sal ->
find_function_start_sal_1.  In this case the section would be known from
the struct symbol.

I went a bit down the overlay rabbit hole, and I think that your change
might have broken that (but... I don't know how to test that).  Here's
the summary of my findings (I'm perhaps completely wrong).

Two ELF sections overlay each other if they have the same VMA but
different LMA.

 - LMA is the address where the code is permanently stored (e.g. large
   flash memory, non-executable).  Unique for each section.
 - VMA is the address where the code gets copied when executed (e.g.
   small RAM, executable).

An overlay manager in the program takes care of copying the right
section from its LMA to its VMA before executing it.

My understanding is that the ELF symbols for the various functions in
these overlaying region have VMA region of memory.  So, you would have
overlapping ELF symbols, but you can know which ELF symbol is part of
which section, because ELF symbols have a "section index" property.  We
record that in minimal_symbol::m_section.

Before your patch, when seting a breakpoint by minimal symbol in an
overlay situation, this:

  sal.section = msymbol->obj_section (objfile);

would return the right section based on the minimal symbol you
specified.  I guess this is important later.  To know if it should
insert the breakpoint location, GDB must decide if the breakpoint
location is in the section currently mapped at the VMA or not.  For
that, it needs to know in which section you intended to put the
breakpoint.  However, the new line:

  sal.section = find_pc_overlay (sal.pc);

would return one of the multiple sections in which sal.pc (a VMA)
appears, possibly the wrong one.  This would mess up the "should we
insert this location" logic later.

This is what I gathered from an hour of reading the code, it's perhaps
wrong.  I need to go for now, but I thought I'd share these findings.

Simon

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

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 9/25/25 1:56 PM, Andrew Burgess wrote:
>> Part of the reason I'd push for a wider fix is that there are lots
>> different linespec formats, and I don't think they all pass through
>> minsym_found (or maybe they do?).
>
> It's certainly possble to create SALs without going through
> minsym_found.
>
>> If they don't then it feels like you should be able to adjust your
>> original test such that minsym_found isn't called, and you'll have the
>> same incorrect gdbarch problem.  So then you'll have to add a
>> find_pc_section in _another_ place....
>
> If the test was compiled with DWARF info, the SAL would be created from
> a full (DWARF) symbol, and we'd go through another path that would cause
> the section field to be set.  I think find_function_start_sal ->
> find_function_start_sal_1.  In this case the section would be known from
> the struct symbol.
>
> I went a bit down the overlay rabbit hole, and I think that your change
> might have broken that (but... I don't know how to test that).  Here's
> the summary of my findings (I'm perhaps completely wrong).
>
> Two ELF sections overlay each other if they have the same VMA but
> different LMA.
>
>  - LMA is the address where the code is permanently stored (e.g. large
>    flash memory, non-executable).  Unique for each section.
>  - VMA is the address where the code gets copied when executed (e.g.
>    small RAM, executable).
>
> An overlay manager in the program takes care of copying the right
> section from its LMA to its VMA before executing it.
>
> My understanding is that the ELF symbols for the various functions in
> these overlaying region have VMA region of memory.  So, you would have
> overlapping ELF symbols, but you can know which ELF symbol is part of
> which section, because ELF symbols have a "section index" property.  We
> record that in minimal_symbol::m_section.
>
> Before your patch, when seting a breakpoint by minimal symbol in an
> overlay situation, this:
>
>   sal.section = msymbol->obj_section (objfile);
>
> would return the right section based on the minimal symbol you
> specified.  I guess this is important later.  To know if it should
> insert the breakpoint location, GDB must decide if the breakpoint
> location is in the section currently mapped at the VMA or not.  For
> that, it needs to know in which section you intended to put the
> breakpoint.  However, the new line:
>
>   sal.section = find_pc_overlay (sal.pc);
>
> would return one of the multiple sections in which sal.pc (a VMA)
> appears, possibly the wrong one.  This would mess up the "should we
> insert this location" logic later.

That sounds reasonable enough.  But the original code was still wrong I
think.

I don't have time right now to reexamine the code I'm afraid, so I'm
going from memory a bit here.

But in the ifunc case, I think MSYMBOL is the symbol from the resolver,
not the actual function where the breakpoint is being placed.

Maybe the answer is as simple as moving the .section assignment into the
earlier if block, something like:

  if (is_function && want_start_sal)
    {
      sal = find_function_start_sal (func_addr, NULL, self->funfirstline);

      /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
         anywhere, in a completely different section to MSYMBOL, or even
         in a different objfile!

         TODO: I haven't checked, maybe find_function_start_sal already
         fills this stuff in for us?  Or maybe it could be made too?
         For now I'm assuming all we have is an address, but this needs
         checking.  */
      sal.section = find_pc_overlay (func_addr);
      if (sal.section == nullptr)
        sal.section = find_pc_section (func_addr);
    }
  else
    {
      sal.objfile = objfile;
      sal.msymbol = msymbol;
      /* Store func_addr, not the minsym's address in case this was an
	 ifunc that hasn't been resolved yet.  */
      if (is_function)
	sal.pc = func_addr;
      else
	sal.pc = msymbol->value_address (objfile);
      sal.pspace = current_program_space;

      /* We can assign the section based on MSYMBOL here because the
         breakpoint is actually being placed at (or near) MSYMBOL.  */
      sal.section = msymbol->obj_section (objfile);
    }

Now we retain the use of MSYMBOL where we can, which addresses the valid
issues you identify above.

But we no longer use MSYMBOL when it's the wrong thing to do, which
addresses the problem I was trying to fix.

Does this look like a valid path forward maybe?

FYI: I'm off work for a couple of days now, but will catch up when I
return.  There was a test with my original change, so as long as that's
still passing I'm happy with whatever you think best.

Thanks,
Andrew

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

[Simon Marchi]

On 2025-09-25 17:40, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 9/25/25 1:56 PM, Andrew Burgess wrote:
>>> Part of the reason I'd push for a wider fix is that there are lots
>>> different linespec formats, and I don't think they all pass through
>>> minsym_found (or maybe they do?).
>>
>> It's certainly possble to create SALs without going through
>> minsym_found.
>>
>>> If they don't then it feels like you should be able to adjust your
>>> original test such that minsym_found isn't called, and you'll have the
>>> same incorrect gdbarch problem.  So then you'll have to add a
>>> find_pc_section in _another_ place....
>>
>> If the test was compiled with DWARF info, the SAL would be created from
>> a full (DWARF) symbol, and we'd go through another path that would cause
>> the section field to be set.  I think find_function_start_sal ->
>> find_function_start_sal_1.  In this case the section would be known from
>> the struct symbol.
>>
>> I went a bit down the overlay rabbit hole, and I think that your change
>> might have broken that (but... I don't know how to test that).  Here's
>> the summary of my findings (I'm perhaps completely wrong).
>>
>> Two ELF sections overlay each other if they have the same VMA but
>> different LMA.
>>
>>  - LMA is the address where the code is permanently stored (e.g. large
>>    flash memory, non-executable).  Unique for each section.
>>  - VMA is the address where the code gets copied when executed (e.g.
>>    small RAM, executable).
>>
>> An overlay manager in the program takes care of copying the right
>> section from its LMA to its VMA before executing it.
>>
>> My understanding is that the ELF symbols for the various functions in
>> these overlaying region have VMA region of memory.  So, you would have
>> overlapping ELF symbols, but you can know which ELF symbol is part of
>> which section, because ELF symbols have a "section index" property.  We
>> record that in minimal_symbol::m_section.
>>
>> Before your patch, when seting a breakpoint by minimal symbol in an
>> overlay situation, this:
>>
>>   sal.section = msymbol->obj_section (objfile);
>>
>> would return the right section based on the minimal symbol you
>> specified.  I guess this is important later.  To know if it should
>> insert the breakpoint location, GDB must decide if the breakpoint
>> location is in the section currently mapped at the VMA or not.  For
>> that, it needs to know in which section you intended to put the
>> breakpoint.  However, the new line:
>>
>>   sal.section = find_pc_overlay (sal.pc);
>>
>> would return one of the multiple sections in which sal.pc (a VMA)
>> appears, possibly the wrong one.  This would mess up the "should we
>> insert this location" logic later.
> 
> That sounds reasonable enough.  But the original code was still wrong I
> think.
> 
> I don't have time right now to reexamine the code I'm afraid, so I'm
> going from memory a bit here.
> 
> But in the ifunc case, I think MSYMBOL is the symbol from the resolver,
> not the actual function where the breakpoint is being placed.

Oh, absolutely.  Your fix was correct for the ifunc case.  We just need
to find something that works for ifunc and overlays.

> Maybe the answer is as simple as moving the .section assignment into the
> earlier if block, something like:
> 
>   if (is_function && want_start_sal)
>     {
>       sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> 
>       /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
>          anywhere, in a completely different section to MSYMBOL, or even
>          in a different objfile!
> 
>          TODO: I haven't checked, maybe find_function_start_sal already
>          fills this stuff in for us?  Or maybe it could be made too?
>          For now I'm assuming all we have is an address, but this needs
>          checking.  */
>       sal.section = find_pc_overlay (func_addr);
>       if (sal.section == nullptr)
>         sal.section = find_pc_section (func_addr);
>     }
>   else
>     {
>       sal.objfile = objfile;
>       sal.msymbol = msymbol;
>       /* Store func_addr, not the minsym's address in case this was an
> 	 ifunc that hasn't been resolved yet.  */
>       if (is_function)
> 	sal.pc = func_addr;
>       else
> 	sal.pc = msymbol->value_address (objfile);
>       sal.pspace = current_program_space;
> 
>       /* We can assign the section based on MSYMBOL here because the
>          breakpoint is actually being placed at (or near) MSYMBOL.  */
>       sal.section = msymbol->obj_section (objfile);
>     }
> 
> Now we retain the use of MSYMBOL where we can, which addresses the valid
> issues you identify above.
> 
> But we no longer use MSYMBOL when it's the wrong thing to do, which
> addresses the problem I was trying to fix.

Yeah, I was also thinking about something along those lines.  Only look
up the section if the func addr is different from the original minsym.
Otherwise, you can trust the original minsym's section.

Perhaps we can assume that it's not possible to have both ifuncs and
overlays.  I think that these are used in really different use cases
(system with a full fledged dynamic linker vs embedded system with very
constrained memory).

> Does this look like a valid path forward maybe?
> 
> FYI: I'm off work for a couple of days now, but will catch up when I
> return.  There was a test with my original change, so as long as that's
> still passing I'm happy with whatever you think best.

I'm officially off tomorrow as well, but Sébastien might chime in.

Simon

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

[Sébastien Darche]

On 9/25/25 17:40, Andrew Burgess wrote:
> Maybe the answer is as simple as moving the .section assignment into the
> earlier if block, something like:
> 
>    if (is_function && want_start_sal)
>      {
>        sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> 
>        /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
>           anywhere, in a completely different section to MSYMBOL, or even
>           in a different objfile!
> 
>           TODO: I haven't checked, maybe find_function_start_sal already
>           fills this stuff in for us?  Or maybe it could be made too?
>           For now I'm assuming all we have is an address, but this needs
>           checking.  */
>        sal.section = find_pc_overlay (func_addr);
>        if (sal.section == nullptr)
>          sal.section = find_pc_section (func_addr);
>      }
>    else
>      {
>        sal.objfile = objfile;
>        sal.msymbol = msymbol;
>        /* Store func_addr, not the minsym's address in case this was an
> 	 ifunc that hasn't been resolved yet.  */
>        if (is_function)
> 	sal.pc = func_addr;
>        else
> 	sal.pc = msymbol->value_address (objfile);
>        sal.pspace = current_program_space;
> 
>        /* We can assign the section based on MSYMBOL here because the
>           breakpoint is actually being placed at (or near) MSYMBOL.  */
>        sal.section = msymbol->obj_section (objfile);
>      }
> 

To answer your question on whether find_function_start_sal does fill 
this for us : it depends. It manages to do it on amd64 but not on amdgpu.

By default, the sal does not contain a valid section. It's only when we 
try to adjust the pc past the prologue (skip_prologue_sal) that a 
section is computed for the pc at the start of the function. If we do 
have a prologue, then we assign that section (symtab.c:3914). If not 
(and that is the case on amdgpu), then we're left with an empty 
sal.section. I would say the behavior is not really consistent.

I would agree it could be made to.
> Does this look like a valid path forward maybe?

Your solution seems to work for the gnu-ifunc test and fixes the 
regression for gdb.rocm/displaced-stepping.exp - so I'd say it's a good 
aproach. I am not familiar with overlays, so I can't really judge if the 
change would impact how they are handled.

I think it would be best to ensure find_function_start_sal has a 
consistent behavior across architectures. I'll submit a small patch 
which should address this. This would also at least reduce the chance 
for another bug like this to appear somewhere else :

 > So then you'll have to add a find_pc_section in _another_ place....


Sébastien

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

[Andrew Burgess]

Sébastien Darche <sdarche@efficios.com> writes:

> On 9/25/25 17:40, Andrew Burgess wrote:
>> Maybe the answer is as simple as moving the .section assignment into the
>> earlier if block, something like:
>> 
>>    if (is_function && want_start_sal)
>>      {
>>        sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
>> 
>>        /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
>>           anywhere, in a completely different section to MSYMBOL, or even
>>           in a different objfile!
>> 
>>           TODO: I haven't checked, maybe find_function_start_sal already
>>           fills this stuff in for us?  Or maybe it could be made too?
>>           For now I'm assuming all we have is an address, but this needs
>>           checking.  */
>>        sal.section = find_pc_overlay (func_addr);
>>        if (sal.section == nullptr)
>>          sal.section = find_pc_section (func_addr);
>>      }
>>    else
>>      {
>>        sal.objfile = objfile;
>>        sal.msymbol = msymbol;
>>        /* Store func_addr, not the minsym's address in case this was an
>> 	 ifunc that hasn't been resolved yet.  */
>>        if (is_function)
>> 	sal.pc = func_addr;
>>        else
>> 	sal.pc = msymbol->value_address (objfile);
>>        sal.pspace = current_program_space;
>> 
>>        /* We can assign the section based on MSYMBOL here because the
>>           breakpoint is actually being placed at (or near) MSYMBOL.  */
>>        sal.section = msymbol->obj_section (objfile);
>>      }
>> 
>
> To answer your question on whether find_function_start_sal does fill 
> this for us : it depends. It manages to do it on amd64 but not on amdgpu.
>
> By default, the sal does not contain a valid section. It's only when we 
> try to adjust the pc past the prologue (skip_prologue_sal) that a 
> section is computed for the pc at the start of the function. If we do 
> have a prologue, then we assign that section (symtab.c:3914). If not 
> (and that is the case on amdgpu), then we're left with an empty 
> sal.section. I would say the behavior is not really consistent.
>
> I would agree it could be made to.
>> Does this look like a valid path forward maybe?
>
> Your solution seems to work for the gnu-ifunc test and fixes the 
> regression for gdb.rocm/displaced-stepping.exp - so I'd say it's a good 
> aproach. I am not familiar with overlays, so I can't really judge if the 
> change would impact how they are handled.
>
> I think it would be best to ensure find_function_start_sal has a 
> consistent behavior across architectures. I'll submit a small patch 
> which should address this. This would also at least reduce the chance 
> for another bug like this to appear somewhere else :
>

Hi,

I'm proposing the patch below.  You should double check that this still
addresses the issue you're seeing with the amdgpu target.  Given Simon's
concerns, I do wonder if there might still be some issues with this
related to overlay debugging, but without any way to test it, and no
known overlay users, I think we can probably just ignore that for now.

If this fixes your regression, then maybe we should merge this, and
figure any other issues out later?

Thanks,
Andrew

---

commit 6ffea587445eeacf8b2962de6d3b00d6efa98213
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Oct 6 10:27:08 2025 +0100

    gdb: fixes for setting the section in minsym_found
    
    After this commit:
    
      commit 6f7ad2381ae72aa592ada4a0921265aa3292b1fa
      Date:   Wed Sep 3 19:57:42 2025 +0100
    
          gdb: ensure bp_location::section is set correct to avoid an assert
    
    Some issues were reported as a result of the bp_location::section
    being left as NULL by the call to find_pc_overlay that was
    introduced.  See this thread:
    
      https://inbox.sourceware.org/gdb-patches/7febb0c1-7bbd-45d5-8ebe-91c34bb4a6ce@efficios.com
    
    The problem was that code_breakpoint::add_location relies on the
    section being set in order to deduce the gdbarch.  If the section is
    not set then the gdbarch is deduced using the breakpoint's gdbarch.
    
    The bug was reported by the ROCm engineers, who have inferiors running
    mixed host and GPU code, and so rely on the section being set in order
    to establish the correct architecture for a specific address.
    
    During discussion in the above thread Simon pointed out that the
    change made in the above commit might not be correct anyway for
    overlay debugging (does that even work, or is it used any more?), as
    the commit relies on establishing a section by calling
    find_pc_overlay.  However, when presented with multiple possible
    sections, find_pc_overlay cannot know which section to select, and so
    just picks one.  This could be different from the section of the
    minimal_symbol we already had to hand.
    
    This patch I think should (at least) resolve the issues the ROCm
    engineers are seeing.
    
    Instead of always calling find_pc_overlay I have moved the section
    assignment inside the if/then/else blocks with the following
    reasoning.
    
    In the 'else' block, this is the non-function or non-ifunc case, the
    address used is based on the msymbol's address, and so should be in
    the same section.  In this case we can use the msymbol's section.
    
    In the 'if' block things are more complicated.  This could be the
    ifunc case, in which case func_addr could have been adjusted to a
    different section, or even different objfile.
    
    Further, when we call find_function_start_sal, we pass in just an
    address, so the SAL being returned isn't going to consider which
    overlay section the original msymbol was from, which could cause
    problems for overlay debugging maybe?
    
    Anyway, I'm ignoring that for now, as fixing that would be a whole big
    thing.  So I'm proposing that, if find_function_start_sal returns a
    symtab_and_line with a section set, then we use that section.
    Otherwise, we can try to figure out a section.

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 2ddc495babf..4d9c5ac26f3 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4083,13 +4083,16 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
 
   CORE_ADDR func_addr;
   bool is_function = msymbol_is_function (objfile, msymbol, &func_addr);
+  bool is_ifunc = false;
 
   if (is_function)
     {
       const char *msym_name = msymbol->linkage_name ();
 
-      if (msymbol->type () == mst_text_gnu_ifunc
-	  || msymbol->type () == mst_data_gnu_ifunc)
+      is_ifunc = (msymbol->type () == mst_text_gnu_ifunc
+		  || msymbol->type () == mst_data_gnu_ifunc);
+
+      if (is_ifunc)
 	want_start_sal = gnu_ifunc_resolve_name (msym_name, &func_addr);
       else
 	want_start_sal = true;
@@ -4098,7 +4101,32 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
   symtab_and_line sal;
 
   if (is_function && want_start_sal)
-    sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
+    {
+      sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
+
+      /* If SAL already has a section then we'll use that.  If not, then we
+	 can try to find a section.
+
+	 In the ifunc case though we cannot rely on the section of MSYMBOL,
+	 the ifunc target could be in a different section, or even a
+	 different objfile, from the original MSYMBOL.  For this case, we
+	 fall back to looking up a section based on FUNC_ADDR.
+
+	 For the non-ifunc case, we can use the section of MSYMBOL, as
+	 that's how we filled in FUNC_ADDR, so they should be in the same
+	 section.  */
+      if (sal.section == nullptr)
+	{
+	  if (!is_ifunc)
+	    sal.section = msymbol->obj_section (objfile);
+	  else
+	    {
+	      sal.section = find_pc_overlay (func_addr);
+	      if (sal.section == nullptr)
+		sal.section = find_pc_section (func_addr);
+	    }
+	}
+    }
   else
     {
       sal.objfile = objfile;
@@ -4110,14 +4138,13 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
       else
 	sal.pc = msymbol->value_address (objfile);
       sal.pspace = current_program_space;
-    }
 
-  /* Don't use the section from the msymbol, the code above might have
-     adjusted FUNC_ADDR, in which case the msymbol's section might not be
-     the section containing FUNC_ADDR.  It might not even be in the same
-     objfile.  As the section is primarily to assist with overlay
-     debugging, it should reflect the SAL's pc value.  */
-  sal.section = find_pc_overlay (sal.pc);
+      /* We can assign the section based on MSYMBOL here because the
+	 breakpoint is actually being placed at (or near) MSYMBOL.  Note,
+	 this is not a path where ifunc resolution can have occurred, which
+	 could adjust FUNC_ADDR significantly.  */
+      sal.section = msymbol->obj_section (objfile);
+    }
 
   if (self->maybe_add_address (objfile->pspace (), sal.pc))
     add_sal_to_sals (self, result, &sal, msymbol->natural_name (), false);

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

[Rohr, Stephan]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Monday, 6 October 2025 14:11
> To: Sébastien Darche <sdarche@efficios.com>; simark@simark.ca
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH] gdb: ensure bp_location::section is set correct to avoid an
> assert
> 
> Sébastien Darche <sdarche@efficios.com> writes:
> 
> > On 9/25/25 17:40, Andrew Burgess wrote:
> >> Maybe the answer is as simple as moving the .section assignment into the
> >> earlier if block, something like:
> >>
> >>    if (is_function && want_start_sal)
> >>      {
> >>        sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> >>
> >>        /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
> >>           anywhere, in a completely different section to MSYMBOL, or even
> >>           in a different objfile!
> >>
> >>           TODO: I haven't checked, maybe find_function_start_sal already
> >>           fills this stuff in for us?  Or maybe it could be made too?
> >>           For now I'm assuming all we have is an address, but this needs
> >>           checking.  */
> >>        sal.section = find_pc_overlay (func_addr);
> >>        if (sal.section == nullptr)
> >>          sal.section = find_pc_section (func_addr);
> >>      }
> >>    else
> >>      {
> >>        sal.objfile = objfile;
> >>        sal.msymbol = msymbol;
> >>        /* Store func_addr, not the minsym's address in case this was an
> >> 	 ifunc that hasn't been resolved yet.  */
> >>        if (is_function)
> >> 	sal.pc = func_addr;
> >>        else
> >> 	sal.pc = msymbol->value_address (objfile);
> >>        sal.pspace = current_program_space;
> >>
> >>        /* We can assign the section based on MSYMBOL here because the
> >>           breakpoint is actually being placed at (or near) MSYMBOL.  */
> >>        sal.section = msymbol->obj_section (objfile);
> >>      }
> >>
> >
> > To answer your question on whether find_function_start_sal does fill
> > this for us : it depends. It manages to do it on amd64 but not on amdgpu.
> >
> > By default, the sal does not contain a valid section. It's only when we
> > try to adjust the pc past the prologue (skip_prologue_sal) that a
> > section is computed for the pc at the start of the function. If we do
> > have a prologue, then we assign that section (symtab.c:3914). If not
> > (and that is the case on amdgpu), then we're left with an empty
> > sal.section. I would say the behavior is not really consistent.
> >
> > I would agree it could be made to.
> >> Does this look like a valid path forward maybe?
> >
> > Your solution seems to work for the gnu-ifunc test and fixes the
> > regression for gdb.rocm/displaced-stepping.exp - so I'd say it's a good
> > aproach. I am not familiar with overlays, so I can't really judge if the
> > change would impact how they are handled.
> >
> > I think it would be best to ensure find_function_start_sal has a
> > consistent behavior across architectures. I'll submit a small patch
> > which should address this. This would also at least reduce the chance
> > for another bug like this to appear somewhere else :
> >
> 
> Hi,
> 
> I'm proposing the patch below.  You should double check that this still
> addresses the issue you're seeing with the amdgpu target.  Given Simon's
> concerns, I do wonder if there might still be some issues with this
> related to overlay debugging, but without any way to test it, and no
> known overlay users, I think we can probably just ignore that for now.
> 
> If this fixes your regression, then maybe we should merge this, and
> figure any other issues out later?
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 6ffea587445eeacf8b2962de6d3b00d6efa98213
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Mon Oct 6 10:27:08 2025 +0100
> 
>     gdb: fixes for setting the section in minsym_found
> 
>     After this commit:
> 
>       commit 6f7ad2381ae72aa592ada4a0921265aa3292b1fa
>       Date:   Wed Sep 3 19:57:42 2025 +0100
> 
>           gdb: ensure bp_location::section is set correct to avoid an assert
> 
>     Some issues were reported as a result of the bp_location::section
>     being left as NULL by the call to find_pc_overlay that was
>     introduced.  See this thread:
> 
>       https://inbox.sourceware.org/gdb-patches/7febb0c1-7bbd-45d5-8ebe-
> 91c34bb4a6ce@efficios.com
> 
>     The problem was that code_breakpoint::add_location relies on the
>     section being set in order to deduce the gdbarch.  If the section is
>     not set then the gdbarch is deduced using the breakpoint's gdbarch.
> 
>     The bug was reported by the ROCm engineers, who have inferiors running
>     mixed host and GPU code, and so rely on the section being set in order
>     to establish the correct architecture for a specific address.
> 
>     During discussion in the above thread Simon pointed out that the
>     change made in the above commit might not be correct anyway for
>     overlay debugging (does that even work, or is it used any more?), as
>     the commit relies on establishing a section by calling
>     find_pc_overlay.  However, when presented with multiple possible
>     sections, find_pc_overlay cannot know which section to select, and so
>     just picks one.  This could be different from the section of the
>     minimal_symbol we already had to hand.
> 
>     This patch I think should (at least) resolve the issues the ROCm
>     engineers are seeing.
> 
>     Instead of always calling find_pc_overlay I have moved the section
>     assignment inside the if/then/else blocks with the following
>     reasoning.
> 
>     In the 'else' block, this is the non-function or non-ifunc case, the
>     address used is based on the msymbol's address, and so should be in
>     the same section.  In this case we can use the msymbol's section.
> 
>     In the 'if' block things are more complicated.  This could be the
>     ifunc case, in which case func_addr could have been adjusted to a
>     different section, or even different objfile.
> 
>     Further, when we call find_function_start_sal, we pass in just an
>     address, so the SAL being returned isn't going to consider which
>     overlay section the original msymbol was from, which could cause
>     problems for overlay debugging maybe?
> 
>     Anyway, I'm ignoring that for now, as fixing that would be a whole big
>     thing.  So I'm proposing that, if find_function_start_sal returns a
>     symtab_and_line with a section set, then we use that section.
>     Otherwise, we can try to figure out a section.
> 
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 2ddc495babf..4d9c5ac26f3 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -4083,13 +4083,16 @@ minsym_found (struct linespec_state *self,
> struct objfile *objfile,
> 
>    CORE_ADDR func_addr;
>    bool is_function = msymbol_is_function (objfile, msymbol, &func_addr);
> +  bool is_ifunc = false;
> 
>    if (is_function)
>      {
>        const char *msym_name = msymbol->linkage_name ();
> 
> -      if (msymbol->type () == mst_text_gnu_ifunc
> -	  || msymbol->type () == mst_data_gnu_ifunc)
> +      is_ifunc = (msymbol->type () == mst_text_gnu_ifunc
> +		  || msymbol->type () == mst_data_gnu_ifunc);
> +
> +      if (is_ifunc)
>  	want_start_sal = gnu_ifunc_resolve_name (msym_name,
> &func_addr);
>        else
>  	want_start_sal = true;
> @@ -4098,7 +4101,32 @@ minsym_found (struct linespec_state *self, struct
> objfile *objfile,
>    symtab_and_line sal;
> 
>    if (is_function && want_start_sal)
> -    sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> +    {
> +      sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> +
> +      /* If SAL already has a section then we'll use that.  If not, then we
> +	 can try to find a section.
> +
> +	 In the ifunc case though we cannot rely on the section of MSYMBOL,
> +	 the ifunc target could be in a different section, or even a
> +	 different objfile, from the original MSYMBOL.  For this case, we
> +	 fall back to looking up a section based on FUNC_ADDR.
> +
> +	 For the non-ifunc case, we can use the section of MSYMBOL, as
> +	 that's how we filled in FUNC_ADDR, so they should be in the same
> +	 section.  */
> +      if (sal.section == nullptr)
> +	{
> +	  if (!is_ifunc)
> +	    sal.section = msymbol->obj_section (objfile);
> +	  else
> +	    {
> +	      sal.section = find_pc_overlay (func_addr);
> +	      if (sal.section == nullptr)
> +		sal.section = find_pc_section (func_addr);
> +	    }
> +	}
> +    }
>    else
>      {
>        sal.objfile = objfile;
> @@ -4110,14 +4138,13 @@ minsym_found (struct linespec_state *self,
> struct objfile *objfile,
>        else
>  	sal.pc = msymbol->value_address (objfile);
>        sal.pspace = current_program_space;
> -    }
> 
> -  /* Don't use the section from the msymbol, the code above might have
> -     adjusted FUNC_ADDR, in which case the msymbol's section might not be
> -     the section containing FUNC_ADDR.  It might not even be in the same
> -     objfile.  As the section is primarily to assist with overlay
> -     debugging, it should reflect the SAL's pc value.  */
> -  sal.section = find_pc_overlay (sal.pc);
> +      /* We can assign the section based on MSYMBOL here because the
> +	 breakpoint is actually being placed at (or near) MSYMBOL.  Note,
> +	 this is not a path where ifunc resolution can have occurred, which
> +	 could adjust FUNC_ADDR significantly.  */
> +      sal.section = msymbol->obj_section (objfile);
> +    }
> 
>    if (self->maybe_add_address (objfile->pspace (), sal.pc))
>      add_sal_to_sals (self, result, &sal, msymbol->natural_name (), false);

Hi all,

I came across the same issue when debugging a remote inferior that is compiled w/o debug symbols and a
breakpoint is inserted based on the function name that is called in the inferior. 

Our test started to regress with the introduction of patch
"gdb: ensure bp_location::section is set correct to avoid an assert".  As mentioned in the patch above,
GDB uses the default gdbarch to insert the breakpoint if the section is NULL.  This causes issues later. 

I applied the patch; our breakpoint insertion issue is fixed with this patch.  I reviewed the patch, it is
reasonable to me.  Only thing I wonder is the usage of 'find_pc_ovelay'; this always returns NULL if
overlay debugging is not used? But I guess that's fixed by using 'find_pc_section' in this case.

Thanks for working on this, Andrew!

Stephan
Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928

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

[Simon Marchi]

On 2026-02-02 03:49, Rohr, Stephan wrote:
> Hi all,
> 
> I came across the same issue when debugging a remote inferior that is compiled w/o debug symbols and a
> breakpoint is inserted based on the function name that is called in the inferior. 
> 
> Our test started to regress with the introduction of patch
> "gdb: ensure bp_location::section is set correct to avoid an assert".  As mentioned in the patch above,
> GDB uses the default gdbarch to insert the breakpoint if the section is NULL.  This causes issues later. 
> 
> I applied the patch; our breakpoint insertion issue is fixed with this patch.  I reviewed the patch, it is
> reasonable to me.  Only thing I wonder is the usage of 'find_pc_ovelay'; this always returns NULL if
> overlay debugging is not used? But I guess that's fixed by using 'find_pc_section' in this case.
> 
> Thanks for working on this, Andrew!

I had forgotten about this patch, I think we solved the ROCm issue at
another level, with:

[PATCH 0/4] make it the responsibility of a symtab_and_line's creator to set its section
https://inbox.sourceware.org/gdb-patches/20251024162929.352719-1-sdarche@efficios.com/

Andrew, could you send an updated version of your patch?  It doesn't
apply anymore, and I think it would deserve not being buried in that
other patch's big thread.

Simon

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

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2026-02-02 03:49, Rohr, Stephan wrote:
>> Hi all,
>> 
>> I came across the same issue when debugging a remote inferior that is compiled w/o debug symbols and a
>> breakpoint is inserted based on the function name that is called in the inferior. 
>> 
>> Our test started to regress with the introduction of patch
>> "gdb: ensure bp_location::section is set correct to avoid an assert".  As mentioned in the patch above,
>> GDB uses the default gdbarch to insert the breakpoint if the section is NULL.  This causes issues later. 
>> 
>> I applied the patch; our breakpoint insertion issue is fixed with this patch.  I reviewed the patch, it is
>> reasonable to me.  Only thing I wonder is the usage of 'find_pc_ovelay'; this always returns NULL if
>> overlay debugging is not used? But I guess that's fixed by using 'find_pc_section' in this case.
>> 
>> Thanks for working on this, Andrew!
>
> I had forgotten about this patch, I think we solved the ROCm issue at
> another level, with:
>
> [PATCH 0/4] make it the responsibility of a symtab_and_line's creator to set its section
> https://inbox.sourceware.org/gdb-patches/20251024162929.352719-1-sdarche@efficios.com/
>
> Andrew, could you send an updated version of your patch?  It doesn't
> apply anymore, and I think it would deserve not being buried in that
> other patch's big thread.

I'll get this updated and send a new version today.

Thanks,
Andrew

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

[Andrew Burgess]

"Rohr, Stephan" <stephan.rohr@intel.com> writes:

>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Monday, 6 October 2025 14:11
>> To: Sébastien Darche <sdarche@efficios.com>; simark@simark.ca
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH] gdb: ensure bp_location::section is set correct to avoid an
>> assert
>> 
>> Sébastien Darche <sdarche@efficios.com> writes:
>> 
>> > On 9/25/25 17:40, Andrew Burgess wrote:
>> >> Maybe the answer is as simple as moving the .section assignment into the
>> >> earlier if block, something like:
>> >>
>> >>    if (is_function && want_start_sal)
>> >>      {
>> >>        sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
>> >>
>> >>        /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
>> >>           anywhere, in a completely different section to MSYMBOL, or even
>> >>           in a different objfile!
>> >>
>> >>           TODO: I haven't checked, maybe find_function_start_sal already
>> >>           fills this stuff in for us?  Or maybe it could be made too?
>> >>           For now I'm assuming all we have is an address, but this needs
>> >>           checking.  */
>> >>        sal.section = find_pc_overlay (func_addr);
>> >>        if (sal.section == nullptr)
>> >>          sal.section = find_pc_section (func_addr);
>> >>      }
>> >>    else
>> >>      {
>> >>        sal.objfile = objfile;
>> >>        sal.msymbol = msymbol;
>> >>        /* Store func_addr, not the minsym's address in case this was an
>> >> 	 ifunc that hasn't been resolved yet.  */
>> >>        if (is_function)
>> >> 	sal.pc = func_addr;
>> >>        else
>> >> 	sal.pc = msymbol->value_address (objfile);
>> >>        sal.pspace = current_program_space;
>> >>
>> >>        /* We can assign the section based on MSYMBOL here because the
>> >>           breakpoint is actually being placed at (or near) MSYMBOL.  */
>> >>        sal.section = msymbol->obj_section (objfile);
>> >>      }
>> >>
>> >
>> > To answer your question on whether find_function_start_sal does fill
>> > this for us : it depends. It manages to do it on amd64 but not on amdgpu.
>> >
>> > By default, the sal does not contain a valid section. It's only when we
>> > try to adjust the pc past the prologue (skip_prologue_sal) that a
>> > section is computed for the pc at the start of the function. If we do
>> > have a prologue, then we assign that section (symtab.c:3914). If not
>> > (and that is the case on amdgpu), then we're left with an empty
>> > sal.section. I would say the behavior is not really consistent.
>> >
>> > I would agree it could be made to.
>> >> Does this look like a valid path forward maybe?
>> >
>> > Your solution seems to work for the gnu-ifunc test and fixes the
>> > regression for gdb.rocm/displaced-stepping.exp - so I'd say it's a good
>> > aproach. I am not familiar with overlays, so I can't really judge if the
>> > change would impact how they are handled.
>> >
>> > I think it would be best to ensure find_function_start_sal has a
>> > consistent behavior across architectures. I'll submit a small patch
>> > which should address this. This would also at least reduce the chance
>> > for another bug like this to appear somewhere else :
>> >
>> 
>> Hi,
>> 
>> I'm proposing the patch below.  You should double check that this still
>> addresses the issue you're seeing with the amdgpu target.  Given Simon's
>> concerns, I do wonder if there might still be some issues with this
>> related to overlay debugging, but without any way to test it, and no
>> known overlay users, I think we can probably just ignore that for now.
>> 
>> If this fixes your regression, then maybe we should merge this, and
>> figure any other issues out later?
>> 
>> Thanks,
>> Andrew
>> 
>> ---
>> 
>> commit 6ffea587445eeacf8b2962de6d3b00d6efa98213
>> Author: Andrew Burgess <aburgess@redhat.com>
>> Date:   Mon Oct 6 10:27:08 2025 +0100
>> 
>>     gdb: fixes for setting the section in minsym_found
>> 
>>     After this commit:
>> 
>>       commit 6f7ad2381ae72aa592ada4a0921265aa3292b1fa
>>       Date:   Wed Sep 3 19:57:42 2025 +0100
>> 
>>           gdb: ensure bp_location::section is set correct to avoid an assert
>> 
>>     Some issues were reported as a result of the bp_location::section
>>     being left as NULL by the call to find_pc_overlay that was
>>     introduced.  See this thread:
>> 
>>       https://inbox.sourceware.org/gdb-patches/7febb0c1-7bbd-45d5-8ebe-
>> 91c34bb4a6ce@efficios.com
>> 
>>     The problem was that code_breakpoint::add_location relies on the
>>     section being set in order to deduce the gdbarch.  If the section is
>>     not set then the gdbarch is deduced using the breakpoint's gdbarch.
>> 
>>     The bug was reported by the ROCm engineers, who have inferiors running
>>     mixed host and GPU code, and so rely on the section being set in order
>>     to establish the correct architecture for a specific address.
>> 
>>     During discussion in the above thread Simon pointed out that the
>>     change made in the above commit might not be correct anyway for
>>     overlay debugging (does that even work, or is it used any more?), as
>>     the commit relies on establishing a section by calling
>>     find_pc_overlay.  However, when presented with multiple possible
>>     sections, find_pc_overlay cannot know which section to select, and so
>>     just picks one.  This could be different from the section of the
>>     minimal_symbol we already had to hand.
>> 
>>     This patch I think should (at least) resolve the issues the ROCm
>>     engineers are seeing.
>> 
>>     Instead of always calling find_pc_overlay I have moved the section
>>     assignment inside the if/then/else blocks with the following
>>     reasoning.
>> 
>>     In the 'else' block, this is the non-function or non-ifunc case, the
>>     address used is based on the msymbol's address, and so should be in
>>     the same section.  In this case we can use the msymbol's section.
>> 
>>     In the 'if' block things are more complicated.  This could be the
>>     ifunc case, in which case func_addr could have been adjusted to a
>>     different section, or even different objfile.
>> 
>>     Further, when we call find_function_start_sal, we pass in just an
>>     address, so the SAL being returned isn't going to consider which
>>     overlay section the original msymbol was from, which could cause
>>     problems for overlay debugging maybe?
>> 
>>     Anyway, I'm ignoring that for now, as fixing that would be a whole big
>>     thing.  So I'm proposing that, if find_function_start_sal returns a
>>     symtab_and_line with a section set, then we use that section.
>>     Otherwise, we can try to figure out a section.
>> 
>> diff --git a/gdb/linespec.c b/gdb/linespec.c
>> index 2ddc495babf..4d9c5ac26f3 100644
>> --- a/gdb/linespec.c
>> +++ b/gdb/linespec.c
>> @@ -4083,13 +4083,16 @@ minsym_found (struct linespec_state *self,
>> struct objfile *objfile,
>> 
>>    CORE_ADDR func_addr;
>>    bool is_function = msymbol_is_function (objfile, msymbol, &func_addr);
>> +  bool is_ifunc = false;
>> 
>>    if (is_function)
>>      {
>>        const char *msym_name = msymbol->linkage_name ();
>> 
>> -      if (msymbol->type () == mst_text_gnu_ifunc
>> -	  || msymbol->type () == mst_data_gnu_ifunc)
>> +      is_ifunc = (msymbol->type () == mst_text_gnu_ifunc
>> +		  || msymbol->type () == mst_data_gnu_ifunc);
>> +
>> +      if (is_ifunc)
>>  	want_start_sal = gnu_ifunc_resolve_name (msym_name,
>> &func_addr);
>>        else
>>  	want_start_sal = true;
>> @@ -4098,7 +4101,32 @@ minsym_found (struct linespec_state *self, struct
>> objfile *objfile,
>>    symtab_and_line sal;
>> 
>>    if (is_function && want_start_sal)
>> -    sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
>> +    {
>> +      sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
>> +
>> +      /* If SAL already has a section then we'll use that.  If not, then we
>> +	 can try to find a section.
>> +
>> +	 In the ifunc case though we cannot rely on the section of MSYMBOL,
>> +	 the ifunc target could be in a different section, or even a
>> +	 different objfile, from the original MSYMBOL.  For this case, we
>> +	 fall back to looking up a section based on FUNC_ADDR.
>> +
>> +	 For the non-ifunc case, we can use the section of MSYMBOL, as
>> +	 that's how we filled in FUNC_ADDR, so they should be in the same
>> +	 section.  */
>> +      if (sal.section == nullptr)
>> +	{
>> +	  if (!is_ifunc)
>> +	    sal.section = msymbol->obj_section (objfile);
>> +	  else
>> +	    {
>> +	      sal.section = find_pc_overlay (func_addr);
>> +	      if (sal.section == nullptr)
>> +		sal.section = find_pc_section (func_addr);
>> +	    }
>> +	}
>> +    }
>>    else
>>      {
>>        sal.objfile = objfile;
>> @@ -4110,14 +4138,13 @@ minsym_found (struct linespec_state *self,
>> struct objfile *objfile,
>>        else
>>  	sal.pc = msymbol->value_address (objfile);
>>        sal.pspace = current_program_space;
>> -    }
>> 
>> -  /* Don't use the section from the msymbol, the code above might have
>> -     adjusted FUNC_ADDR, in which case the msymbol's section might not be
>> -     the section containing FUNC_ADDR.  It might not even be in the same
>> -     objfile.  As the section is primarily to assist with overlay
>> -     debugging, it should reflect the SAL's pc value.  */
>> -  sal.section = find_pc_overlay (sal.pc);
>> +      /* We can assign the section based on MSYMBOL here because the
>> +	 breakpoint is actually being placed at (or near) MSYMBOL.  Note,
>> +	 this is not a path where ifunc resolution can have occurred, which
>> +	 could adjust FUNC_ADDR significantly.  */
>> +      sal.section = msymbol->obj_section (objfile);
>> +    }
>> 
>>    if (self->maybe_add_address (objfile->pspace (), sal.pc))
>>      add_sal_to_sals (self, result, &sal, msymbol->natural_name (), false);
>
> Hi all,
>
> I came across the same issue when debugging a remote inferior that is compiled w/o debug symbols and a
> breakpoint is inserted based on the function name that is called in the inferior. 
>
> Our test started to regress with the introduction of patch
> "gdb: ensure bp_location::section is set correct to avoid an assert".  As mentioned in the patch above,
> GDB uses the default gdbarch to insert the breakpoint if the section is NULL.  This causes issues later. 
>
> I applied the patch; our breakpoint insertion issue is fixed with this patch.  I reviewed the patch, it is
> reasonable to me.  Only thing I wonder is the usage of 'find_pc_ovelay'; this always returns NULL if
> overlay debugging is not used? But I guess that's fixed by using 'find_pc_section' in this case.

Hey Stephan,

Could you confirm if you were testing against master, or against a
release branch?  If you're using a release branch, could you check if
the issue is still present on master.

I'm surprised that commit 539fc2164f44a doesn't fix the issue you're
seeing, but this is only available on master right now.

If you are testing on master, and you still applied my patch then you
must have resolved the merge conflicts, could you post your diff so I
can see how you resolved things.

Thanks,
Andrew

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

[Rohr, Stephan]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Wednesday, 4 February 2026 14:00
> To: Rohr, Stephan <stephan.rohr@intel.com>
> Cc: gdb-patches@sourceware.org; Sébastien Darche <sdarche@efficios.com>;
> simark@simark.ca
> Subject: RE: [PATCH] gdb: ensure bp_location::section is set correct to avoid an
> assert
> 
> "Rohr, Stephan" <stephan.rohr@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Andrew Burgess <aburgess@redhat.com>
> >> Sent: Monday, 6 October 2025 14:11
> >> To: Sébastien Darche <sdarche@efficios.com>; simark@simark.ca
> >> Cc: gdb-patches@sourceware.org
> >> Subject: Re: [PATCH] gdb: ensure bp_location::section is set correct to avoid
> an
> >> assert
> >>
> >> Sébastien Darche <sdarche@efficios.com> writes:
> >>
> >> > On 9/25/25 17:40, Andrew Burgess wrote:
> >> >> Maybe the answer is as simple as moving the .section assignment into
> the
> >> >> earlier if block, something like:
> >> >>
> >> >>    if (is_function && want_start_sal)
> >> >>      {
> >> >>        sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> >> >>
> >> >>        /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
> >> >>           anywhere, in a completely different section to MSYMBOL, or even
> >> >>           in a different objfile!
> >> >>
> >> >>           TODO: I haven't checked, maybe find_function_start_sal already
> >> >>           fills this stuff in for us?  Or maybe it could be made too?
> >> >>           For now I'm assuming all we have is an address, but this needs
> >> >>           checking.  */
> >> >>        sal.section = find_pc_overlay (func_addr);
> >> >>        if (sal.section == nullptr)
> >> >>          sal.section = find_pc_section (func_addr);
> >> >>      }
> >> >>    else
> >> >>      {
> >> >>        sal.objfile = objfile;
> >> >>        sal.msymbol = msymbol;
> >> >>        /* Store func_addr, not the minsym's address in case this was an
> >> >> 	 ifunc that hasn't been resolved yet.  */
> >> >>        if (is_function)
> >> >> 	sal.pc = func_addr;
> >> >>        else
> >> >> 	sal.pc = msymbol->value_address (objfile);
> >> >>        sal.pspace = current_program_space;
> >> >>
> >> >>        /* We can assign the section based on MSYMBOL here because the
> >> >>           breakpoint is actually being placed at (or near) MSYMBOL.  */
> >> >>        sal.section = msymbol->obj_section (objfile);
> >> >>      }
> >> >>
> >> >
> >> > To answer your question on whether find_function_start_sal does fill
> >> > this for us : it depends. It manages to do it on amd64 but not on amdgpu.
> >> >
> >> > By default, the sal does not contain a valid section. It's only when we
> >> > try to adjust the pc past the prologue (skip_prologue_sal) that a
> >> > section is computed for the pc at the start of the function. If we do
> >> > have a prologue, then we assign that section (symtab.c:3914). If not
> >> > (and that is the case on amdgpu), then we're left with an empty
> >> > sal.section. I would say the behavior is not really consistent.
> >> >
> >> > I would agree it could be made to.
> >> >> Does this look like a valid path forward maybe?
> >> >
> >> > Your solution seems to work for the gnu-ifunc test and fixes the
> >> > regression for gdb.rocm/displaced-stepping.exp - so I'd say it's a good
> >> > aproach. I am not familiar with overlays, so I can't really judge if the
> >> > change would impact how they are handled.
> >> >
> >> > I think it would be best to ensure find_function_start_sal has a
> >> > consistent behavior across architectures. I'll submit a small patch
> >> > which should address this. This would also at least reduce the chance
> >> > for another bug like this to appear somewhere else :
> >> >
> >>
> >> Hi,
> >>
> >> I'm proposing the patch below.  You should double check that this still
> >> addresses the issue you're seeing with the amdgpu target.  Given Simon's
> >> concerns, I do wonder if there might still be some issues with this
> >> related to overlay debugging, but without any way to test it, and no
> >> known overlay users, I think we can probably just ignore that for now.
> >>
> >> If this fixes your regression, then maybe we should merge this, and
> >> figure any other issues out later?
> >>
> >> Thanks,
> >> Andrew
> >>
> >> ---
> >>
> >> commit 6ffea587445eeacf8b2962de6d3b00d6efa98213
> >> Author: Andrew Burgess <aburgess@redhat.com>
> >> Date:   Mon Oct 6 10:27:08 2025 +0100
> >>
> >>     gdb: fixes for setting the section in minsym_found
> >>
> >>     After this commit:
> >>
> >>       commit 6f7ad2381ae72aa592ada4a0921265aa3292b1fa
> >>       Date:   Wed Sep 3 19:57:42 2025 +0100
> >>
> >>           gdb: ensure bp_location::section is set correct to avoid an assert
> >>
> >>     Some issues were reported as a result of the bp_location::section
> >>     being left as NULL by the call to find_pc_overlay that was
> >>     introduced.  See this thread:
> >>
> >>       https://inbox.sourceware.org/gdb-patches/7febb0c1-7bbd-45d5-
> 8ebe-
> >> 91c34bb4a6ce@efficios.com
> >>
> >>     The problem was that code_breakpoint::add_location relies on the
> >>     section being set in order to deduce the gdbarch.  If the section is
> >>     not set then the gdbarch is deduced using the breakpoint's gdbarch.
> >>
> >>     The bug was reported by the ROCm engineers, who have inferiors
> running
> >>     mixed host and GPU code, and so rely on the section being set in order
> >>     to establish the correct architecture for a specific address.
> >>
> >>     During discussion in the above thread Simon pointed out that the
> >>     change made in the above commit might not be correct anyway for
> >>     overlay debugging (does that even work, or is it used any more?), as
> >>     the commit relies on establishing a section by calling
> >>     find_pc_overlay.  However, when presented with multiple possible
> >>     sections, find_pc_overlay cannot know which section to select, and so
> >>     just picks one.  This could be different from the section of the
> >>     minimal_symbol we already had to hand.
> >>
> >>     This patch I think should (at least) resolve the issues the ROCm
> >>     engineers are seeing.
> >>
> >>     Instead of always calling find_pc_overlay I have moved the section
> >>     assignment inside the if/then/else blocks with the following
> >>     reasoning.
> >>
> >>     In the 'else' block, this is the non-function or non-ifunc case, the
> >>     address used is based on the msymbol's address, and so should be in
> >>     the same section.  In this case we can use the msymbol's section.
> >>
> >>     In the 'if' block things are more complicated.  This could be the
> >>     ifunc case, in which case func_addr could have been adjusted to a
> >>     different section, or even different objfile.
> >>
> >>     Further, when we call find_function_start_sal, we pass in just an
> >>     address, so the SAL being returned isn't going to consider which
> >>     overlay section the original msymbol was from, which could cause
> >>     problems for overlay debugging maybe?
> >>
> >>     Anyway, I'm ignoring that for now, as fixing that would be a whole big
> >>     thing.  So I'm proposing that, if find_function_start_sal returns a
> >>     symtab_and_line with a section set, then we use that section.
> >>     Otherwise, we can try to figure out a section.
> >>
> >> diff --git a/gdb/linespec.c b/gdb/linespec.c
> >> index 2ddc495babf..4d9c5ac26f3 100644
> >> --- a/gdb/linespec.c
> >> +++ b/gdb/linespec.c
> >> @@ -4083,13 +4083,16 @@ minsym_found (struct linespec_state *self,
> >> struct objfile *objfile,
> >>
> >>    CORE_ADDR func_addr;
> >>    bool is_function = msymbol_is_function (objfile, msymbol, &func_addr);
> >> +  bool is_ifunc = false;
> >>
> >>    if (is_function)
> >>      {
> >>        const char *msym_name = msymbol->linkage_name ();
> >>
> >> -      if (msymbol->type () == mst_text_gnu_ifunc
> >> -	  || msymbol->type () == mst_data_gnu_ifunc)
> >> +      is_ifunc = (msymbol->type () == mst_text_gnu_ifunc
> >> +		  || msymbol->type () == mst_data_gnu_ifunc);
> >> +
> >> +      if (is_ifunc)
> >>  	want_start_sal = gnu_ifunc_resolve_name (msym_name,
> >> &func_addr);
> >>        else
> >>  	want_start_sal = true;
> >> @@ -4098,7 +4101,32 @@ minsym_found (struct linespec_state *self,
> struct
> >> objfile *objfile,
> >>    symtab_and_line sal;
> >>
> >>    if (is_function && want_start_sal)
> >> -    sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> >> +    {
> >> +      sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
> >> +
> >> +      /* If SAL already has a section then we'll use that.  If not, then we
> >> +	 can try to find a section.
> >> +
> >> +	 In the ifunc case though we cannot rely on the section of MSYMBOL,
> >> +	 the ifunc target could be in a different section, or even a
> >> +	 different objfile, from the original MSYMBOL.  For this case, we
> >> +	 fall back to looking up a section based on FUNC_ADDR.
> >> +
> >> +	 For the non-ifunc case, we can use the section of MSYMBOL, as
> >> +	 that's how we filled in FUNC_ADDR, so they should be in the same
> >> +	 section.  */
> >> +      if (sal.section == nullptr)
> >> +	{
> >> +	  if (!is_ifunc)
> >> +	    sal.section = msymbol->obj_section (objfile);
> >> +	  else
> >> +	    {
> >> +	      sal.section = find_pc_overlay (func_addr);
> >> +	      if (sal.section == nullptr)
> >> +		sal.section = find_pc_section (func_addr);
> >> +	    }
> >> +	}
> >> +    }
> >>    else
> >>      {
> >>        sal.objfile = objfile;
> >> @@ -4110,14 +4138,13 @@ minsym_found (struct linespec_state *self,
> >> struct objfile *objfile,
> >>        else
> >>  	sal.pc = msymbol->value_address (objfile);
> >>        sal.pspace = current_program_space;
> >> -    }
> >>
> >> -  /* Don't use the section from the msymbol, the code above might have
> >> -     adjusted FUNC_ADDR, in which case the msymbol's section might not
> be
> >> -     the section containing FUNC_ADDR.  It might not even be in the same
> >> -     objfile.  As the section is primarily to assist with overlay
> >> -     debugging, it should reflect the SAL's pc value.  */
> >> -  sal.section = find_pc_overlay (sal.pc);
> >> +      /* We can assign the section based on MSYMBOL here because the
> >> +	 breakpoint is actually being placed at (or near) MSYMBOL.  Note,
> >> +	 this is not a path where ifunc resolution can have occurred, which
> >> +	 could adjust FUNC_ADDR significantly.  */
> >> +      sal.section = msymbol->obj_section (objfile);
> >> +    }
> >>
> >>    if (self->maybe_add_address (objfile->pspace (), sal.pc))
> >>      add_sal_to_sals (self, result, &sal, msymbol->natural_name (), false);
> >
> > Hi all,
> >
> > I came across the same issue when debugging a remote inferior that is
> compiled w/o debug symbols and a
> > breakpoint is inserted based on the function name that is called in the
> inferior.
> >
> > Our test started to regress with the introduction of patch
> > "gdb: ensure bp_location::section is set correct to avoid an assert".  As
> mentioned in the patch above,
> > GDB uses the default gdbarch to insert the breakpoint if the section is NULL.
> This causes issues later.
> >
> > I applied the patch; our breakpoint insertion issue is fixed with this patch.  I
> reviewed the patch, it is
> > reasonable to me.  Only thing I wonder is the usage of 'find_pc_ovelay'; this
> always returns NULL if
> > overlay debugging is not used? But I guess that's fixed by using
> 'find_pc_section' in this case.
> 
> Hey Stephan,
> 
> Could you confirm if you were testing against master, or against a
> release branch?  If you're using a release branch, could you check if
> the issue is still present on master.
> 
> I'm surprised that commit 539fc2164f44a doesn't fix the issue you're
> seeing, but this is only available on master right now.
> 
> If you are testing on master, and you still applied my patch then you
> must have resolved the merge conflicts, could you post your diff so I
> can see how you resolved things.
> 
> Thanks,
> Andrew


Hi Andrew,

I was testing against the GDB 17.1 release branch, with Intel GPU specific patches
on top.

I'm not able to test against master as I would need to cherry-pick those
patches first.

I applied your patch on our internal branch before I tried the patch series from Sébastien,
which is on master but not on the GDB 17.1 release.  The patch you originally submitted
fixed the issue.

Next, I cherry-picked the commits from Sébastien's patch series w/o applying your patch,
which also works in my case.  With these applied, your patch doesn't apply cleanly
anymore in the "else" branch.

I wonder if we still need the "else" part as we're now obtaining the section based on
the "msymbol" at the beginning of the function and asserting it is not NULL in the
else branch.

Thanks
Stephan

Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928

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

[Andrew Burgess]

"Rohr, Stephan" <stephan.rohr@intel.com> writes:

>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Wednesday, 4 February 2026 14:00
>> To: Rohr, Stephan <stephan.rohr@intel.com>
>> Cc: gdb-patches@sourceware.org; Sébastien Darche <sdarche@efficios.com>;
>> simark@simark.ca
>> Subject: RE: [PATCH] gdb: ensure bp_location::section is set correct to avoid an
>> assert
>> 
>> "Rohr, Stephan" <stephan.rohr@intel.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Andrew Burgess <aburgess@redhat.com>
>> >> Sent: Monday, 6 October 2025 14:11
>> >> To: Sébastien Darche <sdarche@efficios.com>; simark@simark.ca
>> >> Cc: gdb-patches@sourceware.org
>> >> Subject: Re: [PATCH] gdb: ensure bp_location::section is set correct to avoid
>> an
>> >> assert
>> >>
>> >> Sébastien Darche <sdarche@efficios.com> writes:
>> >>
>> >> > On 9/25/25 17:40, Andrew Burgess wrote:
>> >> >> Maybe the answer is as simple as moving the .section assignment into
>> the
>> >> >> earlier if block, something like:
>> >> >>
>> >> >>    if (is_function && want_start_sal)
>> >> >>      {
>> >> >>        sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
>> >> >>
>> >> >>        /* This breakpoint is for the ifunc case, FUNC_ADDR is can be
>> >> >>           anywhere, in a completely different section to MSYMBOL, or even
>> >> >>           in a different objfile!
>> >> >>
>> >> >>           TODO: I haven't checked, maybe find_function_start_sal already
>> >> >>           fills this stuff in for us?  Or maybe it could be made too?
>> >> >>           For now I'm assuming all we have is an address, but this needs
>> >> >>           checking.  */
>> >> >>        sal.section = find_pc_overlay (func_addr);
>> >> >>        if (sal.section == nullptr)
>> >> >>          sal.section = find_pc_section (func_addr);
>> >> >>      }
>> >> >>    else
>> >> >>      {
>> >> >>        sal.objfile = objfile;
>> >> >>        sal.msymbol = msymbol;
>> >> >>        /* Store func_addr, not the minsym's address in case this was an
>> >> >> 	 ifunc that hasn't been resolved yet.  */
>> >> >>        if (is_function)
>> >> >> 	sal.pc = func_addr;
>> >> >>        else
>> >> >> 	sal.pc = msymbol->value_address (objfile);
>> >> >>        sal.pspace = current_program_space;
>> >> >>
>> >> >>        /* We can assign the section based on MSYMBOL here because the
>> >> >>           breakpoint is actually being placed at (or near) MSYMBOL.  */
>> >> >>        sal.section = msymbol->obj_section (objfile);
>> >> >>      }
>> >> >>
>> >> >
>> >> > To answer your question on whether find_function_start_sal does fill
>> >> > this for us : it depends. It manages to do it on amd64 but not on amdgpu.
>> >> >
>> >> > By default, the sal does not contain a valid section. It's only when we
>> >> > try to adjust the pc past the prologue (skip_prologue_sal) that a
>> >> > section is computed for the pc at the start of the function. If we do
>> >> > have a prologue, then we assign that section (symtab.c:3914). If not
>> >> > (and that is the case on amdgpu), then we're left with an empty
>> >> > sal.section. I would say the behavior is not really consistent.
>> >> >
>> >> > I would agree it could be made to.
>> >> >> Does this look like a valid path forward maybe?
>> >> >
>> >> > Your solution seems to work for the gnu-ifunc test and fixes the
>> >> > regression for gdb.rocm/displaced-stepping.exp - so I'd say it's a good
>> >> > aproach. I am not familiar with overlays, so I can't really judge if the
>> >> > change would impact how they are handled.
>> >> >
>> >> > I think it would be best to ensure find_function_start_sal has a
>> >> > consistent behavior across architectures. I'll submit a small patch
>> >> > which should address this. This would also at least reduce the chance
>> >> > for another bug like this to appear somewhere else :
>> >> >
>> >>
>> >> Hi,
>> >>
>> >> I'm proposing the patch below.  You should double check that this still
>> >> addresses the issue you're seeing with the amdgpu target.  Given Simon's
>> >> concerns, I do wonder if there might still be some issues with this
>> >> related to overlay debugging, but without any way to test it, and no
>> >> known overlay users, I think we can probably just ignore that for now.
>> >>
>> >> If this fixes your regression, then maybe we should merge this, and
>> >> figure any other issues out later?
>> >>
>> >> Thanks,
>> >> Andrew
>> >>
>> >> ---
>> >>
>> >> commit 6ffea587445eeacf8b2962de6d3b00d6efa98213
>> >> Author: Andrew Burgess <aburgess@redhat.com>
>> >> Date:   Mon Oct 6 10:27:08 2025 +0100
>> >>
>> >>     gdb: fixes for setting the section in minsym_found
>> >>
>> >>     After this commit:
>> >>
>> >>       commit 6f7ad2381ae72aa592ada4a0921265aa3292b1fa
>> >>       Date:   Wed Sep 3 19:57:42 2025 +0100
>> >>
>> >>           gdb: ensure bp_location::section is set correct to avoid an assert
>> >>
>> >>     Some issues were reported as a result of the bp_location::section
>> >>     being left as NULL by the call to find_pc_overlay that was
>> >>     introduced.  See this thread:
>> >>
>> >>       https://inbox.sourceware.org/gdb-patches/7febb0c1-7bbd-45d5-
>> 8ebe-
>> >> 91c34bb4a6ce@efficios.com
>> >>
>> >>     The problem was that code_breakpoint::add_location relies on the
>> >>     section being set in order to deduce the gdbarch.  If the section is
>> >>     not set then the gdbarch is deduced using the breakpoint's gdbarch.
>> >>
>> >>     The bug was reported by the ROCm engineers, who have inferiors
>> running
>> >>     mixed host and GPU code, and so rely on the section being set in order
>> >>     to establish the correct architecture for a specific address.
>> >>
>> >>     During discussion in the above thread Simon pointed out that the
>> >>     change made in the above commit might not be correct anyway for
>> >>     overlay debugging (does that even work, or is it used any more?), as
>> >>     the commit relies on establishing a section by calling
>> >>     find_pc_overlay.  However, when presented with multiple possible
>> >>     sections, find_pc_overlay cannot know which section to select, and so
>> >>     just picks one.  This could be different from the section of the
>> >>     minimal_symbol we already had to hand.
>> >>
>> >>     This patch I think should (at least) resolve the issues the ROCm
>> >>     engineers are seeing.
>> >>
>> >>     Instead of always calling find_pc_overlay I have moved the section
>> >>     assignment inside the if/then/else blocks with the following
>> >>     reasoning.
>> >>
>> >>     In the 'else' block, this is the non-function or non-ifunc case, the
>> >>     address used is based on the msymbol's address, and so should be in
>> >>     the same section.  In this case we can use the msymbol's section.
>> >>
>> >>     In the 'if' block things are more complicated.  This could be the
>> >>     ifunc case, in which case func_addr could have been adjusted to a
>> >>     different section, or even different objfile.
>> >>
>> >>     Further, when we call find_function_start_sal, we pass in just an
>> >>     address, so the SAL being returned isn't going to consider which
>> >>     overlay section the original msymbol was from, which could cause
>> >>     problems for overlay debugging maybe?
>> >>
>> >>     Anyway, I'm ignoring that for now, as fixing that would be a whole big
>> >>     thing.  So I'm proposing that, if find_function_start_sal returns a
>> >>     symtab_and_line with a section set, then we use that section.
>> >>     Otherwise, we can try to figure out a section.
>> >>
>> >> diff --git a/gdb/linespec.c b/gdb/linespec.c
>> >> index 2ddc495babf..4d9c5ac26f3 100644
>> >> --- a/gdb/linespec.c
>> >> +++ b/gdb/linespec.c
>> >> @@ -4083,13 +4083,16 @@ minsym_found (struct linespec_state *self,
>> >> struct objfile *objfile,
>> >>
>> >>    CORE_ADDR func_addr;
>> >>    bool is_function = msymbol_is_function (objfile, msymbol, &func_addr);
>> >> +  bool is_ifunc = false;
>> >>
>> >>    if (is_function)
>> >>      {
>> >>        const char *msym_name = msymbol->linkage_name ();
>> >>
>> >> -      if (msymbol->type () == mst_text_gnu_ifunc
>> >> -	  || msymbol->type () == mst_data_gnu_ifunc)
>> >> +      is_ifunc = (msymbol->type () == mst_text_gnu_ifunc
>> >> +		  || msymbol->type () == mst_data_gnu_ifunc);
>> >> +
>> >> +      if (is_ifunc)
>> >>  	want_start_sal = gnu_ifunc_resolve_name (msym_name,
>> >> &func_addr);
>> >>        else
>> >>  	want_start_sal = true;
>> >> @@ -4098,7 +4101,32 @@ minsym_found (struct linespec_state *self,
>> struct
>> >> objfile *objfile,
>> >>    symtab_and_line sal;
>> >>
>> >>    if (is_function && want_start_sal)
>> >> -    sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
>> >> +    {
>> >> +      sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
>> >> +
>> >> +      /* If SAL already has a section then we'll use that.  If not, then we
>> >> +	 can try to find a section.
>> >> +
>> >> +	 In the ifunc case though we cannot rely on the section of MSYMBOL,
>> >> +	 the ifunc target could be in a different section, or even a
>> >> +	 different objfile, from the original MSYMBOL.  For this case, we
>> >> +	 fall back to looking up a section based on FUNC_ADDR.
>> >> +
>> >> +	 For the non-ifunc case, we can use the section of MSYMBOL, as
>> >> +	 that's how we filled in FUNC_ADDR, so they should be in the same
>> >> +	 section.  */
>> >> +      if (sal.section == nullptr)
>> >> +	{
>> >> +	  if (!is_ifunc)
>> >> +	    sal.section = msymbol->obj_section (objfile);
>> >> +	  else
>> >> +	    {
>> >> +	      sal.section = find_pc_overlay (func_addr);
>> >> +	      if (sal.section == nullptr)
>> >> +		sal.section = find_pc_section (func_addr);
>> >> +	    }
>> >> +	}
>> >> +    }
>> >>    else
>> >>      {
>> >>        sal.objfile = objfile;
>> >> @@ -4110,14 +4138,13 @@ minsym_found (struct linespec_state *self,
>> >> struct objfile *objfile,
>> >>        else
>> >>  	sal.pc = msymbol->value_address (objfile);
>> >>        sal.pspace = current_program_space;
>> >> -    }
>> >>
>> >> -  /* Don't use the section from the msymbol, the code above might have
>> >> -     adjusted FUNC_ADDR, in which case the msymbol's section might not
>> be
>> >> -     the section containing FUNC_ADDR.  It might not even be in the same
>> >> -     objfile.  As the section is primarily to assist with overlay
>> >> -     debugging, it should reflect the SAL's pc value.  */
>> >> -  sal.section = find_pc_overlay (sal.pc);
>> >> +      /* We can assign the section based on MSYMBOL here because the
>> >> +	 breakpoint is actually being placed at (or near) MSYMBOL.  Note,
>> >> +	 this is not a path where ifunc resolution can have occurred, which
>> >> +	 could adjust FUNC_ADDR significantly.  */
>> >> +      sal.section = msymbol->obj_section (objfile);
>> >> +    }
>> >>
>> >>    if (self->maybe_add_address (objfile->pspace (), sal.pc))
>> >>      add_sal_to_sals (self, result, &sal, msymbol->natural_name (), false);
>> >
>> > Hi all,
>> >
>> > I came across the same issue when debugging a remote inferior that is
>> compiled w/o debug symbols and a
>> > breakpoint is inserted based on the function name that is called in the
>> inferior.
>> >
>> > Our test started to regress with the introduction of patch
>> > "gdb: ensure bp_location::section is set correct to avoid an assert".  As
>> mentioned in the patch above,
>> > GDB uses the default gdbarch to insert the breakpoint if the section is NULL.
>> This causes issues later.
>> >
>> > I applied the patch; our breakpoint insertion issue is fixed with this patch.  I
>> reviewed the patch, it is
>> > reasonable to me.  Only thing I wonder is the usage of 'find_pc_ovelay'; this
>> always returns NULL if
>> > overlay debugging is not used? But I guess that's fixed by using
>> 'find_pc_section' in this case.
>> 
>> Hey Stephan,
>> 
>> Could you confirm if you were testing against master, or against a
>> release branch?  If you're using a release branch, could you check if
>> the issue is still present on master.
>> 
>> I'm surprised that commit 539fc2164f44a doesn't fix the issue you're
>> seeing, but this is only available on master right now.
>> 
>> If you are testing on master, and you still applied my patch then you
>> must have resolved the merge conflicts, could you post your diff so I
>> can see how you resolved things.
>> 
>> Thanks,
>> Andrew
>
>
> Hi Andrew,
>
> I was testing against the GDB 17.1 release branch, with Intel GPU specific patches
> on top.
>
> I'm not able to test against master as I would need to cherry-pick those
> patches first.
>
> I applied your patch on our internal branch before I tried the patch series from Sébastien,
> which is on master but not on the GDB 17.1 release.  The patch you originally submitted
> fixed the issue.
>
> Next, I cherry-picked the commits from Sébastien's patch series w/o applying your patch,
> which also works in my case.  With these applied, your patch doesn't apply cleanly
> anymore in the "else" branch.
>
> I wonder if we still need the "else" part as we're now obtaining the section based on
> the "msymbol" at the beginning of the function and asserting it is not NULL in the
> else branch.

I think the assert is fine, at least for ELF input.  I believe that
every ELF symbol is assigned to a section, if the symbol isn't
associated with a section in the ELF then I believe either BFD or GDB
assigns it to the absolute section as a fall back.  At least, that's my
understanding.

Now it's possible some none-ELF file type does things differently, in
which case, I guess this assert could be a problem then.  But even if
the assert did trigger, it's not clear if the right solution would be to
change the assert to handle the NULL case, or to change how msymbols are
parsed such that every symbol has a section.

Until I have an actual example before me that triggers the assert, I'm
inclined to leave it be for now.

Thanks,
Andrew