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

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

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.

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.

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

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

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