From: Walfred Tedeschi <walfred.tedeschi@intel.com>
To: gdb-patches@sourceware.org, Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: [PATCH V4 4/8] MPX for amd64
Date: Tue, 10 Sep 2013 14:56:00 -0000 [thread overview]
Message-ID: <522F3319.5040403@intel.com> (raw)
In-Reply-To: <201309101248.r8ACmI7N030367@glazunov.sibelius.xs4all.nl>
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 <walfred.tedeschi@intel.com>
>> Date: Thu, 5 Sep 2013 11:26:24 +0200
>>
>> 2013-06-24 Walfred Tedeschi <walfred.tedeschi@intel.com>
>>
>> * 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[] =
>> -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
>
>> \f
>>
>> @@ -1094,18 +1096,41 @@ amd64_linux_read_description (struct target_ops *ops)
>> }
>>
>> /* Check the native XCR0 only if PTRACE_GETREGSET is available. */
>> - if (have_ptrace_getregset
>> - && (xcr0 & I386_XSTATE_AVX_MASK) == 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 @@
>>
>> #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"
>>
>> /* 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[] =
>> -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" */
>> };
>> \f
>> @@ -1287,8 +1291,14 @@ amd64_linux_core_read_description (struct gdbarch *gdbarch,
>> {
>> /* Linux/x86-64. */
>> uint64_t xcr0 = 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) == 32)
>> + return tdesc_x32_mpx_linux;
>> + else
>> + return tdesc_amd64_mpx_linux;
>> case I386_XSTATE_AVX_MASK:
>> if (gdbarch_ptr_bit (gdbarch) == 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 >= 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)
>>
>> /* 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)
>>
>> /* 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;
>>
>> /* 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 @@
>>
>> #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"
>>
>> #include "ax.h"
>> #include "ax-gdb.h"
>> @@ -92,6 +94,11 @@ static const char *amd64_ymmh_names[] =
>> "ymm12h", "ymm13h", "ymm14h", "ymm15h"
>> };
>>
>> +static const char *amd64_mpx_names[] =
>> +{
>> + "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
system. (we considered it too intrusive).
Asside of that we would like to display the value as addresses to easy
the use.
>> +
>> /* The registers used to pass integer arguments during a function call. */
>> static int amd64_dummy_call_integer_regs[] =
>> {
>> @@ -2870,6 +2877,15 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>> tdep->ymm0h_regnum = AMD64_YMM0H_REGNUM;
>> }
>>
>> + if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.mpx") != NULL)
>> + {
>> + tdep->mpx_register_names = amd64_mpx_names;
>> + tdep->num_mpx_regs = AMD64_BNDSTATUS_REGNUM - AMD64_BND0R_REGNUM + 1;
>> + tdep->bndcfgu_regnum = AMD64_BNDCFGU_REGNUM;
>> + tdep->bnd0r_regnum = AMD64_BND0R_REGNUM;
>> + tdep->num_bnd_regs = 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
next prev parent reply other threads:[~2013-09-10 14:56 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-05 9:26 [PATCH V4 0/8] Intel(R) MPX register support Walfred Tedeschi
2013-09-05 9:26 ` [PATCH V4 3/8] Add MPX support for i386 Walfred Tedeschi
2013-09-05 9:40 ` Eli Zaretskii
2013-09-06 7:36 ` Tedeschi, Walfred
2013-09-05 9:26 ` [PATCH V4 4/8] MPX for amd64 Walfred Tedeschi
2013-09-10 12:48 ` Mark Kettenis
2013-09-10 14:56 ` Walfred Tedeschi [this message]
2013-09-12 8:53 ` Tedeschi, Walfred
2013-09-05 9:26 ` [PATCH V4 6/8] Add pretty-printer for MPX bnd registers Walfred Tedeschi
2013-09-05 9:26 ` [PATCH V4 5/8] Add MPX support to gdbserver Walfred Tedeschi
2013-09-05 9:26 ` [PATCH V4 8/8] Add MPX feature description to GDB manual Walfred Tedeschi
2013-09-05 9:37 ` Eli Zaretskii
2013-09-05 9:26 ` [PATCH V4 7/8] Add MPX registers tests Walfred Tedeschi
2013-09-05 9:26 ` [PATCH V4 1/8] Fix conditions in creating a bitfield Walfred Tedeschi
2013-09-05 9:26 ` [PATCH V4 2/8] Add MPX registers XML files Walfred Tedeschi
2013-09-05 9:41 ` [PATCH V4 0/8] Intel(R) MPX register support Eli Zaretskii
[not found] ` <AC542571535E904D8E8ADAE745D60B191B18DB69@IRSMSX104.ger.corp.intel.com>
2013-09-09 16:32 ` Eli Zaretskii
2013-09-25 11:51 ` Tedeschi, Walfred
2013-09-25 12:33 ` Eli Zaretskii
2013-09-25 12:41 ` Tedeschi, Walfred
2013-09-25 15:37 ` Eli Zaretskii
2013-09-25 12:47 ` Mark Kettenis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=522F3319.5040403@intel.com \
--to=walfred.tedeschi@intel.com \
--cc=gdb-patches@sourceware.org \
--cc=mark.kettenis@xs4all.nl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox