Re: [PATCHv2] gdb/x86: Handle kernels using compact xsave format

[Pedro Alves <palves@redhat.com>]

On 05/03/2018 06:37 PM, Andrew Burgess wrote:

> Hope that helps,

It does, thank you.

The patch looks good to me, with the following nits
addressed.

Thanks much for doing all this.  Nice work.  Bonus points
for the testcase.

In the commit log:

> had not yet enableld the x87 feature then within GDB we would see

"enableld" -> "enabled"

> @@ -844,39 +908,67 @@ i387_xsave_to_cache (struct regcache *regcache, const void *buf)
>  	}
>      }
>  
> -  supply_register_by_name (regcache, "fioff", &fp->fioff);
> -  supply_register_by_name (regcache, "fooff", &fp->fooff);
> -  supply_register_by_name (regcache, "mxcsr", &fp->mxcsr);
> +  if ((clear_bv & (X86_XSTATE_SSE | X86_XSTATE_AVX)) ==
> +      (X86_XSTATE_SSE | X86_XSTATE_AVX))
> +    {

== sign goes on next line.

> +ULONGEST
> +i387_xsave_get_clear_bv (struct gdbarch *gdbarch, const void *xsave)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  const gdb_byte *regs = (const gdb_byte *) xsave;
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  ULONGEST clear_bv, xstate_bv = 0;

I see this is factored out from elsewhere, but note we can skip initializing
to zero when ...

> +
> +  /* Get `xstat_bv'.  The supported bits in `xstat_bv' are 8 bytes.  */
> +  xstate_bv = extract_unsigned_integer (XSAVE_XSTATE_BV_ADDR (regs),
> +					8, byte_order);
... we're immediately overriding with some value.

While at it, we can declare the variables at their initialization
to avoid potential for confusion:

  /* Get `xstat_bv'.  The supported bits in `xstat_bv' are 8 bytes.  */
  ULONGEST xstate_bv = extract_unsigned_integer (XSAVE_XSTATE_BV_ADDR (regs),
						 8, byte_order);

  /* Clear part in vector registers if its bit in xstat_bv is zero.  */
  ULONGEST clear_bv = (~(xstate_bv)) & tdep->xcr0;


> +
> +  /* Clear part in vector registers if its bit in xstat_bv is zero.  */
> +  clear_bv = (~(xstate_bv)) & tdep->xcr0;
> +
> +  return clear_bv;
> +}
> +

> +		/* Initial value for fctrl register, as defined in the X86
> +		   manual, and confirmed in the (Linux) kernel source.  */
> +		store_unsigned_integer (buf, 4, byte_order, 0x037f);

This constant appears in more than one place.  How about giving it a
name, like I387_FCTRL_INIT_VAL or some such.  Then the describing
comment can be moved/centralized there too.  Likewise 0x1f80 for mxcsr.

> +		regcache_raw_supply (regcache, i, buf);


> +      /* The mxcsr register is written into the xsave buffer if either AVX
> +	 or SSE is enabled, so only clear it if both of those features
> +	 require clearing.  */
> +      if ((clear_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE))
> +	  == (X86_XSTATE_AVX | X86_XSTATE_SSE))
> +	{
> +	  gdb_byte buf[2];
>  
> -	  if ((clear_bv & X86_XSTATE_AVX))
> -	    for (i = I387_YMM0H_REGNUM (tdep);
> -		 i < I387_YMMENDH_REGNUM (tdep); i++)
> -	      memset (XSAVE_AVXH_ADDR (tdep, regs, i), 0, 16);
> +	  store_unsigned_integer (buf, 2, byte_order, 0x1f80);
> +	  memcpy (FXSAVE_ADDR (tdep, regs, i), buf, 2);

Do we need to extract to a buffer and then memcpy
the buffer to the destination?  Why not extract to the
destination directly, like:

	  store_unsigned_integer (FXSAVE_ADDR (tdep, regs, i),
                                  2, byte_order, 0x1f80);

Ditto here:

> +	      if (i == I387_FCTRL_REGNUM (tdep))
> +		{
> +		  gdb_byte buf[2];
> +
> +		  store_unsigned_integer (buf, 2, byte_order, 0x037f);
> +		  memcpy (FXSAVE_ADDR (tdep, regs, i), buf, 2);
> +		}



> +      /* Check if any X87 registers are changed.  Only the non-control
> +	 registers are handled here, the control registers are all */
> +      if ((tdep->xcr0 & X86_XSTATE_X87))

Incomplete sentence.


> +		  /* We're only setting MXCSR, so check the initial state
> +		     to see if either of AVX or SSE are already enabled.
> +		     If they are then we'll attribute this changed MXCSR to
> +		     that feature.  If neither feature is enabled, then
> +		     we'll attribute this change to the SSE feature.  */
> +		  xstate_bv |=
> +		    (initial_xstate_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE));

'!=' goes on the next line.

> +		  if ((xstate_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE)) == 0)
> +		    xstate_bv |= X86_XSTATE_SSE;
> +		  memcpy (p, raw, 2);


> +++ b/gdb/gdbserver/regcache.c
> @@ -346,6 +346,17 @@ supply_register_zeroed (struct regcache *regcache, int n)
>  #endif
>  }
>  
> +#ifndef IN_PROCESS_AGENT
> +
> +void
> +supply_register_by_name_zeroed (struct regcache *regcache,
> +				const char *name)
> +{

Missing intro comment.


On the testcase:

> +# Test initial values of x87 control registers, before any x87
> +# instructions have been executed in the inferior.
> +
> +if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
> +    return
> +}

Should probably be is_amd64_regs_target, so that
both "i386-* -m64" and "x32" exercise it too.

> +    gdb_test "p/x \$${regname}" " = ${regvalue}"  "Check initial value of \$${regname}"

Please use lowercase test names throughout:

 "Check" -> "check",
 "Step" -> "step", 
etc.

> +# Grab the address of this instruction, it will appear in later
> +# results.
> +set addr [capture_command_output "p/x \$pc" "\\\$$decimal = "]
> +

Could be:

 set addr [get_hexadecimal_valueof "\$pc" "0"]

> +# ===========================================================
> +
> +clean_restart ${binfile}
> +
> +if ![runto_main] then {
> +    fail "can't run to main"
> +    return 0
> +}
> +
> +gdb_test_no_output "set \$mxcsr=0x9f80" "Set a new value for MXCSR"
> +gdb_test "stepi" "fwait" "Step forward one instruction for mxcsr test"
> +gdb_test "p/x \$mxcsr" " = 0x9f80" "Check new value of MXCSR is still in place"
> +

Please wrap these test sequences with e.g., with_test_prefix, like:

with_test_prefix "something here"  {
  clean_restart ${binfile}

  if ![runto_main] then {
      fail "can't run to main"
      return 0
  }
  gdb_test_no_output "set \$mxcsr=0x9f80" "set new value for MXCSR"
  gdb_test "stepi" "fwait" "step forward one instruction for mxcsr test"
  gdb_test "p/x \$mxcsr" " = 0x9f80" "check new value of MXCSR still in place"
}

This way, the runto_main failure gets a distinct FAIL message.
Plus, it gives a nice visual indication of the separate
gdb restarts/testing, without the need for the ======= comment lines.
A short leading sentence indicating what each ======= region
is about to test would be nice too.

If instead of with_test_prefix you put the subtests in procedures,
like 

 proc_with_prefix something-here {} {
   ...
 }

then an early FAIL will still continue testing other tests that
don't rely on the same gdb invocation.

Thanks,
Pedro Alves