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

[John Baldwin <jhb@FreeBSD.org>]

On 4/11/23 7:23 AM, Pedro Alves wrote:
> 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.

At present this object is not supported across the remote protocol.  Instead,
both GDB and gdbserver use this internally to extract register values from an
XSAVE register set and the individual register values are passed across the
remote protocol boundary.  That said, in another role I have of maintaining a
GDB stub for a hypervisor on FreeBSD, it would be really nice if I could send
the XSAVE blob (and some sort of layout information) across the boundary and
let GDB deal with pulling out all the sub-registers from the blob instead of
doing that work in the GDB stub.  I think though if we wanted to ever support
that, we would not pass this raw XSAVE_LAYOUT object across the wire, but
instead add a new xfer "thing" that matches the new core dump note earlier in
this series where you get a set of CPUID leaf values that the layout is then
synthesized from.  That is, the proposed core dump note would be an array
of structures that are something like:

struct {
     uint32_t cpuid_leaf; /* EAX input */
     uint32_t cpuid_subleaf; /* ECX input */
     uint32_t eax;  /* results of CPUID */
     uint32_t ebx;
     uint32_t ecx;
     uint32_t edx;
};

It would be up to the stub to provide enough "meaningful" leafs (e.g. the sub-leaf
for each feature active in XCR0 and the 7/0 top-level XSAVE leaf) to construct
the XSAVE layout.  You could even imagine perhaps a scheme for the remote protocol
where you can encode the leaf/subleaf in the annex so that the debugger could
request specific leafs as-needed.  (In fact, I'd probably prefer that latter method).
However, we would only need that if we wanted to support sending the raw XSAVE
blob back and forth across the protocol rather than the current approach of
just reading/writing individual registers over the protocol.  Dealing with
registers is probably simpler over-all, just requires a bit of duplicated work
in each stub/server to parse the XSAVE layout.

>>> +/* 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;
>    }

Hmm, we could (and there are some other cases like the aarch64_features
struct similarly used to identify tdesc properties).  One risk of the
memcmp approach is if there is any compiler-inserted padding then
comparing the padding bytes might be UB.  Presumably the default comparison
in C++20 exists partly so that the compiler can DTRT with well-defined
behavior in case there is any padding.

-- 
John Baldwin