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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

Hi John,

See comments inline.

> -----Original Message-----
> From: Gdb-patches <gdb-patches-
> bounces+felix.willgerodt=intel.com@sourceware.org> On Behalf Of John
> Baldwin
> Sent: Donnerstag, 17. März 2022 19:03
> To: gdb-patches@sourceware.org
> Subject: Re: [RFC PATCH 0/4] Handle variable XSAVE layouts
> 
> On 3/17/22 9:20 AM, John Baldwin wrote:
> > On 3/17/22 6:17 AM, Willgerodt, Felix wrote:
> >>> 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. 

Just out of curiosity: If we wouldn't implement i387_set_xsave_layout(),
and read that info from CPUID and the corefile (once that note exists),
would we still need this?

>> 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.
> 
> I have made these changes and it does appear to work.  I do think the
> approach
> of a new TARGET_OBJECT will work (and it will support adding a new
> NT_X86_XSAVE_LAYOUT or the like in the future to better support core
> dumps).
> If you agree with that part of the design (and storing it in the tdep, etc.
> I can try to merge in parts of your patchset (in particular, moving some
> things to gdbsupport and similar gdbserver patches) or I'm happy to let you
> drive.  I will send a V2 with the changes to store the layout in the tdep.
> 
> --
> John Baldwin


Please feel free to take (and adjust) any code or idea from our patchset
that you like. I just posted it trying to be helpful.


I must admit I am not so sure about your approach. Yes it helps
now. But assume in the future there is a 64-byte component Y and a 64-byte
component X. What happens if one CPU drops X and another drops Y?
We could have the same XCR0 and same XSAVE size and no way to 
distinguish the layouts.
To me there is no real alternative to getting everything from CPUID.
I personally would at least like to see CPUID implemented for live
processes, and a huge comment that this is a last-resort fallback
for corefiles, that could fail. Until we have the corefile stuff figured out.

That said, I am no maintainer and not the right person for deciding.
This is just my point of view.


I looked at the Intel software development manual a bit. There is a
compacted xsave format and a standard format. In GDB we
never check which one is used and assume standard format, afaik.
(I think we have even dropped that information when we are in
I387-tdep.c.)

SDM:
"Standard format. Each state component i (i ≥ 2) is located at the byte
offset from the base address of the XSAVE area enumerated in
CPUID.(EAX=0DH,ECX=i):EBX. (CPUID.(EAX=0DH,ECX=i):EAX enumerates
The number of bytes required for state component I"

MPX is i = 3 (and 4).

That said, MPX is marked as deprecated and I don't know if that
has any influence.

Judging by your XCR0 values in your earlier email, you are not in
compacted mode, right? Could you check the CPUID leaves for MPX?
I wonder what they report.

Felix
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