Re: [PATCH 1/1] Add support for Intel PKRU register to GDB and GDBserver.

[Pedro Alves <palves@redhat.com>]

On 12/17/2015 03:43 PM, Michael Sturm wrote:

> Intel(R) 64 and IA-32 Architecures Software Developer's

(Architectures)

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 484d98d..74103f0 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -150,6 +150,10 @@ show remote exec-event-feature-packet
>  
>  *** Changes in GDB 7.10
>  
> +* GDB now supports access to the PKU register on GNU/Linux. The register is 
> +  added by the Memory Protection Keys for Userspace feature which will be
> +  available in future Intel CPUs.
> +

Please remember to move this to the 7.11 section.

> +
>  /* Supported mask and size of the extended state.  */
>  #define X86_XSTATE_X87_MASK	X86_XSTATE_X87
>  #define X86_XSTATE_SSE_MASK	(X86_XSTATE_X87 | X86_XSTATE_SSE)
>  #define X86_XSTATE_AVX_MASK	(X86_XSTATE_SSE_MASK | X86_XSTATE_AVX)
>  #define X86_XSTATE_MPX_MASK	(X86_XSTATE_AVX_MASK | X86_XSTATE_MPX)
>  #define X86_XSTATE_AVX512_MASK	(X86_XSTATE_AVX_MASK | X86_XSTATE_AVX512)
> -#define X86_XSTATE_MPX_AVX512_MASK (X86_XSTATE_MPX_MASK | X86_XSTATE_AVX512)
> +#define X86_XSTATE_MPX_AVX512_MASK (X86_XSTATE_MPX_MASK | X86_XSTATE_AVX512 | X86_XSTATE_PKRU)

Looks like X86_XSTATE_MPX_AVX512_MASK ends up misnamed?

> diff --git a/gdb/features/i386/32bit-pkeys.xml b/gdb/features/i386/32bit-pkeys.xml
> new file mode 100644
> index 0000000..d4702e2
> --- /dev/null
> +++ b/gdb/features/i386/32bit-pkeys.xml
> @@ -0,0 +1,13 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2015 Free Software Foundation, Inc.

You'll need to write "2015-2016" now.  Here and all other new files
in the patch.

> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.i386.pkeys">
> +
> +  <reg name="pkru" bitsize="32" type="uint32"/>
> +
> +</feature>
> diff --git a/gdb/features/i386/64bit-pkeys.xml b/gdb/features/i386/64bit-pkeys.xml

If I'm reading right, this is exactly the same as "32bit-pkeys.xml".  Any reason
to have two files?

> @@ -951,7 +968,8 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
>    if (regclass != none)
>      {
>        /* Get `xstat_bv'.  */
> -      const gdb_byte *xstate_bv_p = XSAVE_XSTATE_BV_ADDR (regs);
> +      const unsigned long *xstate_bv_p =
> +	(unsigned long*) XSAVE_XSTATE_BV_ADDR (regs);

This ...

> @@ -1333,15 +1383,21 @@ i387_collect_xsave (const struct regcache *regcache, int regnum,
>    if ((regclass & check))
>      {
>        gdb_byte raw[I386_MAX_REGISTER_SIZE];
> -      gdb_byte *xstate_bv_p = XSAVE_XSTATE_BV_ADDR (regs);
> -      unsigned int xstate_bv = 0;
> +      unsigned long *xstate_bv_p =
> +	(unsigned long*) XSAVE_XSTATE_BV_ADDR (regs);
> +      unsigned long xstate_bv = 0;
>        /* The supported bits in `xstat_bv' are 1 byte.  */
> -      unsigned int clear_bv = (~(*xstate_bv_p)) & tdep->xcr0;
> +      unsigned long clear_bv = (~(*xstate_bv_p)) & tdep->xcr0;
>        gdb_byte *p;

... looks problematic for a host-independant file, which should work
with big endian hosts, and hosts that don't support unaligned accesses.

> +gdb_test "info register pkru" ".*pkru$test_string" "Read pkru register"
> +
> +set test_string ".*0x44444444.*"
> +gdb_test "print /x \$pkru = 0x44444444" "= 0x44444444" "Set pkru value"
> +gdb_test "info register pkru" ".*pkru$test_string" "Read value after setting value"

Please lowercase test messages.

> +
> +send_gdb "quit\n"
> 

Please remove this.

Thanks,
Pedro Alves