From 396d2e56bed9cb1b90696d5a0969786144101d25 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Thu, 9 Jun 2022 11:18:51 +0100 Subject: [PATCH] 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. --- 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.30.2