[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
  

> -----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
  

"Willgerodt, Felix" <felix.willgerodt@intel.com> writes:

>> -----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?

You are 100% correct.  And I agree with everything you've said.

I also spotted this issue and had a choice to make.

I could:

  (a) fix the caching mechanism first, which would require duplicating
  the effort in GDB and gdbserver, then

  (b) merge the GDB and gdbserver code.

Or I could:

  (a) merge the GDB and gdbserver code, then

  (b) fix the caching mechanism.

As you can see, I opted to merge the code first, but this was already 11
patches.  Which is going to be at least 12 in v6.

Realistically, if I tried to "fix" caching at the same time then either
each patch is going to get much bigger (and break the one change per
patch rule) or the series is going to start getting really long.

I've stopped where I have to try and keep the number of patches down; I
don't want to overload reviewers.  Plus, if I write too much on top of
earlier patches and get asked to rewrite things the rewrites become more
work.

My plan after this series is merged was to unify all of the x86 tdesc
caching, not just for Linux, but for all x86 targets (e.g. FreeBSD).  I
was going to adopt an approach similar to the approach I used for
risc-v, which I think works pretty well, that is have a single struct
that can holds all the features that impact tdesc creation, give that
struct a hash() function, and then use this as the key for caching
created tdesc.

I'll take a look and see if I can do something part way towards this
solution as part of this patch which would address your immediate
concerns.

Thanks,
Andrew
  

> >> 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/afff428f38a1f03ac0127797deda524d81ec1
> 56e
> >
> > 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?
>
> You are 100% correct.  And I agree with everything you've said.
> 
> I also spotted this issue and had a choice to make.
> 
> I could:
> 
>   (a) fix the caching mechanism first, which would require duplicating
>   the effort in GDB and gdbserver, then
> 
>   (b) merge the GDB and gdbserver code.
> 
> Or I could:
> 
>   (a) merge the GDB and gdbserver code, then
> 
>   (b) fix the caching mechanism.
> 
> As you can see, I opted to merge the code first, but this was already 11
> patches.  Which is going to be at least 12 in v6.
> 
> Realistically, if I tried to "fix" caching at the same time then either
> each patch is going to get much bigger (and break the one change per
> patch rule) or the series is going to start getting really long.
> 
> I've stopped where I have to try and keep the number of patches down; I
> don't want to overload reviewers.  Plus, if I write too much on top of
> earlier patches and get asked to rewrite things the rewrites become more
> work.

I agree that we need to take this step by step and try to keep this series
focused on the actual goal. For your sake and the sake of reviewers.

> My plan after this series is merged was to unify all of the x86 tdesc
> caching, not just for Linux, but for all x86 targets (e.g. FreeBSD).  I
> was going to adopt an approach similar to the approach I used for
> risc-v, which I think works pretty well, that is have a single struct
> that can holds all the features that impact tdesc creation, give that
> struct a hash() function, and then use this as the key for caching
> created tdesc.
> 
> I'll take a look and see if I can do something part way towards this
> solution as part of this patch which would address your immediate
> concerns.

Thanks a lot, I wasn't aware that risc-v already has a solution for this.

Just for the record, I also would find it ok to not change to a
map for this series yet. Though I for sure won't object to doing it
here either.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

"Willgerodt, Felix" <felix.willgerodt@intel.com> writes:

>> >> 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/afff428f38a1f03ac0127797deda524d81ec1
>> 56e
>> >
>> > 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?
>>
>> You are 100% correct.  And I agree with everything you've said.
>> 
>> I also spotted this issue and had a choice to make.
>> 
>> I could:
>> 
>>   (a) fix the caching mechanism first, which would require duplicating
>>   the effort in GDB and gdbserver, then
>> 
>>   (b) merge the GDB and gdbserver code.
>> 
>> Or I could:
>> 
>>   (a) merge the GDB and gdbserver code, then
>> 
>>   (b) fix the caching mechanism.
>> 
>> As you can see, I opted to merge the code first, but this was already 11
>> patches.  Which is going to be at least 12 in v6.
>> 
>> Realistically, if I tried to "fix" caching at the same time then either
>> each patch is going to get much bigger (and break the one change per
>> patch rule) or the series is going to start getting really long.
>> 
>> I've stopped where I have to try and keep the number of patches down; I
>> don't want to overload reviewers.  Plus, if I write too much on top of
>> earlier patches and get asked to rewrite things the rewrites become more
>> work.
>
> I agree that we need to take this step by step and try to keep this series
> focused on the actual goal. For your sake and the sake of reviewers.
>
>> My plan after this series is merged was to unify all of the x86 tdesc
>> caching, not just for Linux, but for all x86 targets (e.g. FreeBSD).  I
>> was going to adopt an approach similar to the approach I used for
>> risc-v, which I think works pretty well, that is have a single struct
>> that can holds all the features that impact tdesc creation, give that
>> struct a hash() function, and then use this as the key for caching
>> created tdesc.
>> 
>> I'll take a look and see if I can do something part way towards this
>> solution as part of this patch which would address your immediate
>> concerns.
>
> Thanks a lot, I wasn't aware that risc-v already has a solution for this.
>
> Just for the record, I also would find it ok to not change to a
> map for this series yet. Though I for sure won't object to doing it
> here either.

I'd like to update my response to your feedback a little.

You originally asked:

> 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.

And don't think I was clear enough in my reply.

As far as this patch is concerned, caching based only on xcr0 is
absolutely fine.  Remember that this series is only for Linux, and for
Linux the other (non xcr0) flags that are used when creating a target
description (the is_linux and segments arguments to
{amd64,i386}_create_target_description) are always given fixed values.
As such we don't need to consider these flags at all when caching target
descriptions (for x86 Linux).

Once this series is merged, as I've said, I'd like to look at extending
the caching to all x86 targets, in which case we will need to consider
all the flags.

The reason that I switched from an array to a map in this commit is
because I wanted to avoid hard coding the array sizes for each x86
target type (i386, amd64, x32):

  - Only in gdb/arch/x86-linux-tdesc-features.c could we make use of the
    constexpr x86_linux_*_tdesc_count_1() functions to create fixed
    C-style arrays of the required sizes, this would require all of the
    caching code to live in this file, which didn't seem right,

  - I could move to a dynamic array like structure, e.g. std::vector, I
    could then grow the cache as needed, but I figure if I'm going to
    change the data structure anyway, then I might as well go with a
    map,

  - So I switched to use a map structure, keyed off the xcr0, which is
    fine, because (for x86 Linux) that's the only value that matters.

I'll update the commit message to try and explain all this a bit
better.

Thanks,
Andrew