This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH V4 4/8] MPX for amd64


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

  
@@ -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" */
  };
  
@@ -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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]