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

[Sven Schnelle <svens@stackframe.org>]

Simon Marchi <simark@simark.ca> writes:

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

Ah, got it. Maybe we should default to 32 bit (as userspace is very
likely to be 32 bit, only the kernel might be 64 bit right now), and
print a warning in that case?