[PATCH v4 01/13] x86: Add an x86_xsave_layout structure to handle variable XSAVE layouts.

[John Baldwin]

On 4/6/23 12:09 PM, Simon Marchi wrote:
> On 3/17/23 21:08, John Baldwin wrote:
>> The standard layout of the XSAVE extended state area consists of three
>> regions.  The first 512 bytes (legacy region) match the layout of the
>> FXSAVE instruction including floating point registers, MMX registers,
>> and SSE registers.  The next 64 bytes (XSAVE header) contains a header
>> with a fixed layout.  The final region (extended region) contains zero
>> or more optional state components.  Examples of these include the
>> upper 128 bits of YMM registers for AVX.
>>
>> These optional state components generally have an
>> architecturally-fixed size, but they are not assigned architectural
>> offsets in the extended region.  Instead, processors provide
>> additional CPUID leafs describing the size and offset of each
>> component in the "standard" layout for a given CPU.  (There is also a
>> "compact" format which uses an alternate layout, but existing OS's
>> currently export the "standard" layout when exporting XSAVE data via
>> ptrace() and core dumps.)
>>
>> To date, GDB has assumed the layout used on current Intel processors
>> for state components in the extended region and hardcoded those
>> offsets in the tables in i387-tdep.c and i387-fp.cc.  However, this
>> fails on recent AMD processors which use a different layout.
>> Specifically, AMD Ryzen 9 processors at least do not leave space for
>> the MPX register set in between the AVX and AVX512 register sets.  It
>> is not known if other AMD processors are effected, but seems probable.
>>
>> To rectify this, add an x86_xsave_layout structure which contains the
>> total size of the XSAVE extended state area as well as the offset of
>> each known optional state component.  This structure is stored in
>> i386_gdbarch_tdep and is fetched from the current target in
>> i386_gdbarch_init as a TARGET_OBJECT_X86_XSAVE_LAYOUT object.
>>
>> Subsequent commits will modify XSAVE parsing to use x86_xsave_layout.
> 
> Thanks for the nice description, it helped me, since I was not familiar
> with that feature.  Just some minor comments and questions.
> 
>> Co-authored-by: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
>> ---
>>   gdb/i386-tdep.c         | 36 +++++++++++++++++++++++++-
>>   gdb/i386-tdep.h         |  4 +++
>>   gdb/target.h            |  2 ++
>>   gdbsupport/x86-xstate.h | 57 +++++++++++++++++++++++++++++++++++------
>>   4 files changed, 90 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>> index 96c04c1a3d6..5c2d9e42d8d 100644
>> --- a/gdb/i386-tdep.c
>> +++ b/gdb/i386-tdep.c
>> @@ -8493,19 +8493,51 @@ i386_type_align (struct gdbarch *gdbarch, struct type *type)
>>   }
>>   
>>   

>> +/* Fetch the XSAVE layout for the current target.  */
>> +
>> +static struct x86_xsave_layout
> 
> Nit: drop the struct keyword whenever you use x86_xsave_layout.

Will fix in the entire series.

