[PATCH 2/2] gdb: Fix setting breakpoint by address in heterogeneous systems

[Simon Marchi]

On 11/7/24 11:28 AM, Lancelot SIX wrote:
> On heterogeneous systems, parts of a given program run on different
> pieces of hardware with different architectures.  If one tries to set a
> breakpoint by address in a code section which does not have debug
> information available, then GDB will take the current architecture to
> figure out the breakpoint instruction to use.  If the breakpoint needs
> to be inserted in a code section that executes on the non-current
> architecture, GDB ends up using the wrong instruction to set-up a
> breakpoint.

Can you clarify what "current" and "non-current" architecture mean
exactly?  I guess it ultimately means the current inferior's
architecture, as in `inferior->arch ()`?

> We can see that when debugging AMDGPU programs.  Setting a breakpoint by
> address in a GPU program while focusing on CPU (x86_64) leads GDB to
> use '0xcc' (int3) as a breakpoint instruction instead of '0xbf920001'
> (s_trap 1).

... from this paragraph, I understand that if you are focusing on a GPU
thread, then it works, so "current architecture" would mean architecture
of the current thread (as in `target_thread_architecture (ptid)`?

> 
> This patch proposes to fix that by having
> convert_address_location_to_sals look which objfile contains the address
> and record this in the associated symbol_and_line.  This patch also
> updates get_sal_arch to look at the sal's objfile if set.
> 
> Another approach we could have used is to update find_pc_sect_line to
> record the msymbol (and objfile) if found by
> lookup_minimal_symbol_by_pc, but this would not handle cases where even
> msymbols are not available.
> 
> Reviewed-By: Pedro Alves <pedro@palves.net>
> Change-Id: If723b59cb8d198c62fef74b46db5d5a8521847c9
> ---
>  gdb/breakpoint.c                              |  2 +
>  gdb/linespec.c                                |  7 ++
>  .../gdb.rocm/addr-bp-gpu-no-deb-info.cpp      | 30 +++++++++
>  .../gdb.rocm/addr-bp-gpu-no-deb-info.exp      | 66 +++++++++++++++++++
>  4 files changed, 105 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.rocm/addr-bp-gpu-no-deb-info.cpp
>  create mode 100644 gdb/testsuite/gdb.rocm/addr-bp-gpu-no-deb-info.exp
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 29b03b997c3..b1c841d4702 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7712,6 +7712,8 @@ get_sal_arch (struct symtab_and_line sal)
>      return sal.section->objfile->arch ();
>    if (sal.symtab != nullptr)
>      return sal.symtab->compunit ()->objfile ()->arch ();
> +  if (sal.objfile != nullptr)
> +    return sal.objfile->arch ();

I was wondering why this use case wouldn't be handled by the
`sal.section` case.  After all, the PC we insert the breakpoint at
should be inside an obj_section that points to the same objfile you
would end up with in the `sal.objfile` case, right?

It turns out that most of the time, `sal.section` is only set when using
overlays (using `find_pc_overlay()`).  In our case, it will always be
nullptr.  It said "most of the time" because some other spots set that
field differently, so presumably it would be set even when not using
overlays.  I find this a bit confusing, I would have expected a field
name `section` to always be set whenever there is a section matching the
location.

All of this to say: what if we made it so that `sal.section` is always
set?  I guess in this use case we would find an obj_section and pick up
the objfile, and therefore arch from there?

I imagine we could call `find_pc_section` instead of `find_pc_overlay`
to initialize that field (I'm just not sure if there would be problems
when actually using overlay debugging, like could this call find a
section that's not currently mapped).  This does a bsearch over the
program space's section list, so it should be relatively quick.  And
then, it could perhaps make it so that we could remove
`symtab_and_line::objfile` (since you could always get the objfile from
`symtab_and_line::section`)?


>  V
>    return nullptr;
>  }
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 4d8a8c1731e..ac26d4c8309 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -2188,6 +2188,13 @@ convert_address_location_to_sals (struct linespec_state *self,
>    sal.explicit_pc = 1;
>    sal.symbol = find_pc_sect_containing_function (sal.pc, sal.section);
>  
> +  for (objfile *objfile : current_program_space->objfiles ())
> +    if (is_addr_in_objfile (address, objfile))
> +      {
> +	sal.objfile = objfile;
> +	break;
> +      }

Could this use program_space::objfile_for_address?

Simon