Re: [PATCH v2] gdb/hppa: guess g packet size

[Simon Marchi <simark@simark.ca>]

On 11/4/25 1:30 AM, Sven Schnelle wrote:
> With qemu supporting 64 bit now, add some code to determine the
> register size of a hppa remote target.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
> 
> Changes in v2:
> 
> - adjust coding style
> - use target_desc_up, make it static
> - use nullptr instead of NULL
> 
>  gdb/hppa-tdep.c | 51 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
> index 96cb797c023..bd250408951 100644
> --- a/gdb/hppa-tdep.c
> +++ b/gdb/hppa-tdep.c
> @@ -33,6 +33,8 @@
>  #include "trad-frame.h"
>  #include "frame-unwind.h"
>  #include "frame-base.h"
> +#include "remote.h"
> +#include "target-descriptions.h"
>  
>  #include "gdbcore.h"
>  #include "cli/cli-cmds.h"
> @@ -43,6 +45,14 @@
>  
>  static bool hppa_debug = false;
>  
> +/* Properties (for struct target_desc) describing the g/G packet
> +   layout.  */
> +#define PROPERTY_GP32 "internal: transfers-32bit-registers"
> +#define PROPERTY_GP64 "internal: transfers-64bit-registers"
> +
> +static target_desc_up hppa_tdesc32;
> +static target_desc_up hppa_tdesc64;
> +
>  /* Some local constants.  */
>  static const int hppa32_num_regs = 128;
>  static const int hppa64_num_regs = 96;
> @@ -2978,6 +2988,17 @@ hppa_skip_trampoline_code (const frame_info_ptr &frame, CORE_ADDR pc)
>  
>     -- chastain 2003-12-18  */
>  
> +static void
> +hppa_register_g_packet_guesses (struct gdbarch *gdbarch)
> +{
> +  /* If the size matches the set of 32-bit or 64-bit integer registers,
> +     assume that's what we've got.  */
> +  register_remote_g_packet_guess (gdbarch, hppa32_num_regs * 4, hppa_tdesc32.get());
> +  register_remote_g_packet_guess (gdbarch, hppa64_num_regs * 8, hppa_tdesc64.get());

Missing space before parenthesis, and line too long.  This would work:

  register_remote_g_packet_guess (gdbarch, hppa32_num_regs * 4,
				  hppa_tdesc32.get ());
  register_remote_g_packet_guess (gdbarch, hppa64_num_regs * 8,
				  hppa_tdesc64.get ());

> +
> +  /* Otherwise we don't have a useful guess.  */
> +}
> +
>  static struct gdbarch *
>  hppa_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {
> @@ -2991,15 +3012,28 @@ hppa_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>      = gdbarch_alloc (&info, gdbarch_tdep_up (new hppa_gdbarch_tdep));
>    hppa_gdbarch_tdep *tdep = gdbarch_tdep<hppa_gdbarch_tdep> (gdbarch);
>  
> -  /* Determine from the bfd_arch_info structure if we are dealing with
> -     a 32 or 64 bits architecture.  If the bfd_arch_info is not available,
> -     then default to a 32bit machine.  */
> -  if (info.bfd_arch_info != NULL)
> -    tdep->bytes_per_address =
> -      info.bfd_arch_info->bits_per_address / info.bfd_arch_info->bits_per_byte;
> +  /* Determine from the target description if we are dealing with
> +     a 32 or 64 bits architecture. If the target description is not
> +     available, then check whether bfd_arch_info could be used.
> +     Otherwise default to a 32bit machine.
> +  */
> +  if (info.target_desc != nullptr)
> +    {
> +      if (tdesc_property (info.target_desc, PROPERTY_GP64) != nullptr)
> +	tdep->bytes_per_address = 8;
> +      else if (tdesc_property (info.target_desc, PROPERTY_GP32) != nullptr)
> +	tdep->bytes_per_address = 4;

What happens in the (not shown) "else" branch here?  It seems like
bytes_per_address won't be set and we'll hit the internal error below.
Should we error out?

> +    }
> +  else if (info.bfd_arch_info != nullptr)
> +    {
> +      tdep->bytes_per_address =
> +	info.bfd_arch_info->bits_per_address / info.bfd_arch_info->bits_per_byte;
> +    }

Remove curly braces:

  else if (info.bfd_arch_info != nullptr)
    tdep->bytes_per_address =
      info.bfd_arch_info->bits_per_address / info.bfd_arch_info->bits_per_byte;
  else

>    else
>      tdep->bytes_per_address = 4;
>  
> +  hppa_register_g_packet_guesses (gdbarch);
> +
>    tdep->find_global_pointer = hppa_find_global_pointer;
>  
>    /* Some parts of the gdbarch vector depend on whether we are running
> @@ -3122,6 +3156,11 @@ hppa_dump_tdep (struct gdbarch *gdbarch, struct ui_file *file)
>  INIT_GDB_FILE (hppa_tdep)
>  {
>    gdbarch_register (bfd_arch_hppa, hppa_gdbarch_init, hppa_dump_tdep);
> +  hppa_tdesc32 = allocate_target_description ();
> +  set_tdesc_property (hppa_tdesc32.get(), PROPERTY_GP32, "");
> +
> +  hppa_tdesc64 = allocate_target_description ();
> +  set_tdesc_property (hppa_tdesc64.get(), PROPERTY_GP64, "");
>  
>    add_cmd ("unwind", class_maintenance, unwind_command,
>  	   _("Print unwind table entry at given address."),

I'm not super familiar with remote target descriptions, here's my
understanding of what is happening, please let me know if this is
correct.  The target descriptions you create are never actually used are
target descriptions, but are just some "flags" to indicate whether the g
packet size guess resulted in 32 or 64.  It seems a bit silly / strange
to use a target description this way, but perhaps there's no better way
with what we currently have.  Are there other arches in GDB that work
this way, that I could reference as "prior art"?

I guess I'm just slightly worried that something that uses this
gdbarch_info with a non-nullptr target_desc will see "oh, there a
target_desc, let me use it".  Then the results will be bogus because
it's not an actual target description.  But it's perhaps fine in
practice, and it's still a step forward.

If the above is correct, can you add some comment to indicate that the
target descriptions are not actually used to describe target registers,
and what its real purpose is?

Otherwise code-wise the patch seems ok.

Simon