>> +i386_fetch_xsave_layout ()
>> +{
>> +  struct x86_xsave_layout layout;
>> +  LONGEST len = target_read (current_inferior ()->top_target (),
>> +			     TARGET_OBJECT_X86_XSAVE_LAYOUT, nullptr,
>> +			     (gdb_byte *) &layout, 0, sizeof (layout));
>> +  if (len != sizeof (layout))
>> +    return {};
> 
> Do you have an idea of which conditions can make it so `len != sizeof
> (layout)`?  If I understand correctly, the contract for
> TARGET_OBJECT_X86_XSAVE_LAYOUT is that the caller passes a pointer to an
> x86_xsave_layout object and the callee fills it.  This is all internal
> to GDB.  I don't imagine how a target could return something else than
> ;`sizeof (layout)` (indicating success) or TARGET_XFER_E_IO (indicating
> failure), other than being buggy.
> 
> All of this to say, should it be an assert?

Hmm, in the case of TARGET_XFER_E_IO this function needs to return an
empty object (e.g. for native targets that don't support XSAVE at all).

I should perhaps make it check for an error explicitly and then assert
that otherwise they are equal?

>> +
>> +  return layout;
>> +}
>> +
>>   /* Note: This is called for both i386 and amd64.  */
>>   
>>   static struct gdbarch *
>>   i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>   {
>>     const struct target_desc *tdesc;
>> +  struct x86_xsave_layout xsave_layout;
>>     int mm0_regnum;
>>     int ymm0_regnum;
>>     int bnd0_regnum;
>>     int num_bnd_cooked;
>>   
>> +  xsave_layout = i386_fetch_xsave_layout ();
> 
> Declare the variable where you initialize it.

Fixed.

>> +
>>     /* If there is already a candidate, use it.  */
>> -  arches = gdbarch_list_lookup_by_info (arches, &info);
>> +  for (arches = gdbarch_list_lookup_by_info (arches, &info);
>> +       arches != NULL;
>> +       arches = gdbarch_list_lookup_by_info (arches->next, &info))
>> +    {
>> +      /* Check that the XSAVE layout of ARCHES matches the layout for
>> +	 the current target.  */
>> +      i386_gdbarch_tdep *other_tdep
>> +	= (i386_gdbarch_tdep *) gdbarch_tdep (arches->gdbarch);
> 
> This needs to be updated to gdbarch_tdep<i386_gdbarch_tdep> (...).

Humm, I had fixed these when rebasing it, but I must have fixed this later
in the series.  I've fixed it here now.

>> @@ -8762,6 +8794,8 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>         gdbarch_free (gdbarch);
>>         return NULL;
>>       }
>> +  if (tdep->xcr0 != 0)
>> +    tdep->xsave_layout = xsave_layout;
> 
> Just wondering if the "if" is really necessary.  If nothing is
> supported, I suppose the RHS xsave_layout will be a default one.  And
> the LHS is always a default one, having just been created.

That's fair.

>> @@ -145,6 +146,9 @@ struct i386_gdbarch_tdep : gdbarch_tdep_base
>>     /* Offset of XCR0 in XSAVE extended state.  */
>>     int xsave_xcr0_offset = 0;
> 
> Does this represent the offset the saved value of xcr0 in the xsave
> area?  I haven't read that the xcr0 value would be saved there, but this
> comment surely makes it sound like it.  If so, does it belong inside
> x86_xsave_layout?

So both Linux and FreeBSD re-use a reserved field in the normal FXSAVE
state area to store XCR0 in the XSAVE note saved in core dumps.  The
gdbarch_core_read_description methods then fetch the XCR0 mask from the first
XSAVE core dump note to determine the XCR0 mask used for the core dump
(and thus the XSAVE size and layout).  Note that unlike the fields in
xsave_layout (which are a description of architecture-specified behavior),
this field is a software construct followed by Linux and FreeBSD.

Note that the new core dump note discussed in earlier versions of this
series would result in this field probably not being used for core dumps
which include the new note.

>> +/* Size and offsets of register states in the XSAVE area extended
>> +   region.  Offsets are set to 0 to indicate the absence of the
>> +   associated registers.  */
>> +
>> +struct x86_xsave_layout
>> +{
>> +  int sizeof_xsave = 0;
>> +  int avx_offset = 0;
>> +  int bndregs_offset = 0;
>> +  int bndcfg_offset = 0;
>> +  int k_offset = 0;
>> +  int zmm_h_offset = 0;
>> +  int zmm_offset = 0;
>> +  int pkru_offset = 0;
>> +
>> +  bool operator!= (const x86_xsave_layout &other)
>> +  {
>> +    return sizeof_xsave != other.sizeof_xsave
>> +      || avx_offset != other.avx_offset
>> +      || bndregs_offset != other.bndregs_offset
>> +      || bndcfg_offset != other.bndcfg_offset
>> +      || k_offset != other.k_offset
>> +      || zmm_h_offset != other.zmm_h_offset
>> +      || zmm_offset != other.zmm_offset
>> +      || pkru_offset != other.pkru_offset;
>> +  }
> 
> Heh, would be nice to be able to use
> 
> https://en.cppreference.com/w/cpp/language/default_comparisons

Huh, I didn't know about that feature, but yes that would be nice
to use eventually.

-- 
John Baldwin