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

[Pedro Alves <pedro@palves.net>]

On 2023-04-06 8:09 p.m., Simon Marchi via Gdb-patches wrote:

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

How is this meant to work with remote debugging, though?  Is
a TARGET_OBJECT_X86_XSAVE_LAYOUT object going to be crossing over the
remote protocol?  If so, it's fine if the object is binary, but, we need to consider
the case of the host running GDB not being x86, and not even being little endian.
I.e., we can't just cast byte buffer to x86_xsave_layout and expect that it works.

> 
>> +/* 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
> 
> !
> 

In this particular case, we could write the op instead as:

  bool operator!= (const x86_xsave_layout &other)
  {
    /* Just to make sure memcpy isn't comparing padding.  */
    static_assert (sizeof (x86_xsave_layout) == 8 * sizeof (int));

    return memcmp (this, &other, sizeof (x86_xsave_layout)) != 0;
  }