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

[John Baldwin <jhb@FreeBSD.org>]

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