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

[Andrew Burgess <aburgess@redhat.com>]

"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