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

[John Baldwin <jhb@FreeBSD.org>]

On 3/18/22 6:49 AM, Willgerodt, Felix wrote:
> 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?

Once corefile support exists, this i387_set_xsave_layout() function won't
be used.  To be clear, in the current patches (which don't yet include all
of the Linux-specific changes), CPUID is used for native targets.  This
function is only used for coredumps, and only in OS-specific gdbarch
method to read the XSAVE layout from a core dump.  Once OS's add a new
core dump note to describe the XSAVE layout, those OS-specific gdbarch
methods can read that core dump note instead and will only call
i387_set_xsave_layout() for older core dumps without the note.

I'm happy to work on the core dump note and I can implement it in
FreeBSD easily.  I think it would be nice to agree on a common format that
is also used in Linux, but I'm not a Linux developer and would need someone
else to work on implementing the core dump note in Linux.

>>> 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.

I have tried to make that apparent in the commit log in the V2 of the series
I posted (and have split the commits up a bit further so that it is clear
that the intention is to only use the static layouts as a fallback for
core dumps, not in native targets).

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

I am not a global maintainer either FWIW.

> 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.)

Yes, GDB currently assumes the standard format.  FreeBSD doesn't currently
make use of the compacted format, and Linux is careful to convert state
saved in the compact format to the standard format before exporting the
XSAVE state either via ptrace() or via core dump notes.

That said, there may come a day when OS's may want to export data in
the compacted format.  In that case we may want to compute a different
set of offsets for each state component.  That is the reason I'm inclined
to make the core dump note store all of the information for each active
leaf (all 4 registers from cpuid) so that they can be used to compute
the layout of the compacted format (rather than just storing the
ID, size, and standard offset).
  
> 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.

The AMD CPU in this case doesn't implement MPX so bits 3 and 4 are always
zero in XCR0.  Per the SDM for CPUID, the cpuid leaves return a size and
offset of 0 for those leaves.  This CPU also doesn't support AVX512, so
it reports 0's for those three leaves as well.

-- 
John Baldwin