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

["Sturm, Michael" <michael dot sturm at intel dot com>]

Hello Pedro,

thanks for your review. Please see comments/answers below. I'll post a 
new revision of the patch series soon.

Thanks and Regards,
Michael

On 26/01/2017 16:38, Pedro Alves wrote:
> On 12/06/2016 10:58 AM, Michael Sturm wrote:
>
> > diff --git a/gdb/features/i386/32bit-pkeys.xml b/gdb/features/i386/32bit-pkeys.xml
> > new file mode 100644
> > index 0000000..84fff51
> > --- /dev/null
> > +++ b/gdb/features/i386/32bit-pkeys.xml
> > @@ -0,0 +1,13 @@
> > +<?xml version="1.0"?>
> > +<!-- Copyright (C) 2015-2016 Free Software Foundation, Inc.
> 2017.
Ok.
> > +
> > +/* PKRU register?  */
> > +
> > +int
> > +i386_pkru_regnum_p (struct gdbarch *gdbarch, int regnum)
> We should start using bool.
Ok.
> > +{
> > +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> > +  int pkru_regnum = tdep->pkru_regnum;
> > +
> > +  if (pkru_regnum < 0)
> > +    return 0;
> return false;
Ok.
> > +
> > +  regnum -= pkru_regnum;
> > +  return regnum >= 0 && regnum < I387_NUM_PKEYS_REGS;
> > +}
> > +
>
> > +   Copyright 2015-2016 Free Software Foundation, Inc.
> > +
> > +   Contributed by Intel Corp. <michael.sturm@intel.com>
> Please see:
>
>    https://sourceware.org/gdb/wiki/ContributionChecklist#Attribution
Ok, I'll remove these lines.
> In the test:
>
> > +#include <stdio.h>
> This is not needed, right?
It's needed to pull in the definition for NULL which is used in the call 
to __get_cpuid_
> > +#include "x86-cpuid.h"
>
>
> > +# Read values from pseudo registers.
> > +gdb_breakpoint [ gdb_get_line_number "break here 1" ]
> > +gdb_continue_to_breakpoint "break here 1" ".*break here 1.*"
> > +
> > +# set test_string ".*0x12345678.*"
> > +gdb_test "info register pkru" ".*pkru.*0x12345678.*" "read pkru register"
> > +
> > +# set test_string ".*0x44444444.*"
> > +gdb_test "print /x \$pkru = 0x44444444" "= 0x44444444" "set pkru value"
> > +gdb_test "info register pkru" ".*pkru.*0x44444444.*" "read value after setting value"
> > +
> > +gdb_breakpoint [ gdb_get_line_number "break here 2" ]
> > +gdb_continue_to_breakpoint "break here 2" ".*break here 2.*"
> > +
> > +gdb_test "print /x rd_value" ".*" "variable after reading pkru"
> Is this last test too lax?  It'll basically accept anything
> but a crash, including an error.
You're absolutely right, I'll correct this. Thanks for catching this!
> Otherwise it LGTM.
>
> Thanks,
> Pedro Alves

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928