gdb/aarch64: fix 32-bit arm compatibility
authorAndrew Burgess <aburgess@redhat.com>
Thu, 9 Jun 2022 10:18:51 +0000 (11:18 +0100)
committerAndrew Burgess <aburgess@redhat.com>
Thu, 9 Jun 2022 17:47:08 +0000 (18:47 +0100)
commit396d2e56bed9cb1b90696d5a0969786144101d25
treee8b5382d21bb9361b3e2475a5232a5c7301ea7e6
parent2d9cf99d9a6c701de912d3e95ea3ffa134af4c62
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