From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-136570-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 66541 invoked by alias); 27 Jan 2017 15:00:34 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
Received: (qmail 63584 invoked by uid 89); 27 Jan 2017 15:00:27 -0000
Authentication-Results: sourceware.org; auth=none
X-Virus-Found: No
X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:ip*10.253.24.20, H*RU:HELO, Hx-spam-relays-external:HELO, H*MI:sk:c160cf9
X-HELO: mga14.intel.com
Received: from mga14.intel.com (HELO mga14.intel.com) (192.55.52.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 27 Jan 2017 15:00:17 +0000
Received: from fmsmga006.fm.intel.com ([10.253.24.20])  by fmsmga103.fm.intel.com with ESMTP; 27 Jan 2017 07:00:12 -0800
X-ExtLoop1: 1
Received: from msturm-mobl2.ger.corp.intel.com (HELO [172.28.205.47]) ([172.28.205.47])  by fmsmga006.fm.intel.com with ESMTP; 27 Jan 2017 07:00:11 -0800
Subject: Re: [PATCH v3 5/5] Add support for Intel PKRU register to GDB and GDBserver.
To: Pedro Alves <palves@redhat.com>, mark.kettenis@xs4all.nl, eliz@gnu.org
References: <1481021894-29471-1-git-send-email-michael.sturm@intel.com> <1481021894-29471-6-git-send-email-michael.sturm@intel.com> <c160cf95-ddf6-2c62-963b-588558f6c1d2@redhat.com>
Cc: gdb-patches@sourceware.org
From: "Sturm, Michael" <michael.sturm@intel.com>
Message-ID: <588B607A.9040507@intel.com>
Date: Fri, 27 Jan 2017 15:00:00 -0000
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0
MIME-Version: 1.0
In-Reply-To: <c160cf95-ddf6-2c62-963b-588558f6c1d2@redhat.com>
Content-Type: text/plain; charset="windows-1252"; format="flowed"
Content-Transfer-Encoding: quoted-printable
X-SW-Source: 2017-01/txt/msg00626.txt.bz2

Hello Pedro,

thanks for your review. Please see comments/answers below. I'll post a=20
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=3D"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 =3D gdbarch_tdep (gdbarch);
>> +  int pkru_regnum =3D tdep->pkru_regnum;
>> +
>> +  if (pkru_regnum < 0)
>> +    return 0;
> return false;
Ok.
>
>> +
>> +  regnum -=3D pkru_regnum;
>> +  return regnum >=3D 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=20
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 registe=
r"
>> +
>> +# set test_string ".*0x44444444.*"
>> +gdb_test "print /x \$pkru =3D 0x44444444" "=3D 0x44444444" "set pkru va=
lue"
>> +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