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

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

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