[PATCH] gdb/aarch64: fix 32-bit arm compatibility

[PATCH] gdb/aarch64: fix 32-bit arm compatibility

[Andrew Burgess]

GDB's ability to run 32-bit ARM processes on an AArch64 native target
is currently broken.  The test gdb.multi/multi-arch.exp currently
fails with a timeout.

The cause of these problems is the following three functions:

  aarch64_linux_nat_target::thread_architecture
  aarch64_linux_nat_target::fetch_registers
  aarch64_linux_nat_target::store_registers

What has happened, over time, is that these functions have been
modified, forgetting that any particular thread (running on the native
target) might be an ARM thread, or might be an AArch64 thread.

The problems always start with a line similar to this:

  aarch64_gdbarch_tdep *tdep
    = (aarch64_gdbarch_tdep *) gdbarch_tdep (inf->gdbarch);

The problem with this line is that if 'inf->gdbarch' is an ARM
architecture, then gdbarch_tdep will return a pointer to an
arm_gdbarch_tdep object, not an aarch64_gdbarch_tdep object.  The
result of the above cast will, as a consequence, be undefined.

In aarch64_linux_nat_target::thread_architecture, after the undefined
cast we then proceed to make use of TDEP, like this:

  if (vq == tdep->vq)
    return inf->gdbarch;

Obviously at this point the result is undefined, but, if this check
returns false we then proceed with this code:

  struct gdbarch_info info;
  info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
  info.id = (int *) (vq == 0 ? -1 : vq);
  return gdbarch_find_by_info (info);

As a consequence we will return an AArch64 gdbarch object for our ARM
thread!  Things go downhill from there on.

There are similar problems, with similar undefined behaviour, in the
fetch_registers and store_registers functions.

The solution is to make use of a check like this:

  if (gdbarch_bfd_arch_info (inf->gdbarch)->bits_per_word == 32)

If the word size is 32 then we know we have an ARM architecture.  We
just need to make sure that we perform this check before trying to
read the tdep field.

In aarch64_linux_nat_target::thread_architecture a little reordering,
and the addition of the above check allows us to easily avoid the
undefined behaviour.

For fetch_registers and store_registers I made the decision to split
each of the functions into two new helper functions, and so
aarch64_linux_nat_target::fetch_registers now calls to either
aarch64_fetch_registers or aarch32_fetch_registers, and there's a
similar change for store_registers.

One thing I had to decide was whether to place the new aarch32_*
functions into the aarch32-linux-nat.c file.  In the end I decided to
NOT place the functions there, but instead leave them in
aarch64-linux-nat.c, my reasoning was this:

The existing functions in that file are shared from arm-linux-nat.c
and aarch64-linux-nat.c, this generic code to support 32-bit ARM
debugging from either native target.

In contrast, the two new aarch32 functions I have added _only_ make
sense when debugging on an AArch64 native target.  These function
shouldn't be called from arm-linux-nat.c at all, and so, if we places
the functions into aarch32-linux-nat.c, the functions would be built
into a 32-bit ARM GDB, but never used.

With that said, there's no technical reason why they couldn't go in
aarch32-linux-nat.c, so if that is preferred I'm happy to move them.

After this commit the gdb.multi/multi-arch.exp passes.
---
 gdb/aarch64-linux-nat.c | 114 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 100 insertions(+), 14 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index ff5c4c0a212..d58ad0143a2 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -46,6 +46,7 @@
 
 #include "gregset.h"
 #include "linux-tdep.h"
+#include "arm-tdep.h"
 
 /* Defines ps_err_e, struct ps_prochandle.  */
 #include "gdb_proc_service.h"
@@ -485,11 +486,11 @@ store_tlsregs_to_thread (struct regcache *regcache)
     perror_with_name (_("unable to store TLS register"));
 }
 
-/* Implement the "fetch_registers" target_ops method.  */
+/* The AArch64 version of the "fetch_registers" target_ops method.  Fetch
+   REGNO from the target and place the result into REGCACHE.  */
 
