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

[John Baldwin <jhb@FreeBSD.org>]

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.

-- 
John Baldwin