Re: [RFC PATCH 0/4] Handle variable XSAVE layouts

[John Baldwin <jhb@FreeBSD.org>]

On 3/17/22 6:17 AM, Willgerodt, Felix wrote:
> Hi John,
> 
> We looked at this recently while working on AMX support (I hope I can post
> that soon), and decided to leave it hardcoded for now due to the missing
> information for corefiles. We weren't aware of the AMD behaviour.
> 
> You can find an old prototype we discussed internally here:
> https://github.com/intel/gdb/tree/experimental/xsave_offsets
> It is not working for corefiles, which is why we put it on hold for now.
> But it might be interesting to see. I just rebased it quickly from an old
> state, so there might be problems with the patches.

Yes, I like many aspects of this such as moving code to gdbuspport.
I do think it is perhaps more future-proof/cleaner to go ahead and
assume arbitrary offsets for each region (e.g. don't assume a single
relative offset for the 3 AVX-512 regions) which my patch 2 does,
but there are certainly many similarities.
  
> I would like to see the offset info in corefiles in the long run, as you
> mentioned in your earlier mail. To me your approach of hardcoding
> known combinations seems hard to maintain. But obviously making it
> a bit better right now, if there are no conflicting combinations.

So I view hardcoding combinations as a fallback.  I would much prefer
adding a new NT_X86_XSAVE_LAYOUT or the like.  It could be fetched for
live processes using PT_GETREGSET if desired (though using cpuid directly
which both of our patch sets do for native is fine).  The fallback would
exist to handle older core dumps without the new note.  For example, if
we had the note, then the new function in patch 3 would use that note if
it exists and only fall back to calling i387_set_xsave_offsets() when the
note isn't present.

I'm happy to come up with a scheme for the proposed NT_X86_XSAVE_LAYOUT.
A simple layout would be for it to simply contain an array of the
x86_extended_feature type from your structures, but I don't know if it
wouldn't be better to store "raw" CPUID results in case there are future
needs for other bits (like the alignment if we ever wanted to support the
compact format in userland for some reason).  To that end, perhaps a
structure like:

struct {
    uint32_t id;
    uint32_t size;    /* eax */
    uint32_t offset;  /* ebx */
    uint32_t ecx;
    uint32_t edx;
};

Where the note is just an array of those?  The total size of the state
can be inferred from the size of the NT_X86_XSTATE note.
    > I responded a bit more below.
> 
>> -----Original Message-----
>> From: Gdb-patches <gdb-patches-
>> bounces+felix.willgerodt=intel.com@sourceware.org> On Behalf Of John
>> Baldwin
>> Sent: Mittwoch, 16. März 2022 20:46
>> To: gdb-patches@sourceware.org
>> Subject: [RFC PATCH 0/4] Handle variable XSAVE layouts
>>
>> This is a first attempt at resolving the issue with XSAVE I described
>> previously.  There are more details in the commit logs, but here I think
>> will describe some caveats about the current prototype:
>>
>> - It is probably terrible performance-wise to be reading the offsets
>>    from the target every time collect/supply_xsave is called.  I'd
>>    actually much prefer to store these (along with the total XSAVE area
>>    size) in the tdep.  The issue is that you can have gdbarches with the
>>    same tdesc that use different layouts (e.g. if you open a core dump
>>    from an Intel CPU on a host with an AMD CPU, the two CPUs could have
>>    identical XCR0 masks, but the layout in the core dump wouldn't match
>>    the layout of a live process).  Perhaps if I can fetch the offsets
>>    from the target in i386_gdbarch_init though I can iterate over
>>    matching arches looking for a match.
> 
> I don't quite understand why storing them in tdep wouldn't work.
> We get XCR0 from the coredump, not from the CPU analysing
> the coredump. For live targets we would query CPUID on GDB/gdbserver.
> I don't see how this would clash in your example, but maybe I missed
> something in your patches.

The problem is that two tdep's with the same XCR0 value currently
have an identical tdesc and thus share the same 'struct gdbarch'.
However, an Intel CPU with XCR0 of 0x207 uses a different layout
than an AMD CPU with an XCR0 of 0x207.  We would thus need separate
gdbarches for those.  I think though I can make that work if I fetch
TARGET_OBJECT_X86_XSAVE_OFFSETS in i386_gdbarch_init() before this
loop:

   /* If there is already a candidate, use it.  */
   arches = gdbarch_list_lookup_by_info (arches, &info);
   if (arches != NULL)
     return arches->gdbarch;

