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

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

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=396d2e56bed9cb1b90696d5a0969786144101d25

commit 396d2e56bed9cb1b90696d5a0969786144101d25
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Jun 9 11:18:51 2022 +0100

    gdb/aarch64: fix 32-bit arm compatibility
    
    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.

Diff:
---
 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;