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

[PATCH RESEND] 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>
---
 gdb/hppa-tdep.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
index 96cb797c023..ca59fc63d1d 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"
+
+struct target_desc *hppa_tdesc32;
+struct target_desc *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);
+  register_remote_g_packet_guess (gdbarch, hppa64_num_regs * 8, hppa_tdesc64);
+
+  /* Otherwise we don't have a useful guess.  */
+}
+
 static struct gdbarch *
 hppa_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 {
@@ -2991,14 +3012,24 @@ 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)
+  /* 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 != NULL) {
+    if (tdesc_property (info.target_desc, PROPERTY_GP64) != NULL)
+      tdep->bytes_per_address = 8;
+    else if (tdesc_property (info.target_desc, PROPERTY_GP32) != NULL)
+      tdep->bytes_per_address = 4;
+  } else if (info.bfd_arch_info != NULL) {
     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;
 
@@ -3122,6 +3153,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 ().release ();
+  set_tdesc_property (hppa_tdesc32, PROPERTY_GP32, "");
+
+  hppa_tdesc64 = allocate_target_description ().release ();
+  set_tdesc_property (hppa_tdesc64, PROPERTY_GP64, "");
 
   add_cmd ("unwind", class_maintenance, unwind_command,
 	   _("Print unwind table entry at given address."),
-- 
2.51.0

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

[Helge Deller]

On 11/1/25 09:05, 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>

Tested-by: Helge Deller <deller@gmx.de>

Helge


> ---
>   gdb/hppa-tdep.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
> index 96cb797c023..ca59fc63d1d 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"
> +
> +struct target_desc *hppa_tdesc32;
> +struct target_desc *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);
> +  register_remote_g_packet_guess (gdbarch, hppa64_num_regs * 8, hppa_tdesc64);
> +
> +  /* Otherwise we don't have a useful guess.  */
> +}
> +
>   static struct gdbarch *
>   hppa_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>   {
> @@ -2991,14 +3012,24 @@ 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)
> +  /* 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 != NULL) {
> +    if (tdesc_property (info.target_desc, PROPERTY_GP64) != NULL)
> +      tdep->bytes_per_address = 8;
> +    else if (tdesc_property (info.target_desc, PROPERTY_GP32) != NULL)
> +      tdep->bytes_per_address = 4;
> +  } else if (info.bfd_arch_info != NULL) {
>       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;
>   
> @@ -3122,6 +3153,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 ().release ();
> +  set_tdesc_property (hppa_tdesc32, PROPERTY_GP32, "");
> +
> +  hppa_tdesc64 = allocate_target_description ().release ();
> +  set_tdesc_property (hppa_tdesc64, PROPERTY_GP64, "");
>   
>     add_cmd ("unwind", class_maintenance, unwind_command,
>   	   _("Print unwind table entry at given address."),

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

[Tom Tromey]

>>>>> "Sven" == Sven Schnelle <svens@stackframe.org> writes:

Sven> With qemu supporting 64 bit now, add some code to determine the
Sven> register size of a hppa remote target.

Thanks for the patch.

Sven> +struct target_desc *hppa_tdesc32;
Sven> +struct target_desc *hppa_tdesc64;

These should be 'static'.  They can also just be objects, not pointers,
I think.

Sven> +  /* Determine from the target description if we are dealing with
Sven> +     a 32 or 64 bits architecture. If the target description is not
Sven> +     available, then check whether bfd_arch_info could be used.
Sven> +     Otherwise default to a 32bit machine.
Sven> +  */
Sven> +  if (info.target_desc != NULL) {
Sven> +    if (tdesc_property (info.target_desc, PROPERTY_GP64) != NULL)
Sven> +      tdep->bytes_per_address = 8;
Sven> +    else if (tdesc_property (info.target_desc, PROPERTY_GP32) != NULL)
Sven> +      tdep->bytes_per_address = 4;
Sven> +  } else if (info.bfd_arch_info != NULL) {

There's a few coding style issues here.  Use 'nullptr' not 'NULL'.
And the braces are in the wrong spots -- see other code in gdb or the
GNU coding standards for where to put them.

Sven> +  } else {
Sven> +     tdep->bytes_per_address = 4;
Sven> +  }

Also single statements like this don't need braces, there were a couple
of instances of this.

Sven> +  hppa_tdesc32 = allocate_target_description ().release ();
Sven> +  set_tdesc_property (hppa_tdesc32, PROPERTY_GP32, "");
Sven> +
Sven> +  hppa_tdesc64 = allocate_target_description ().release ();
Sven> +  set_tdesc_property (hppa_tdesc64, PROPERTY_GP64, "");

