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

Simon Marchi
2025-11-10T17:45:58Z

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