From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21869 invoked by alias); 10 Sep 2013 14:56:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 21857 invoked by uid 89); 10 Sep 2013 14:56:31 -0000 Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Sep 2013 14:56:31 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,KHOP_THREADED,RDNS_NONE,SPF_SOFTFAIL,T_FRT_BELOW2 autolearn=no version=3.3.2 X-HELO: mga02.intel.com Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 10 Sep 2013 07:56:27 -0700 X-ExtLoop1: 1 Received: from wtedesch-mobl2.ger.corp.intel.com (HELO [172.28.205.47]) ([172.28.205.47]) by orsmga002.jf.intel.com with ESMTP; 10 Sep 2013 07:56:26 -0700 Message-ID: <522F3319.5040403@intel.com> Date: Tue, 10 Sep 2013 14:56:00 -0000 From: Walfred Tedeschi User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 MIME-Version: 1.0 To: gdb-patches@sourceware.org, Mark Kettenis Subject: Re: [PATCH V4 4/8] MPX for amd64 References: <1378373188-31144-1-git-send-email-walfred.tedeschi@intel.com> <1378373188-31144-5-git-send-email-walfred.tedeschi@intel.com> <201309101248.r8ACmI7N030367@glazunov.sibelius.xs4all.nl> In-Reply-To: <201309101248.r8ACmI7N030367@glazunov.sibelius.xs4all.nl> Content-Type: text/plain; charset="iso-8859-15"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00337.txt.bz2 Hi Mark, Thanks a lot for your review! Please see my comments bellow. Best regards, -Fred Am 9/10/2013 2:48 PM, schrieb Mark Kettenis: >> From: Walfred Tedeschi >> Date: Thu, 5 Sep 2013 11:26:24 +0200 >> >> 2013-06-24 Walfred Tedeschi >> >> * amd64-linux-nat.c (amd64_linux_gregset32_reg_offset): >> Add MPX registers. >> (amd64_linux_read_description): Add initialization for MPX and >> AVX independently. >> * amd64-linux-tdep.c: Includes features/i386/amd64-mpx-linux.c and >> features/i386/x32-mpx-linux.c. >> (amd64_linux_gregset_reg_offset): Add MPX registers. >> (amd64_linux_core_read_description): Add initialization for MPX >> registers. >> (_initialize_amd64_linux_tdep): Initialize MPX targets. >> * amd64-linux-tdep.h (AMD64_LINUX_NUM_REGS): Set it to the last >> register on the list. >> (tdesc_amd64_mpx_linux and tdesc_x32_mpx_linux) Add new targets >> for MPX. >> * amd64-tdep.c: Includes features/i386/amd64-mpx.c and >> features/i386/x32-mpx.c. >> (amd64_mpx_names): MPX register names. >> (amd64_init_abi): Add MPX register while initializing the ABI. >> (_initialize_amd64_tdep): Initialize MPX targets. >> * amd64-tdep.h (amd64_regnum): Add MPX registers. >> (AMD64_NUM_REGS): Set number of registers taking MPX into account. >> >> --- >> gdb/amd64-linux-nat.c | 43 ++++++++++++++++++++++++++++++++++------= --- >> gdb/amd64-linux-tdep.c | 14 +++++++++++++- >> gdb/amd64-linux-tdep.h | 6 ++++-- >> gdb/amd64-tdep.c | 18 ++++++++++++++++++ >> gdb/amd64-tdep.h | 8 ++++++-- >> 5 files changed, 75 insertions(+), 14 deletions(-) >> >> diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c >> index 8dfe7c5..a230dca 100644 >> --- a/gdb/amd64-linux-nat.c >> +++ b/gdb/amd64-linux-nat.c >> @@ -100,7 +100,9 @@ static int amd64_linux_gregset32_reg_offset[] =3D >> -1, -1, -1, -1, -1, -1, -1, -1, >> -1, -1, -1, -1, -1, -1, -1, -1, -1, >> -1, -1, -1, -1, -1, -1, -1, -1, >> - ORIG_RAX * 8 /* "orig_eax" */ >> + ORIG_RAX * 8, /* "orig_eax" */ >> + -1, -1, -1, -1, /* MPX registers BND0 ... BND3. */ >> + -1, -1 /* MPX registers BNDCFGU, BNDSTATUS. */ >> }; > This looks wrong to me now that you've kept "orig_eax" at the end. I changed this in version V5 already to review on the list. http://sourceware.org/ml/gdb-patches/2013-09/msg00286.html > >> =0C >>=20=20=20 >> @@ -1094,18 +1096,41 @@ amd64_linux_read_description (struct target_ops = *ops) >> } >>=20=20=20 >> /* Check the native XCR0 only if PTRACE_GETREGSET is available. */ >> - if (have_ptrace_getregset >> - && (xcr0 & I386_XSTATE_AVX_MASK) =3D=3D I386_XSTATE_AVX_MASK) >> + if (have_ptrace_getregset && (xcr0 & I386_XSTATE_ALL_MASK)) >> { >> - if (is_64bit) >> + switch (xcr0 & I386_XSTATE_ALL_MASK) >> { >> - if (is_x32) >> - return tdesc_x32_avx_linux; >> + case I386_XSTATE_MPX_MASK: >> + if (is_64bit) >> + { >> + if (is_x32) >> + return tdesc_x32_mpx_linux; >> + else >> + return tdesc_amd64_mpx_linux; >> + } >> + else >> + return tdesc_i386_mpx_linux; >> + case I386_XSTATE_AVX_MASK: >> + if (is_64bit) >> + { >> + if (is_x32) >> + return tdesc_x32_avx_linux; >> + else >> + return tdesc_amd64_avx_linux; >> + } >> + else >> + return tdesc_i386_avx_linux; >> + default: >> + if (is_64bit) >> + { >> + if (is_x32) >> + return tdesc_x32_linux; >> + else >> + return tdesc_amd64_linux; >> + } >> else >> - return tdesc_amd64_avx_linux; >> + return tdesc_i386_linux; >> } >> - else >> - return tdesc_i386_avx_linux; >> } >> else >> { >> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c >> index 4f67762..9740aff 100644 >> --- a/gdb/amd64-linux-tdep.c >> +++ b/gdb/amd64-linux-tdep.c >> @@ -42,8 +42,10 @@ >>=20=20=20 >> #include "features/i386/amd64-linux.c" >> #include "features/i386/amd64-avx-linux.c" >> +#include "features/i386/amd64-mpx-linux.c" >> #include "features/i386/x32-linux.c" >> #include "features/i386/x32-avx-linux.c" >> +#include "features/i386/x32-mpx-linux.c" >>=20=20=20 >> /* The syscall's XML filename for i386. */ >> #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml" >> @@ -96,6 +98,8 @@ int amd64_linux_gregset_reg_offset[] =3D >> -1, -1, -1, -1, -1, -1, -1, -1, -1, >> -1, -1, -1, -1, -1, -1, -1, -1, >> -1, -1, -1, -1, -1, -1, -1, -1, >> + -1, -1, -1, -1, /* MPX registers BND0 ... BND3. */ >> + -1, -1, /* MPX registers BNDCFGU and BNDSTATUS. */ >> 15 * 8 /* "orig_rax" */ >> }; >> =0C >> @@ -1287,8 +1291,14 @@ amd64_linux_core_read_description (struct gdbarch= *gdbarch, >> { >> /* Linux/x86-64. */ >> uint64_t xcr0 =3D i386_linux_core_read_xcr0 (abfd); >> - switch ((xcr0 & I386_XSTATE_AVX_MASK)) >> + >> + switch (xcr0 & I386_XSTATE_ALL_MASK) >> { >> + case I386_XSTATE_MPX_MASK: >> + if (gdbarch_ptr_bit (gdbarch) =3D=3D 32) >> + return tdesc_x32_mpx_linux; >> + else >> + return tdesc_amd64_mpx_linux; >> case I386_XSTATE_AVX_MASK: >> if (gdbarch_ptr_bit (gdbarch) =3D=3D 32) >> return tdesc_x32_avx_linux; >> @@ -1623,6 +1633,8 @@ _initialize_amd64_linux_tdep (void) >> /* Initialize the Linux target description. */ >> initialize_tdesc_amd64_linux (); >> initialize_tdesc_amd64_avx_linux (); >> + initialize_tdesc_amd64_mpx_linux (); >> initialize_tdesc_x32_linux (); >> initialize_tdesc_x32_avx_linux (); >> + initialize_tdesc_x32_mpx_linux (); >> } >> diff --git a/gdb/amd64-linux-tdep.h b/gdb/amd64-linux-tdep.h >> index e9eaeaa..ec10b7c 100644 >> --- a/gdb/amd64-linux-tdep.h >> +++ b/gdb/amd64-linux-tdep.h >> @@ -26,16 +26,18 @@ >> /* Register number for the "orig_rax" register. If this register >> contains a value >=3D 0 it is interpreted as the system call number >> that the kernel is supposed to restart. */ >> -#define AMD64_LINUX_ORIG_RAX_REGNUM (AMD64_YMM15H_REGNUM + 1) >> +#define AMD64_LINUX_ORIG_RAX_REGNUM (AMD64_BNDSTATUS_REGNUM + 1) >>=20=20=20 >> /* Total number of registers for GNU/Linux. */ >> -#define AMD64_LINUX_NUM_REGS (AMD64_LINUX_ORIG_RAX_REGNUM + 1) >> +#define AMD64_LINUX_NUM_REGS (AMD64_BNDSTATUS_REGNUM + 2) >>=20=20=20 >> /* Linux target description. */ >> extern struct target_desc *tdesc_amd64_linux; >> extern struct target_desc *tdesc_amd64_avx_linux; >> +extern struct target_desc *tdesc_amd64_mpx_linux; >> extern struct target_desc *tdesc_x32_linux; >> extern struct target_desc *tdesc_x32_avx_linux; >> +extern struct target_desc *tdesc_x32_mpx_linux; >>=20=20=20 >> /* Enum that defines the syscall identifiers for amd64 linux. >> Used for process record/replay, these will be translated into >> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c >> index 3ab74f0..f0654ba 100644 >> --- a/gdb/amd64-tdep.c >> +++ b/gdb/amd64-tdep.c >> @@ -43,8 +43,10 @@ >>=20=20=20 >> #include "features/i386/amd64.c" >> #include "features/i386/amd64-avx.c" >> +#include "features/i386/amd64-mpx.c" >> #include "features/i386/x32.c" >> #include "features/i386/x32-avx.c" >> +#include "features/i386/x32-mpx.c" >>=20=20=20 >> #include "ax.h" >> #include "ax-gdb.h" >> @@ -92,6 +94,11 @@ static const char *amd64_ymmh_names[] =3D >> "ymm12h", "ymm13h", "ymm14h", "ymm15h" >> }; >>=20=20=20 >> +static const char *amd64_mpx_names[] =3D >> +{ >> + "bnd0raw", "bnd1raw", "bnd2raw", "bnd3raw", "bndcfgu", "bndstatus" >> +}; > I'm not convinced the way you're treating the bounds registers makes > all that much sense. I think bnd0-bnd3 should contain the "raw" > 128-bit value. But I'm a low-level guy. In fact we would like to have both views low level and application view. All the designs we had (not using python) lead to changes on GDB type=20 system. (we considered it too intrusive). Asside of that we would like to display the value as addresses to easy=20 the use. >> + >> /* The registers used to pass integer arguments during a function call= . */ >> static int amd64_dummy_call_integer_regs[] =3D >> { >> @@ -2870,6 +2877,15 @@ amd64_init_abi (struct gdbarch_info info, struct = gdbarch *gdbarch) >> tdep->ymm0h_regnum =3D AMD64_YMM0H_REGNUM; >> } >>=20=20=20 >> + if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.mpx") !=3D NULL) >> + { >> + tdep->mpx_register_names =3D amd64_mpx_names; >> + tdep->num_mpx_regs =3D AMD64_BNDSTATUS_REGNUM - AMD64_BND0R_REGNU= M + 1; >> + tdep->bndcfgu_regnum =3D AMD64_BNDCFGU_REGNUM; >> + tdep->bnd0r_regnum =3D AMD64_BND0R_REGNUM; >> + tdep->num_bnd_regs =3D 4; >> + } > Is the number of bounds registers going to change between 32-bit and > 64-bit mode? Otherwise, I think you could (and should) reduce the > number of new "struct gdbarch_tdep" members. In fact they do not change. I will change that. Thanks. Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052