-void
-aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
-					   int regno)
+static void
+aarch64_fetch_registers (struct regcache *regcache, int regno)
 {
   aarch64_gdbarch_tdep *tdep
     = (aarch64_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
@@ -534,11 +535,48 @@ aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
     fetch_tlsregs_from_thread (regcache);
 }
 
-/* Implement the "store_registers" target_ops method.  */
+/* A version of the "fetch_registers" target_ops method used when running
+   32-bit ARM code on an AArch64 target.  Fetch REGNO from the target and
+   place the result into REGCACHE.  */
+
+static void
+aarch32_fetch_registers (struct regcache *regcache, int regno)
+{
+  arm_gdbarch_tdep *tdep
+    = (arm_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
+
+  if (regno == -1)
+    {
+      fetch_gregs_from_thread (regcache);
+      if (tdep->vfp_register_count > 0)
+	fetch_fpregs_from_thread (regcache);
+    }
+  else if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
+    fetch_gregs_from_thread (regcache);
+  else if (tdep->vfp_register_count > 0
+	   && regno >= ARM_D0_REGNUM
+	   && (regno < ARM_D0_REGNUM + tdep->vfp_register_count
+	       || regno == ARM_FPSCR_REGNUM))
+    fetch_fpregs_from_thread (regcache);
+}
+
+/* Implement the "fetch_registers" target_ops method.  */
 
 void
-aarch64_linux_nat_target::store_registers (struct regcache *regcache,
+aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
 					   int regno)
+{
+  if (gdbarch_bfd_arch_info (regcache->arch ())->bits_per_word == 32)
+    aarch32_fetch_registers (regcache, regno);
+  else
+    aarch64_fetch_registers (regcache, regno);
+}
+
+/* The AArch64 version of the "store_registers" target_ops method.  Copy
+   the value of register REGNO from REGCACHE into the the target.  */
+
+static void
+aarch64_store_registers (struct regcache *regcache, int regno)
 {
   aarch64_gdbarch_tdep *tdep
     = (aarch64_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
@@ -573,6 +611,43 @@ aarch64_linux_nat_target::store_registers (struct regcache *regcache,
     store_tlsregs_to_thread (regcache);
 }
 
+/* A version of the "store_registers" target_ops method used when running
+   32-bit ARM code on an AArch64 target.  Copy the value of register REGNO
+   from REGCACHE into the the target.  */
+
+static void
+aarch32_store_registers (struct regcache *regcache, int regno)
+{
+  arm_gdbarch_tdep *tdep
+    = (arm_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
+
+  if (regno == -1)
+    {
+      store_gregs_to_thread (regcache);
+      if (tdep->vfp_register_count > 0)
+	store_fpregs_to_thread (regcache);
+    }
+  else if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
+    store_gregs_to_thread (regcache);
+  else if (tdep->vfp_register_count > 0
+	   && regno >= ARM_D0_REGNUM
+	   && (regno < ARM_D0_REGNUM + tdep->vfp_register_count
+	       || regno == ARM_FPSCR_REGNUM))
+    store_fpregs_to_thread (regcache);
+}
+
+/* Implement the "store_registers" target_ops method.  */
+
+void
+aarch64_linux_nat_target::store_registers (struct regcache *regcache,
+					   int regno)
+{
+  if (gdbarch_bfd_arch_info (regcache->arch ())->bits_per_word == 32)
+    aarch32_store_registers (regcache, regno);
+  else
+    aarch64_store_registers (regcache, regno);
+}
+
 /* Fill register REGNO (if it is a general-purpose register) in
    *GREGSETPS with the value in GDB's register array.  If REGNO is -1,
    do this for all registers.  */
@@ -793,22 +868,33 @@ aarch64_linux_nat_target::can_do_single_step ()
   return 1;
 }
 
-/* Implement the "thread_architecture" target_ops method.  */
+/* Implement the "thread_architecture" target_ops method.
+
+   Returns the gdbarch for the thread identified by PTID.  If the thread in
+   question is a 32-bit ARM thread, then the architecture returned will be
+   that of the process itself.
+
+   If the thread is an AArch64 thread then we need to check the current
+   vector length; if the vector length has changed then we need to lookup a
+   new gdbarch that matches the new vector length.  */
 
 struct gdbarch *
 aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
 {
-  /* Return the gdbarch for the current thread.  If the vector length has
-     changed since the last time this was called, then do a further lookup.  */
-
-  uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
-
-  /* Find the current gdbarch the same way as process_stratum_target.  Only
-     return it if the current vector length matches the one in the tdep.  */
+  /* Find the current gdbarch the same way as process_stratum_target.  */
   inferior *inf = find_inferior_ptid (this, ptid);
   gdb_assert (inf != NULL);
+
+  /* If this is a 32-bit architecture, then this is ARM, not AArch64.
+     There's no SVE vectors here, so just return the inferior
+     architecture.  */
+  if (gdbarch_bfd_arch_info (inf->gdbarch)->bits_per_word == 32)
+    return inf->gdbarch;
+
+  /* Only return it if the current vector length matches the one in the tdep.  */
   aarch64_gdbarch_tdep *tdep
     = (aarch64_gdbarch_tdep *) gdbarch_tdep (inf->gdbarch);
+  uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
   if (vq == tdep->vq)
     return inf->gdbarch;
 
-- 
2.25.4

Re: [PATCH] gdb/aarch64: fix 32-bit arm compatibility

[Luis Machado]

On 6/9/22 13:32, Andrew Burgess via Gdb-patches wrote:
> GDB's ability to run 32-bit ARM processes on an AArch64 native target
> is currently broken.  The test gdb.multi/multi-arch.exp currently
> fails with a timeout.
> 
> The cause of these problems is the following three functions:
> 
>    aarch64_linux_nat_target::thread_architecture
>    aarch64_linux_nat_target::fetch_registers
>    aarch64_linux_nat_target::store_registers
> 
> What has happened, over time, is that these functions have been
> modified, forgetting that any particular thread (running on the native
> target) might be an ARM thread, or might be an AArch64 thread.
> 
> The problems always start with a line similar to this:
> 
>    aarch64_gdbarch_tdep *tdep
>      = (aarch64_gdbarch_tdep *) gdbarch_tdep (inf->gdbarch);
> 
> The problem with this line is that if 'inf->gdbarch' is an ARM
> architecture, then gdbarch_tdep will return a pointer to an
> arm_gdbarch_tdep object, not an aarch64_gdbarch_tdep object.  The
> result of the above cast will, as a consequence, be undefined.
> 
> In aarch64_linux_nat_target::thread_architecture, after the undefined
> cast we then proceed to make use of TDEP, like this:
> 
>    if (vq == tdep->vq)
>      return inf->gdbarch;
> 
> Obviously at this point the result is undefined, but, if this check
> returns false we then proceed with this code:
> 
>    struct gdbarch_info info;
>    info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
>    info.id = (int *) (vq == 0 ? -1 : vq);
>    return gdbarch_find_by_info (info);
> 
> As a consequence we will return an AArch64 gdbarch object for our ARM
> thread!  Things go downhill from there on.
> 
> There are similar problems, with similar undefined behaviour, in the
> fetch_registers and store_registers functions.
> 
> The solution is to make use of a check like this:
> 
>    if (gdbarch_bfd_arch_info (inf->gdbarch)->bits_per_word == 32)
> 
> If the word size is 32 then we know we have an ARM architecture.  We
> just need to make sure that we perform this check before trying to
> read the tdep field.
> 
> In aarch64_linux_nat_target::thread_architecture a little reordering,
> and the addition of the above check allows us to easily avoid the
> undefined behaviour.
> 
> For fetch_registers and store_registers I made the decision to split
> each of the functions into two new helper functions, and so
> aarch64_linux_nat_target::fetch_registers now calls to either
> aarch64_fetch_registers or aarch32_fetch_registers, and there's a
> similar change for store_registers.
> 
> One thing I had to decide was whether to place the new aarch32_*
> functions into the aarch32-linux-nat.c file.  In the end I decided to
> NOT place the functions there, but instead leave them in
> aarch64-linux-nat.c, my reasoning was this:
> 
> The existing functions in that file are shared from arm-linux-nat.c
> and aarch64-linux-nat.c, this generic code to support 32-bit ARM
> debugging from either native target.
> 
> In contrast, the two new aarch32 functions I have added _only_ make
> sense when debugging on an AArch64 native target.  These function
> shouldn't be called from arm-linux-nat.c at all, and so, if we places
> the functions into aarch32-linux-nat.c, the functions would be built
> into a 32-bit ARM GDB, but never used.
> 
> With that said, there's no technical reason why they couldn't go in
> aarch32-linux-nat.c, so if that is preferred I'm happy to move them.
> 
> After this commit the gdb.multi/multi-arch.exp passes.
> ---
>   gdb/aarch64-linux-nat.c | 114 +++++++++++++++++++++++++++++++++++-----
>   1 file changed, 100 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index ff5c4c0a212..d58ad0143a2 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -46,6 +46,7 @@
>   
>   #include "gregset.h"
>   #include "linux-tdep.h"
> +#include "arm-tdep.h"
>   
>   /* Defines ps_err_e, struct ps_prochandle.  */
>   #include "gdb_proc_service.h"
> @@ -485,11 +486,11 @@ store_tlsregs_to_thread (struct regcache *regcache)
>       perror_with_name (_("unable to store TLS register"));
>   }
>   
> -/* Implement the "fetch_registers" target_ops method.  */
> +/* The AArch64 version of the "fetch_registers" target_ops method.  Fetch
> +   REGNO from the target and place the result into REGCACHE.  */
>   
> -void
> -aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
> -					   int regno)
> +static void
> +aarch64_fetch_registers (struct regcache *regcache, int regno)
>   {
>     aarch64_gdbarch_tdep *tdep
>       = (aarch64_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
> @@ -534,11 +535,48 @@ aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
>       fetch_tlsregs_from_thread (regcache);
>   }
>   
> -/* Implement the "store_registers" target_ops method.  */
> +/* A version of the "fetch_registers" target_ops method used when running
> +   32-bit ARM code on an AArch64 target.  Fetch REGNO from the target and
> +   place the result into REGCACHE.  */
> +
> +static void
> +aarch32_fetch_registers (struct regcache *regcache, int regno)
> +{
> +  arm_gdbarch_tdep *tdep
> +    = (arm_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
> +
> +  if (regno == -1)
> +    {
> +      fetch_gregs_from_thread (regcache);
> +      if (tdep->vfp_register_count > 0)
> +	fetch_fpregs_from_thread (regcache);
> +    }
> +  else if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
> +    fetch_gregs_from_thread (regcache);
> +  else if (tdep->vfp_register_count > 0
> +	   && regno >= ARM_D0_REGNUM
> +	   && (regno < ARM_D0_REGNUM + tdep->vfp_register_count
> +	       || regno == ARM_FPSCR_REGNUM))
> +    fetch_fpregs_from_thread (regcache);
> +}
> +
> +/* Implement the "fetch_registers" target_ops method.  */
>   
>   void
> -aarch64_linux_nat_target::store_registers (struct regcache *regcache,
> +aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
>   					   int regno)
> +{
> +  if (gdbarch_bfd_arch_info (regcache->arch ())->bits_per_word == 32)
> +    aarch32_fetch_registers (regcache, regno);
> +  else
> +    aarch64_fetch_registers (regcache, regno);
> +}
> +
> +/* The AArch64 version of the "store_registers" target_ops method.  Copy
> +   the value of register REGNO from REGCACHE into the the target.  */
> +
> +static void
> +aarch64_store_registers (struct regcache *regcache, int regno)
>   {
>     aarch64_gdbarch_tdep *tdep
>       = (aarch64_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
> @@ -573,6 +611,43 @@ aarch64_linux_nat_target::store_registers (struct regcache *regcache,
>       store_tlsregs_to_thread (regcache);
>   }
>   
> +/* A version of the "store_registers" target_ops method used when running
> +   32-bit ARM code on an AArch64 target.  Copy the value of register REGNO
> +   from REGCACHE into the the target.  */
> +
> +static void
> +aarch32_store_registers (struct regcache *regcache, int regno)
> +{
> +  arm_gdbarch_tdep *tdep
> +    = (arm_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
> +
> +  if (regno == -1)
> +    {
> +      store_gregs_to_thread (regcache);
> +      if (tdep->vfp_register_count > 0)
> +	store_fpregs_to_thread (regcache);
> +    }
> +  else if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
> +    store_gregs_to_thread (regcache);
> +  else if (tdep->vfp_register_count > 0
> +	   && regno >= ARM_D0_REGNUM
> +	   && (regno < ARM_D0_REGNUM + tdep->vfp_register_count
> +	       || regno == ARM_FPSCR_REGNUM))
> +    store_fpregs_to_thread (regcache);
> +}
> +
> +/* Implement the "store_registers" target_ops method.  */
> +
> +void
> +aarch64_linux_nat_target::store_registers (struct regcache *regcache,
> +					   int regno)
> +{
> +  if (gdbarch_bfd_arch_info (regcache->arch ())->bits_per_word == 32)
> +    aarch32_store_registers (regcache, regno);
> +  else
> +    aarch64_store_registers (regcache, regno);
> +}
> +
>   /* Fill register REGNO (if it is a general-purpose register) in
>      *GREGSETPS with the value in GDB's register array.  If REGNO is -1,
>      do this for all registers.  */
> @@ -793,22 +868,33 @@ aarch64_linux_nat_target::can_do_single_step ()
>     return 1;
>   }
>   
> -/* Implement the "thread_architecture" target_ops method.  */
> +/* Implement the "thread_architecture" target_ops method.
> +
> +   Returns the gdbarch for the thread identified by PTID.  If the thread in
> +   question is a 32-bit ARM thread, then the architecture returned will be
> +   that of the process itself.
> +
> +   If the thread is an AArch64 thread then we need to check the current
> +   vector length; if the vector length has changed then we need to lookup a
> +   new gdbarch that matches the new vector length.  */
>   
>   struct gdbarch *
>   aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>   {
> -  /* Return the gdbarch for the current thread.  If the vector length has
> -     changed since the last time this was called, then do a further lookup.  */
> -
> -  uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
> -
> -  /* Find the current gdbarch the same way as process_stratum_target.  Only
> -     return it if the current vector length matches the one in the tdep.  */
> +  /* Find the current gdbarch the same way as process_stratum_target.  */
>     inferior *inf = find_inferior_ptid (this, ptid);
>     gdb_assert (inf != NULL);
> +
> +  /* If this is a 32-bit architecture, then this is ARM, not AArch64.
> +     There's no SVE vectors here, so just return the inferior
> +     architecture.  */
> +  if (gdbarch_bfd_arch_info (inf->gdbarch)->bits_per_word == 32)
> +    return inf->gdbarch;
> +
> +  /* Only return it if the current vector length matches the one in the tdep.  */
>     aarch64_gdbarch_tdep *tdep
>       = (aarch64_gdbarch_tdep *) gdbarch_tdep (inf->gdbarch);
> +  uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
>     if (vq == tdep->vq)
>       return inf->gdbarch;
>   

LGTM. Thanks for the patch!