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.