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

[Luis Machado <luis.machado@arm.com>]

On 3/21/24 17:28, John Baldwin wrote:
> On 3/19/24 11:34 AM, Andrew Burgess wrote:
>> John Baldwin <jhb@FreeBSD.org> writes:
>>
>>> 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.
>>
>> Yes, totally.  And it's not like it would even add much code to the IPA,
>> I believe it already pulls in all of the target description creation
>> code, the only thing that is "saved" is the code that probes the
>> hardware to figure out which description we should use.
>>
>> This whole area seems like it has not aged well :-/
>>
>>>
>>> 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.
>>
>> Indeed.  I did the worked on the feature/hash/lookup for RISC-V so I had
>> this in mind when looking at this code.  Originally I had planned to
>> switch to a map style cache, but when I found the IPA/index passing code
>> I lost my nerve and tried to find a solution that was inline with what
>> we had currently.
>>
>> I don't really know about the IPA enough to make claims about what
>> is/isn't a reasonable change in this area.
> 
> I have zero knowledge about the IPA myself.  OTOH, if no one else steps
> up with a strong opinion here, I would suggest that you go ahead and
> fix the IPA as you described so it is more self-contained and then we can
> use a saner approach for dealing with tdesc caching for x86.
> 

Should we start thinking of deprecating the IPA if no one shows any interest in
keeping it alive? It is there to provide support for (fast/static) tracepoints,
and I haven't seen anyone using it in the open for years now.

The cost of maintenance may not justify keeping it alive.