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

[Sven Schnelle <svens@stackframe.org>]

Simon Marchi <simark@simark.ca> writes:

> 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
>> @@ -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?

I'm afraid I didn't get the question, because:

 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;
    }
  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
    tdep->bytes_per_address = 4;

So tdep->bytes_per_address should be set.

>> @@ -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?

I think I stolen the code from mips a year ago because 
register_remote_g_packet_guess() takes a target description. If it's a
bit idea to do that, I need to find another way. But i don't know the
gdb code well enough, so I can't say.