And instead only return an existing gdbarch if it has the same XSAVE
layout.  For example, RISC-V does the following logic to handle
differences in gdbarches that aren't fully handled by the tdesc:

   /* Find a candidate among the list of pre-declared architectures.  */
   for (arches = gdbarch_list_lookup_by_info (arches, &info);
        arches != NULL;
        arches = gdbarch_list_lookup_by_info (arches->next, &info))
     {
       /* Check that the feature set of the ARCHES matches the feature set
	 we are looking for.  If it doesn't then we can't reuse this
	 gdbarch.  */
       riscv_gdbarch_tdep *other_tdep
	= (riscv_gdbarch_tdep *) gdbarch_tdep (arches->gdbarch);

       if (other_tdep->isa_features != features
	  || other_tdep->abi_features != abi_features)
	continue;

       break;
     }

   if (arches != NULL)
     return arches->gdbarch;

I think it would also be handy in this case to extend the xsave_offsets
structure to include the total size that can be used in the collect/supply
callbacks.

>> - The cpuid function I added in patch 3 isn't FreeBSD-specific at all
>>    (and would work on i386).  I could add it to x86-nat.c instead
>>    easily enough.  Even if OS's start providing a new ptrace op
>>    to fetch this info we probably should ship a cpuid-based variant as
>>    a fallback?
> 
> We will also need this in gdbserver, so maybe gdbsupport or nat/ is
> the better place. I personally don't see a new ptrace op coming,
> as ptrace won't help us with corefiles either. But that is just my guess.

I would only see the ptrace op being one that comes "for free" via
PT_GETREGSET to fetch the new core dump note.  However, I'm also happy
to just use cpuid always for native targets.

>> - The collect size I used in patch 3 for the XSAVE register set
>>    isn't really correct.  Really if I had the "real" XSAVE register
>>    set size available in the tdep (see point 1) I would not set
>>    REGSET_VARIABLE_SIZE and instead use tdep->sizeof_xsave for both
>>    sizes.
>>
>> - I have no idea how gdbserver is impacted.  So far I haven't really
>>    found similar tables to i387-tdep.c in gdbserver.  (It's also harder
>>    for me to test currently as I haven't yet added FreeBSD support to
>>    gdbserver).
>>
> 
> The gdbserver part is in i387-fp.cc. It is quite similar to i386-tdep.c.
> There is a struct i387_xsave, which also assumes a fixed layout.

Ok, I had read that, but the structure wasn't obvious to me.

>> - I haven't added any support for fetching the offsets on Linux (the
>>    only other OS that supports XSAVE state currently).  I am waiting
>>    to have the design a bit more finalized before doing that.
>>
>> John Baldwin (4):
>>    x86: Add an xsave_offsets structure to handle variable XSAVE layouts.
>>    core: Support fetching TARGET_OBJECT_X86_XSAVE_OFFSETS from
>>      architectures.
>>    Update x86 FreeBSD architectures to support XSAVE offsets.
>>    Support XSAVE layouts for the current host in the FreeBSD/amd64
>>      target.
>>
>>   gdb/amd64-fbsd-nat.c      |  73 +++++
>>   gdb/amd64-fbsd-tdep.c     |   8 +-
>>   gdb/corelow.c             |  22 ++
>>   gdb/gdbarch-components.py |  13 +
>>   gdb/gdbarch-gen.h         |  10 +
>>   gdb/gdbarch.c             |  32 +++
>>   gdb/i386-fbsd-tdep.c      |  33 ++-
>>   gdb/i386-fbsd-tdep.h      |   6 +
>>   gdb/i387-tdep.c           | 592 +++++++++++++++++++++++++-------------
>>   gdb/i387-tdep.h           |  22 ++
>>   gdb/target.h              |   2 +
>>   11 files changed, 607 insertions(+), 206 deletions(-)
>>
>> --
>> 2.34.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928


-- 
John Baldwin