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