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

Sven Schnelle
2025-11-10T20:12:07Z

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?