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

[John Baldwin <jhb@FreeBSD.org>]

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