RE: [PATCHv5 11/11] gdb/gdbserver: share x86/linux tdesc caching

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Freitag, 26. April 2024 17:02
> To: gdb-patches@sourceware.org
> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
> Subject: [PATCHv5 11/11] gdb/gdbserver: share x86/linux tdesc caching
> 
> This commit builds on the previous series of commits to share the
> target description caching code between GDB and gdbserver for
> x86/Linux targets.
> 
> The objective of this commit is to move the four functions (2 each of)
> i386_linux_read_description and amd64_linux_read_description into the
> gdb/arch/ directory and combine them so we have just a single copy of
> each.  Then GDB, gdbserver, and the in-process-agent (IPA) will link
> against these shared functions.
> 
> It is worth reading the description of the previous commit(s) to see
> why this merging is not as simple as it seems, but in summary,
> gdbserver used to generate a different set of possible target
> descriptions than GDB.  The previous commit(s) fixed this, so now it
> should be simpler to share the functions.
> 
> One curiosity with this patch is the function
> x86_linux_post_init_tdesc.  On the gdbserver side the two functions
> amd64_linux_read_description and i386_linux_read_description have some
> functionality that is not present on the GDB side, that is some
> additional configuration that is performed as each target description
> is created to setup the expedited registers.
> 
> To support this I've added the function x86_linux_post_init_tdesc.
> This function is called from the two *_linux_read_description
> functions, but is implemented separately for GDB and gdbserver.
> 
> An alternative approach that avoids adding x86_linux_post_init_tdesc
> would be to have x86_linux_tdesc_for_tid return a non-const target
> description, in x86_target::low_arch_setup we could then inspect the
> target description to figure out if it is 64-bit or not, and modify
> the target description as needed.  In the end I figured that adding
> the x86_linux_post_init_tdesc function was good enough, so went with
> that solution.
> 
> The contents of gdbserver/linux-x86-low.cc have moved to
> gdb/arch/x86-linux-tdesc-features.c, and gdbserver/linux-x86-tdesc.h
> has moved to gdb/arch/x86-linux-tdesc-features.h, this change leads to
> some updates in the #includes in the gdbserver/ directory.
> 
> For testing I've done the following:
> 
>   - Built on x86-64 GNU/Linux for all targets, and just for the native
>     target,
> 
>   - Build on i386 GNU/Linux for all targets, and just for the native
>     target,
> 
>   - Build on a 64-bit, non-x86 GNU/Linux for all targets, just for the
>     native target, and for targets x86_64-*-linux and i386-*-linux.
> 
> This did flush out a bunch of build issues where I'd failed to add a
> required file in a configure.* file, but I think everything should now
> be good.
> ---
>  gdb/Makefile.in                               |   5 +
>  gdb/amd64-linux-tdep.c                        |  31 --
>  gdb/arch/amd64-linux-tdesc.c                  |  61 ++++
>  gdb/arch/i386-linux-tdesc.c                   |  51 +++
>  gdb/arch/x86-linux-tdesc-features.c           | 247 +++++++++++++++
>  .../arch/x86-linux-tdesc-features.h           |  50 +--
>  gdb/arch/x86-linux-tdesc.h                    |  37 +++
>  gdb/configure.nat                             |   6 +-
>  gdb/configure.tgt                             |  11 +-
>  gdb/i386-linux-tdep.c                         |  26 +-
>  gdbserver/configure.srv                       |   9 +
>  gdbserver/linux-amd64-ipa.cc                  |   2 +-
>  gdbserver/linux-i386-ipa.cc                   |   2 +-
>  gdbserver/linux-x86-low.cc                    |   2 +-
>  gdbserver/linux-x86-tdesc.cc                  | 292 +-----------------
>  15 files changed, 469 insertions(+), 363 deletions(-)
>  create mode 100644 gdb/arch/amd64-linux-tdesc.c
>  create mode 100644 gdb/arch/i386-linux-tdesc.c
>  create mode 100644 gdb/arch/x86-linux-tdesc-features.c
>  rename gdbserver/linux-x86-tdesc.h => gdb/arch/x86-linux-tdesc-features.h
> (52%)
>  create mode 100644 gdb/arch/x86-linux-tdesc.h
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index a24b2232daa..1e49ae396f4 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -748,6 +748,7 @@ ALL_64_TARGET_OBS = \
>  	arch/aarch64-insn.o \
>  	arch/aarch64-mte-linux.o \
>  	arch/aarch64-scalable-linux.o \
> +	arch/amd64-linux-tdesc.o \
>  	arch/amd64.o \
>  	arch/riscv.o \
>  	bpf-tdep.o \
> @@ -788,9 +789,11 @@ ALL_TARGET_OBS = \
>  	arch/arm.o \
>  	arch/arm-get-next-pcs.o \
>  	arch/arm-linux.o \
> +	arch/i386-linux-tdesc.o \
>  	arch/i386.o \
>  	arch/loongarch.o \
>  	arch/ppc-linux-common.o \
> +	arch/x86-linux-tdesc-features.o \
>  	arm-bsd-tdep.o \
>  	arm-fbsd-tdep.o \
>  	arm-linux-tdep.o \
> @@ -1558,6 +1561,8 @@ HFILES_NO_SRCDIR = \
>  	arch/ppc-linux-common.h \
>  	arch/ppc-linux-tdesc.h \
>  	arch/riscv.h \
> +	arch/x86-linux-tdesc-features.h \
> +	arch/x86-linux-tdesc.h \
>  	cli/cli-cmds.h \
>  	cli/cli-decode.h \
>  	cli/cli-script.h \
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index bcb9868e79e..c707745cd9a 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1577,37 +1577,6 @@ amd64_linux_record_signal (struct gdbarch *gdbarch,
>    return 0;
>  }
> 
> -const target_desc *
> -amd64_linux_read_description (uint64_t xcr0_features_bit, bool is_x32)
> -{
> -  static target_desc *amd64_linux_tdescs \
> -    [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/] = {};
> -  static target_desc *x32_linux_tdescs \
> -    [2/*AVX*/][2/*AVX512*/][2/*PKRU*/] = {};
> -
> -  target_desc **tdesc;
> -
> -  if (is_x32)
> -    {
> -      tdesc = &x32_linux_tdescs[(xcr0_features_bit & X86_XSTATE_AVX) ? 1 : 0 ]
> -	[(xcr0_features_bit & X86_XSTATE_AVX512) ? 1 : 0]
> -	[(xcr0_features_bit & X86_XSTATE_PKRU) ? 1 : 0];
> -    }
> -  else
> -    {
> -      tdesc = &amd64_linux_tdescs[(xcr0_features_bit & X86_XSTATE_AVX) ? 1 : 0]
> -	[(xcr0_features_bit & X86_XSTATE_MPX) ? 1 : 0]
> -	[(xcr0_features_bit & X86_XSTATE_AVX512) ? 1 : 0]
> -	[(xcr0_features_bit & X86_XSTATE_PKRU) ? 1 : 0];
> -    }
> -
> -  if (*tdesc == NULL)
> -    *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32,
> -					      true, true);
> -
> -  return *tdesc;
> -}
> -
>  /* Get Linux/x86 target description from core dump.  */
> 
>  static const struct target_desc *
> diff --git a/gdb/arch/amd64-linux-tdesc.c b/gdb/arch/amd64-linux-tdesc.c
> new file mode 100644
> index 00000000000..63b5ddfcece
> --- /dev/null
> +++ b/gdb/arch/amd64-linux-tdesc.c
> @@ -0,0 +1,61 @@
> +/* Target description related code for GNU/Linux x86-64.
> +
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "arch/x86-linux-tdesc.h"
> +#include "arch/amd64-linux-tdesc.h"
> +#include "arch/amd64.h"
> +#include "arch/x86-linux-tdesc-features.h"
> +
> +
> +/* See arch/amd64-linux-tdesc.h.  */
> +
> +const struct target_desc *
> +amd64_linux_read_description (uint64_t xcr0, bool is_x32)
> +{
> +  /* The type used for the amd64 and x32 target description caches.  */
> +  using tdesc_cache_type = std::unordered_map<uint64_t, const
> target_desc_up>;
> +
> +  /* Caches for the previously seen amd64 and x32 target descriptions,
> +     indexed by the xcr0 value that created the target description.  These
> +     need to be static within this function to ensure they are initialised
> +     before first use.  */
> +  static tdesc_cache_type amd64_tdesc_cache, x32_tdesc_cache;
> +

Sorry for this long comment, it isn't strictly related to your series, as you don't
change the status quo. But since you are changing the cache, I find it relevant.

I wonder if caching of target descriptions based on xcr0 alone is really
good enough. It isn't always the only thing used for determining registers.
I realize you don't change that fact, though with the map we would need to
hash xcr0 with the other factors for the key. Arguably that could still be
viewed as better than the current way.

Right now there is already the segments and "orig_rax" register for amd64.
Though I don't know if IPA really supports them and I am not super familiar with
their details. It seems like we just add them unconditionally or based on whether
or not we are on Linux. I am not really sure that means we don't care about them
for our cache (especially the Linux part), but it seems that is the current state.

But in the near future there will be the shadow stack pointer register for CET,
which we can only read with a separate ptrace call.
See this patch, which we want to post soon:
https://github.com/intel/gdb/commit/afff428f38a1f03ac0127797deda524d81ec156e

I must admit I never really fully understood why this caching is done.
Is it for a single GDB session connecting to different gdbserver sessions one
after the other? Or for inferior re-starts?
Or for runtime target description updates similar to SVE on aarch64?
(Which we will also add at some point for AMX on x86.)
Or a mix of all?

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928