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

[Andrew Burgess <aburgess@redhat.com>]

"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