Re: [PATCH v6 13/15] gdbserver: Use x86_xstate_layout to parse the XSAVE extended state area.

John Baldwin
2023-08-28T18:38:14Z

On 8/28/23 11:15 AM, Simon Marchi wrote:
> On 7/14/23 11:51, John Baldwin wrote:
>> From: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
>>
>> Replace the extended state area fields of i387_xsave with methods which
>> return an offset into the XSAVE buffer.
>>
>> The two changed functions are called within all tests which runs
>> gdbserver.
>>
>> Signed-off-by: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
>> Co-authored-by: John Baldwin <jhb@FreeBSD.org>
>> ---
>> gdbserver/i387-fp.cc | 117 +++++++++++++++++++++++++------------------
>> 1 file changed, 69 insertions(+), 48 deletions(-)
>>
>> diff --git a/gdbserver/i387-fp.cc b/gdbserver/i387-fp.cc
>> index a122e2d860b..f53a6cfc477 100644
>> --- a/gdbserver/i387-fp.cc
>> +++ b/gdbserver/i387-fp.cc
>> @@ -82,6 +82,8 @@ struct i387_fxsave {
>> };
>>
>> struct i387_xsave : public i387_fxsave {
>> + /* Size of i387_fxsave is 416 bytes. */
>
> Add a gdb_static_assert to make sure of that?

Yes, I can add that. I thought of that when re-reading the patch today myself.

>> + /* Memory address of 16 512-bit zmm16-31 values. */
>> + unsigned char *zmm_space ()
>> + { return xsave () + xsave_layout.zmm_offset; }
>
> Since this doesn't Would it be clearer to call this field (and method) zmm16_offset
> (zmm16_space), to make it clear that it's the space for the zmm
> registers starting at 16?

Oh, yeah, that's a good suggestion.

> In any case:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Simon

--
John Baldwin