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

Pedro Alves
2016-01-20T16:05:15Z

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. -->
> +
> +
> +<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