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

[Simon Marchi <simark@simark.ca>]

On 11/10/25 12:21 PM, Sven Schnelle wrote:
> 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.

I think we should do something where I wrote "What then?" below:

  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
         // What then?
     }
   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;

Could we reach that place if e.g. qemu started to return a target
description for the hppa architecture?  I suppose we can error out
saying something like "that target returned a target description but
this GDB doesn't support it for the HP-PA architecture".

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

It is a bit odd but I don't see a better way.  Well the better ways
would be:

1. Write some target descriptions (store them in gdb/features).  Select
   the right one using that "remote g packet guess" mechanism.  That
   would replace the manual implementations of gdbarch functions like
   gdbarch_register_name and gdbarch_register_type.
2. Have qemu return a target description for the hppa architecture.

To be clear, I don't expect you to do this (well, you can if you want
to), it would be another project.

Simon