Re: [PATCHv2 6/7] gdbserver: update target description creation for x86/linux

[John Baldwin <jhb@FreeBSD.org>]

On 3/5/24 9:00 AM, Andrew Burgess wrote:
> This commit is part of a series which aims to share more of the target
> description creation between GDB and gdbserver for x86/Linux.
> 
> After some refactoring, the previous commit actually started to share
> some code, we added the shared x86_linux_tdesc_for_tid function into
> nat/x86-linux-tdesc.c.  However, this function still relies on
> amd64_linux_read_description and i386_linux_read_description which are
> implemented separately for both gdbserver and GDB.  Given that at
> their core, all these functions to is:
> 
>    1. take an xcr0 value as input,
>    2. mask out some feature bits,
>    3. look for a cached pre-generated target description and return it
>       if found,
>    4. if no cached target description is found then call either
>       amd64_create_target_description or
>       i386_create_target_description to create a new target
>       description, which is then added to the cache.  Return the newly
>       created target description.
> 
> The inner functions amd64_create_target_description and
> i386_create_target_description are already shared between GDB and
> gdbserver (in the gdb/arch/ directory), so the only thing that
> the *_read_description functions really do is add the caching layer,
> and it feels like this really could be shared.
> 
> However, we have a small problem.
> 
> On the GDB side we create target descriptions using a different set of
> cpu features than on the gdbserver side!  This means that for the
> exact same target, we might get a different target description when
> using native GDB vs using gdbserver.  This surely feels like a
> mistake, I would expect to get the same target description on each.
> 
> The table below shows the number of possible different target
> descriptions that we can create on the GDB side vs on the gdbserver
> side for each target type:
> 
>          | GDB | gdbserver
>    ------|-----|----------
>    i386  | 64  | 7
>    amd64 | 32  | 7
>    x32   | 16  | 7
> 
> So in theory, all I want to do is move the GDB version
> of *_read_description into the nat/ directory and have gdbserver use
> that, then both GDB and gdbserver would be able to create any of the
> possible target descriptions.
> 
> Unfortunately it's a little more complex than that due to the in
> process agent (IPA).
> 
> When the IPA is in use, gdbserver sends a target description index to
> the IPA, and the IPA uses this to find the correct target description
> to use.
> 
> ** START OF AN ASIDE **
> 
> Back in the day I suspect this approach made perfect sense.  However
> since this commit:
> 
>    commit a8806230241d201f808d856eaae4d44088117b0c
>    Date:   Thu Dec 7 17:07:01 2017 +0000
> 
>        Initialize target description early in IPA
> 
> I think passing the index is now more trouble than its worth.
> 
> We used to pass the index, and then use that index to lookup which
> target description to instantiate and use.  However, the above commit
> fixed an issue where we can't call malloc() within (certain parts of)
> the IPA (apparently), so instead we now pre-compute _every_ possible
> target description within the IPA.  The index is now only used to
> lookup which of the (many) pre-computed target descriptions to use.
> 
> It would (I think) have been easier all around if the IPA just
> self-inspected, figured out its own xcr0 value, and used that to
> create the one target description that is required.  So long as the
> xcr0 to target description code is shared (at compile time) with
> gdbserver, then we can be sure that the IPA will derive the same
> target description as gdbserver, and we would avoid all this index
> passing business, which has made this commit so very, very painful.
> 
> ** END OF AN ASIDE **
> 
> Currently then for x86/linux, gdbserver sends a number between 0 and 7
> to the IPA, and the IPA uses this to create a target description.
> 
> However, I am proposing that gdbserver should now create one of (up
> to) 64 different target descriptions for i386, so this 0 to 7 index
> isn't going to be good enough any more (amd64 and x32 have slightly
> fewer possible target descriptions, but still more than 8, so the
> problem is the same).
> 
> For a while I wondered if I was going to have to try and find some
> backward compatible solution for this mess.  But after seeing how
> lightly the IPA is actually documented, I wonder if it is not the case
> that there is a tight coupling between a version of gdbserver and a
> version of the IPA?  At least I'm hoping so.
> 
> In this commit I have thrown out the old IPA target description index
> numbering scheme, and switched to a completely new numbering scheme.
> Instead of the index that is passed being arbitrary, the index is
> instead calculated from the set of cpu features that are present on
> the target.  Within the IPA we can then reverse this logic to recreate
> the xcr0 value based on the index, and from the xcr0 value we can
> create the correct target description.
> 
> With the gdbserver to IPA numbering scheme issue resolved I have then
> update the gdbserver versions of amd64_linux_read_description and
> i386_linux_read_description so that they create target descriptions
> using the same set of cpu features as GDB itself.
> 
> After this gdbserver should now always come up with the same target
> description as GDB does on any x86/Linux target.
> 
> This commit does not introduce any new code sharing between GDB and
> gdbserver as previous commits in this series does.  Instead this
> commit is all about bringing GDB and gdbserver into alignment
> functionally so that the next commit can merge the GDB and gdbserver
> versions of these functions.

It does seem like the IPA case should just compute the tdesc it needs
if I understand you correctly.

BTW, on other arches like aarch64 and I believe risc-v, a flat array
is not used for the tdesc cache.  Now the cache is a std::unordered_map<>
and there is a 'struct foo_features' that has an operator== and
std::hash<> specialization.  It really would be nice if x86 was the same,
but that would require abandoning a notion of a portable "index" shared
between gdbserver and the IPA.

-- 
John Baldwin