RE: [PATCH] x86-64: fix ZMM register state tracking

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.09.18 at 19:19, <markus.t.metzger@intel.com> wrote:
>> --- a/gdb/i387-tdep.c
>> +++ b/gdb/i387-tdep.c
>> @@ -923,7 +923,8 @@ i387_supply_xsave (struct regcache *regc
>>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>>    const gdb_byte *regs = (const gdb_byte *) xsave;
>> -  int i;
>> +  int i, zmm_endlo_regnum = I387_ZMM0H_REGNUM (tdep)
>> +			    + std::min (tdep->num_zmm_regs, 16);
> 
> It would be nice to comment on this magic 16 and the min operation.
> It's how XSAVE organizes things but it isn't entirely intuitive.

Okay, I can accept the desire for a (new) comment here, albeit it pretty
much goes along the lines of what I say further down.

>> @@ -1002,7 +1003,8 @@ i387_supply_xsave (struct regcache *regc
>>        return;
>> 
>>      case avx512_zmm_h:
>> -      if ((clear_bv & (X86_XSTATE_ZMM_H | X86_XSTATE_ZMM)))
>> +      if ((clear_bv & (regnum < zmm_endlo_regnum ? X86_XSTATE_ZMM_H
>> +						 : X86_XSTATE_ZMM)))
> 
> A comment that XSAVE stores the lower 16 registers in a different place
> than the higher 16 registers and also guards them by different XCR0 bits
> would be nice.

Such a comment would have been appropriate already prior to my patch.
I have to admit that I'm not inclined to invest time in improving
pre-existing (lack of) commentary, the more that how to exactly word
this might be controversial. If that's really a requirement, then I'd
abandon the patch (perhaps for someone else to pick up), maintaining it
locally until the bug has been fixed upstream by whatever means. This
is even more so that the extra time I'll have to put into playing with
Simon's test case is also nothing I really have time for (but I'll try to
find the time nevertheless).

> We hid the different places behind those XSAVE_AVX512_ZMM_H_ADDR
> macros but there's nothing similar for the guard bits.  Maybe add macros
> for the guard check, as well?

Same here - nothing that this patch really changes.

>> @@ -1135,6 +1135,9 @@ i387_supply_xsave (struct regcache *regc
>>  	    }
>>  	  else
>>  	    {
>> +	      for (i = zmm_endlo_regnum; i < I387_ZMMENDH_REGNUM (tdep); i++)
>> +		regcache->raw_supply (i,
>> +				      XSAVE_AVX512_ZMM_H_ADDR (tdep, regs, i));
> 
> This covers the upper halves of zmm16 to zmm31.  Looking at the function it looks
> like the lower halves are covered in separate cases avx512_ymmh_avx512 and
> avx512_xmmh_avx512.  Maybe reflect this in the comment?  It currently suggests
> that it handles the entire upper 16 registers.

Same here - the split among code regions has been there before (also
for e.g. the YMM register lower and upper halves).

Irrespective of my mostly negative responses thanks for your
comments, Jan