Re: [PATCH v6 15/15] gdbserver: Simplify handling of ZMM registers.

[Simon Marchi <simon.marchi@polymtl.ca>]

On 7/14/23 11:51, John Baldwin wrote:
> - Reuse num_xmm_registers directly for the count of ZMM0-15 registers
>   as is already done for the YMM registers for AVX rather than using
>   a new variable that is always the same.
> 
> - Replace 3 identical variables for the count of upper ZMM16-31
>   registers with a single variable.  Make use of this to merge
>   various loops working on the ZMM XSAVE region so that all of the
>   handling for the various sub-registers in this region are always
>   handled in a single loop.
> 
> - While here, fix some bugs in i387_cache_to_xsave on where if
>   X86_XSTATE_ZMM was set on i386 (e.g. a 32-bit process on a 64-bit
>   kernel), the -1 register nums would wrap around and store the
>   value of GPRs in the XSAVE area.  This should be harmless, but
>   is definitely odd.  Instead, check num_zmm_high_registers directly
>   when checking X86_XSTATE_ZMM and skip the ZMM region handling
>   entirely if the register count is 0.
> ---
>  gdbserver/i387-fp.cc | 132 +++++++++++++++----------------------------
>  1 file changed, 46 insertions(+), 86 deletions(-)
> 
> diff --git a/gdbserver/i387-fp.cc b/gdbserver/i387-fp.cc
> index f53a6cfc477..91d3a0b8ca3 100644
> --- a/gdbserver/i387-fp.cc
> +++ b/gdbserver/i387-fp.cc
> @@ -267,12 +267,8 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
>  
>    /* Amd64 has 16 xmm regs; I386 has 8 xmm regs.  */
>    int num_xmm_registers = amd64 ? 16 : 8;
> -  /* AVX512 extends the existing xmm/ymm registers to a wider mode: zmm.  */
> -  int num_avx512_zmmh_low_registers = num_xmm_registers;
> -  /* AVX512 adds 16 extra regs in Amd64 mode, but none in I386 mode.*/
> -  int num_avx512_zmmh_high_registers = amd64 ? 16 : 0;
> -  int num_avx512_ymmh_registers = amd64 ? 16 : 0;
> -  int num_avx512_xmm_registers = amd64 ? 16 : 0;
> +  /* AVX512 adds 16 extra ZMM regs in Amd64 mode, but none in I386 mode.*/
> +  int num_zmm_high_registers = amd64 ? 16 : 0;

The naming of these variables is confusing because "high" can mean the
high parts of the registers, or it can mean registers 16 to 31.  I wish
we could find different words to express both things.  But at least your
patch doesn't make things worse (it makes things better by eliminating
some variables).

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

Simon