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

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

[Sven Schnelle]

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
@@ -33,6 +33,8 @@
 #include "trad-frame.h"
 #include "frame-unwind.h"
 #include "frame-base.h"
+#include "remote.h"
+#include "target-descriptions.h"
 
 #include "gdbcore.h"
 #include "cli/cli-cmds.h"
@@ -43,6 +45,14 @@
 
 static bool hppa_debug = false;
 
+/* Properties (for struct target_desc) describing the g/G packet
+   layout.  */
+#define PROPERTY_GP32 "internal: transfers-32bit-registers"
+#define PROPERTY_GP64 "internal: transfers-64bit-registers"
+
+static target_desc_up hppa_tdesc32;
+static target_desc_up hppa_tdesc64;
+
 /* Some local constants.  */
 static const int hppa32_num_regs = 128;
 static const int hppa64_num_regs = 96;
@@ -2978,6 +2988,17 @@ hppa_skip_trampoline_code (const frame_info_ptr &frame, CORE_ADDR pc)
 
    -- chastain 2003-12-18  */
 
+static void
+hppa_register_g_packet_guesses (struct gdbarch *gdbarch)
+{
+  /* If the size matches the set of 32-bit or 64-bit integer registers,
+     assume that's what we've got.  */
+  register_remote_g_packet_guess (gdbarch, hppa32_num_regs * 4, hppa_tdesc32.get());
+  register_remote_g_packet_guess (gdbarch, hppa64_num_regs * 8, hppa_tdesc64.get());
+
+  /* Otherwise we don't have a useful guess.  */
+}
+
 static struct gdbarch *
 hppa_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 {
@@ -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;
+    }
+  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;
 
+  hppa_register_g_packet_guesses (gdbarch);
+
   tdep->find_global_pointer = hppa_find_global_pointer;
 
   /* Some parts of the gdbarch vector depend on whether we are running
@@ -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."),
-- 
2.51.0

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

[Sven Schnelle]

Sven Schnelle <svens@stackframe.org> writes:

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

Gentle ping?

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

[Simon Marchi]

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
> @@ -33,6 +33,8 @@
>  #include "trad-frame.h"
>  #include "frame-unwind.h"
>  #include "frame-base.h"
> +#include "remote.h"
> +#include "target-descriptions.h"
>  
>  #include "gdbcore.h"
>  #include "cli/cli-cmds.h"
> @@ -43,6 +45,14 @@
>  
>  static bool hppa_debug = false;
>  
> +/* Properties (for struct target_desc) describing the g/G packet
> +   layout.  */
> +#define PROPERTY_GP32 "internal: transfers-32bit-registers"
> +#define PROPERTY_GP64 "internal: transfers-64bit-registers"
> +
> +static target_desc_up hppa_tdesc32;
> +static target_desc_up hppa_tdesc64;
> +
>  /* Some local constants.  */
>  static const int hppa32_num_regs = 128;
>  static const int hppa64_num_regs = 96;
> @@ -2978,6 +2988,17 @@ hppa_skip_trampoline_code (const frame_info_ptr &frame, CORE_ADDR pc)
>  
>     -- chastain 2003-12-18  */
>  
> +static void
> +hppa_register_g_packet_guesses (struct gdbarch *gdbarch)
> +{
> +  /* If the size matches the set of 32-bit or 64-bit integer registers,
> +     assume that's what we've got.  */
> +  register_remote_g_packet_guess (gdbarch, hppa32_num_regs * 4, hppa_tdesc32.get());
> +  register_remote_g_packet_guess (gdbarch, hppa64_num_regs * 8, hppa_tdesc64.get());

Missing space before parenthesis, and line too long.  This would work:

  register_remote_g_packet_guess (gdbarch, hppa32_num_regs * 4,
				  hppa_tdesc32.get ());
  register_remote_g_packet_guess (gdbarch, hppa64_num_regs * 8,
				  hppa_tdesc64.get ());

> +
> +  /* Otherwise we don't have a useful guess.  */
> +}
> +
>  static struct gdbarch *
>  hppa_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {
> @@ -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?

> +    }
> +  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;
> +    }

Remove curly braces:

  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

>    else
>      tdep->bytes_per_address = 4;
>  
> +  hppa_register_g_packet_guesses (gdbarch);
> +
>    tdep->find_global_pointer = hppa_find_global_pointer;
>  
>    /* Some parts of the gdbarch vector depend on whether we are running
> @@ -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?

Otherwise code-wise the patch seems ok.

Simon

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

[Simon Marchi]

On 11/10/25 11:52 AM, Simon Marchi wrote:
> 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"?

Actually, I checked it myself, it's not too difficult.  It seems like
MIPS does something like that, although it also seems to support "real"
target descriptions, given it does:

  /* Check any target description for validity.  */
  if (tdesc_has_registers (info.target_desc))
    ...

I also checked QEMU's repo [1], it doesn't seem to have a target
description for the hppa architecture.

[1] https://github.com/qemu/qemu/tree/593aee5df98b4a862ff8841a57ea3dbf22131a5f/gdb-xml

Simon

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

[Sven Schnelle]

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.

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

[Simon Marchi]

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

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

[Sven Schnelle]

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?

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

[Simon Marchi]

On 11/10/25 3:10 PM, Sven Schnelle wrote:
>> 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?

I guess we could, then there's a non-0% chance that GDB will be right
and be useful to the user.  If we error out, there is a 0% change that
GDB will be useful to the user.

Simon