The reason I suggested using objects and not pointers is that, although
we do have a number of calls to .release() in gdb, we somewhat try to
avoid them.

thanks,
Tom

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

[Sven Schnelle]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Sven" == Sven Schnelle <svens@stackframe.org> writes:
>
> Sven> With qemu supporting 64 bit now, add some code to determine the
> Sven> register size of a hppa remote target.
>
> Thanks for the patch.
>
> Sven> +struct target_desc *hppa_tdesc32;
> Sven> +struct target_desc *hppa_tdesc64;
>
> These should be 'static'.  They can also just be objects, not pointers,
> I think.

> Sven> +  hppa_tdesc32 = allocate_target_description ().release ();
> Sven> +  set_tdesc_property (hppa_tdesc32, PROPERTY_GP32, "");
> Sven> +
> Sven> +  hppa_tdesc64 = allocate_target_description ().release ();
> Sven> +  set_tdesc_property (hppa_tdesc64, PROPERTY_GP64, "");
>
> The reason I suggested using objects and not pointers is that, although
> we do have a number of calls to .release() in gdb, we somewhat try to
> avoid them.

I might of course be wrong because i don't know the code well, but
struct target_desc is declared in target-description.c and in the
tdep file it's only used as opaque type. So it can't be used as object
because the size isn't known. Is this correct, or am i misreading the
code?

Thanks
Sven

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

[Simon Marchi]

On 2025-11-03 16:39, Sven Schnelle wrote:
> Tom Tromey <tom@tromey.com> writes:
> 
>>>>>>> "Sven" == Sven Schnelle <svens@stackframe.org> writes:
>>
>> Sven> With qemu supporting 64 bit now, add some code to determine the
>> Sven> register size of a hppa remote target.
>>
>> Thanks for the patch.
>>
>> Sven> +struct target_desc *hppa_tdesc32;
>> Sven> +struct target_desc *hppa_tdesc64;
>>
>> These should be 'static'.  They can also just be objects, not pointers,
>> I think.
> 
>> Sven> +  hppa_tdesc32 = allocate_target_description ().release ();
>> Sven> +  set_tdesc_property (hppa_tdesc32, PROPERTY_GP32, "");
>> Sven> +
>> Sven> +  hppa_tdesc64 = allocate_target_description ().release ();
>> Sven> +  set_tdesc_property (hppa_tdesc64, PROPERTY_GP64, "");
>>
>> The reason I suggested using objects and not pointers is that, although
>> we do have a number of calls to .release() in gdb, we somewhat try to
>> avoid them.
> 
> I might of course be wrong because i don't know the code well, but
> struct target_desc is declared in target-description.c and in the
> tdep file it's only used as opaque type. So it can't be used as object
> because the size isn't known. Is this correct, or am i misreading the
> code?

It seems you're right.  My suggestion would perhaps be to make the
global (well, static) objects of type `target_desc_up`, you won't need
the release then.

Simon

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

[Sven Schnelle]

Simon Marchi <simark@simark.ca> writes:

> On 2025-11-03 16:39, Sven Schnelle wrote:
>> Tom Tromey <tom@tromey.com> writes:
>> 
>>>>>>>> "Sven" == Sven Schnelle <svens@stackframe.org> writes:
>>>
>>> Sven> With qemu supporting 64 bit now, add some code to determine the
>>> Sven> register size of a hppa remote target.
>>>
>>> Thanks for the patch.
>>>
>>> Sven> +struct target_desc *hppa_tdesc32;
>>> Sven> +struct target_desc *hppa_tdesc64;
>>>
>>> These should be 'static'.  They can also just be objects, not pointers,
>>> I think.
>> 
>>> Sven> +  hppa_tdesc32 = allocate_target_description ().release ();
>>> Sven> +  set_tdesc_property (hppa_tdesc32, PROPERTY_GP32, "");
>>> Sven> +
>>> Sven> +  hppa_tdesc64 = allocate_target_description ().release ();
>>> Sven> +  set_tdesc_property (hppa_tdesc64, PROPERTY_GP64, "");
>>>
>>> The reason I suggested using objects and not pointers is that, although
>>> we do have a number of calls to .release() in gdb, we somewhat try to
>>> avoid them.
>> 
>> I might of course be wrong because i don't know the code well, but
>> struct target_desc is declared in target-description.c and in the
>> tdep file it's only used as opaque type. So it can't be used as object
>> because the size isn't known. Is this correct, or am i misreading the
>> code?
>
> It seems you're right.  My suggestion would perhaps be to make the
> global (well, static) objects of type `target_desc_up`, you won't need
> the release then.

Thanks. I just sent a v2.