[PATCH V3 0/2] MPX ABI adaptations.

[PATCH V3 0/2] MPX ABI adaptations.

[Walfred Tedeschi]

Full patch is divided into two parts:

1. Add new POINTER class to the amd64 ABI.
2. Add new command to control the set to
the INIT states when using the "return" command.

Difference from previous version:
1. Add NEWS entry.
2. General indentation and comments improvements.
3. Command set/show is now initialized during
target initialization together with MPX map commands.


Walfred Tedeschi (2):
  ABI changes for MPX.
  Add mpx-bnd-init-on-return set/show command for inferior calls.

 gdb/NEWS                                  |   4 +
 gdb/amd64-tdep.c                          |  76 +++++++++++++++---
 gdb/doc/gdb.texinfo                       |  20 +++++
 gdb/i386-tdep.c                           |  27 +++++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 126 ++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  90 +++++++++++++++++++++
 6 files changed, 334 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp

-- 
2.1.4

[PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

[Walfred Tedeschi]

When using the return command, execution of a function is aborted
and present values are returned from that point.  That can cause
bound violations in the MPX context.  To avoid such side-effects
a new set variable was added "mpx-bnd-init-on-return" which controls
the initialization of bound register when using the return command.

As bound initialization it is understood the set of the BND register
to zero allowing the associated pointer to access the whole memory.

As default the value of "mpx-bnd-init-on-return" is set to 1.  So
bound register are initilized when using the "return" command.

2016-01-13  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* i386-tdep.c (mpx_bnd_init_on_return): new external variable.
	(show_mpx_init_on_return): New function.
	(_initialize_i386_tdepamd64_init_abi): Add new commands set/show
	mpx-bnd-init-on-return.
	* amd64-tdep.c (amd64_return_value): Use mpx-bnd-init-on-return.
	* NEWS: Add entry for set/show mpx-bnd-init-on-return command.

gdb/doc/ChangeLog:

	* gdb.texinfo (Intel Memory Protection Extensions): Add
	documentation on using set/show mpx-bnd-init-on-return.

gdb/testsuite/ChangeLog:

	* amd64-mpx-call.c: New file.
	* amd64-mpx-call.exp: New file.

---
 gdb/NEWS                                  |   4 +
 gdb/amd64-tdep.c                          |  32 ++++++++
 gdb/doc/gdb.texinfo                       |  12 +++
 gdb/i386-tdep.c                           |  27 +++++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 126 ++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  90 +++++++++++++++++++++
 6 files changed, 291 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index a222dfb..80022ec 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,10 @@
 
 *** Changes since GDB 7.10
 
+* show mpx-bnd-init-on-return
+  set mpx-bnd-init-on-return on i386 and amd64
+   Support for set bound registers to INIT state when using the command return.
+
 * Record btrace now supports non-stop mode.
 
 * Support for tracepoints on aarch64-linux was added in GDBserver.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 732e91b..0ce41d1 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -708,12 +708,14 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 		    gdb_byte *readbuf, const gdb_byte *writebuf)
 {
   enum amd64_reg_class theclass[2];
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   int len = TYPE_LENGTH (type);
   static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM };
   static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM };
   int integer_reg = 0;
   int sse_reg = 0;
   int i;
+  extern int mpx_bnd_init_on_return;
 
   gdb_assert (!(readbuf && writebuf));
 
@@ -739,6 +741,25 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 
 	  regcache_raw_read_unsigned (regcache, AMD64_RAX_REGNUM, &addr);
 	  read_memory (addr, readbuf, TYPE_LENGTH (type));
+
+	  /* In MPX-enabled programs, if the class is MEMORY, boundary is
+	     expected to be stored in bnd0 register.
+
+	     if the return value we are setting is due to the use of the
+	     "return" command (WRITEBUF is not NULL), it is likely that
+	     the return is made prematurely, and therefore bnd0 register
+	     unset could then cause the program to receive a spurious
+	     bound violation, but this is only done if
+	     "set mpx-bnd-init-on-return" is true.  */
+
+	  if (writebuf != NULL
+	      && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
+	    {
+	      gdb_byte bnd_buf[16];
+
+	      memset (bnd_buf, 0, 16);
+	      regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep), bnd_buf);
+	    }
 	}
 
       return RETURN_VALUE_ABI_RETURNS_ADDRESS;
@@ -833,6 +854,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 				 writebuf + i * 8);
     }
 
+  if (I387_BND0R_REGNUM (tdep) > 0)
+    {
+      gdb_byte bnd_buf[16];
+      int i, init_bnd;
+
+      memset (bnd_buf, 0, 16);
+      if (writebuf && mpx_bnd_init_on_return)
+	for (i = 0; i < I387_NUM_BND_REGS; i++)
+	  regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
+    }
+
   return RETURN_VALUE_REGISTER_CONVENTION;
 }
 \f
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ccde84a..27020ee 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22072,6 +22072,18 @@ can access the whole memory, in this case the register bound register
 associated to it has value 0, e.g. if the register associated is bnd0raw
 its value will be @{0x0, 0x0@}.
 
+While the using the @command{return} bounds can propagate through
+execution causing a boundary violation.
+The behaviour of initializing bounds when using @command{return}
+can be controlled and vizualized via the following commands:
+@table @code
+@kindex set mpx-bnd-init-on-return
+When set to true bound registers will be set to the INIT state when
+using the "return" command.
+@kindex show mpx-bnd-init-on-return
+Show the state of mpx-bnd-init-on-return.
+@end table
+
 @node Alpha
 @subsection Alpha
 
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index ebb21fc..4c3fa02 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8864,6 +8864,24 @@ i386_mpx_set_bounds (char *args, int from_tty)
 				   bt_entry[i]);
 }
 
+/* The value of the "set mpx-bnd-init-on-return setting.  */
+
+int mpx_bnd_init_on_return = 1;
+
+/*  Show state of the setting variable mpx-bnd-init-on-return.  */
+
+static void
+show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty,
+			     struct cmd_list_element *c, const char *value)
+{
+  if (mpx_bnd_init_on_return)
+    fprintf_filtered (file,
+		      _("BND registers will be initialized on return.\n"));
+  else
+    fprintf_filtered (file,
+		      _("BND registers will not be initialized on return.\n"));
+}
+
 static struct cmd_list_element *mpx_set_cmdlist, *mpx_show_cmdlist;
 
 /* Helper function for the CLI commands.  */
@@ -8940,6 +8958,15 @@ Show Intel(R) Memory Protection Extensions specific variables."),
  in the bound table.",
 	   &mpx_set_cmdlist);
 
+  add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class,
+			   &mpx_bnd_init_on_return, _("\
+ Set the bnd registers to INIT state when returning from a call."), _("\
+ Show the state of the mpx-bnd-init-on-return."),
+			   NULL,
+			   NULL,
+			   show_mpx_bnd_init_on_return,
+			   &setlist, &showlist);
+
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_coff_flavour,
 				  i386_coff_osabi_sniffer);
 
diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.c b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
new file mode 100644
index 0000000..d05b8a8
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
@@ -0,0 +1,126 @@
+/* Test for inferior function calls MPX context.
+
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp.  <walfred.tedeschi@intel.com>
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include "x86-cpuid.h"
+
+#define OUR_SIZE    5
+
+int gx[OUR_SIZE];
+int ga[OUR_SIZE];
+int gb[OUR_SIZE];
+int gc[OUR_SIZE];
+int gd[OUR_SIZE];
+
+unsigned int
+have_mpx (void)
+{
+  unsigned int eax, ebx, ecx, edx;
+
+  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
+    return 0;
+
+  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
+    {
+      if (__get_cpuid_max (0, NULL) < 7)
+	return 0;
+
+      __cpuid_count (7, 0, eax, ebx, ecx, edx);
+
+      if ((ebx & bit_MPX) == bit_MPX)
+	return 1;
+      else
+	return 0;
+    }
+  return 0;
+}
+
+int
+bp1 (int value)
+{
+  return 1;
+}
+
+int
+bp2 (int value)
+{
+  return 1;
+}
+
+int
+upper (int *p, int *a, int *b, int *c, int *d, int len)
+{
+  int value;
+
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);
+  value = *(d + len);
+
+  value = value - value + 1;
+  return value;
+}
+
+int *
+upper_ptr (int *p, int *a, int *b, int *c, int *d, int len)
+{
+  int value;
+
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);  /* bkpt 2.  */
+  value = *(d + len);  /* bkpt 3.  */
+  free (p);
+  p = calloc (OUR_SIZE * 2, sizeof (int));
+
+  return ++p;
+}
+
+
+int
+main (void)
+{
+  if (have_mpx ())
+    {
+      int sx[OUR_SIZE];
+      int sa[OUR_SIZE];
+      int sb[OUR_SIZE];
+      int sc[OUR_SIZE];
+      int sd[OUR_SIZE];
+      int *x, *a, *b, *c, *d;
+
+      x = calloc (OUR_SIZE, sizeof (int));
+      a = calloc (OUR_SIZE, sizeof (int));
+      b = calloc (OUR_SIZE, sizeof (int));
+      c = calloc (OUR_SIZE, sizeof (int));
+      d = calloc (OUR_SIZE, sizeof (int));
+
+      upper (sx, sa, sb, sc, sd, 0);  /* bkpt 1.  */
+      x = upper_ptr (sx, sa, sb, sc, sd, 0);
+
+      free (x);
+      free (a);
+      free (b);
+      free (c);
+      free (d);
+    }
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.exp b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
new file mode 100644
index 0000000..d9c6eba
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
@@ -0,0 +1,90 @@
+# Copyright (C) 2016 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
+    verbose "Skipping x86 MPX tests."
+    return
+}
+
+standard_testfile
+
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
+
+if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+    [list debug nowarnings additional_flags=${comp_flags}]] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test_multiple "print have_mpx ()" "have mpx" {
+    -re ".*= 1\r\n$gdb_prompt " {
+        pass "check whether processor supports MPX"
+    }
+    -re ".*= 0\r\n$gdb_prompt " {
+        verbose "processor does not support MPX; skipping MPX tests"
+        return
+    }
+}
+
+# Needed by the return command.
+gdb_test_no_output "set confirm off"
+
+set break "bkpt 1."
+gdb_breakpoint [gdb_get_line_number "${break}"]
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+gdb_test "p upper (x, a, b, c, d, 0)" "= 1"\
+    "test the call of int function - int"
+gdb_test "p upper_ptr (x, a, b, c, d, 0)"\
+    "= \\\(int \\\*\\\) $hex" "test the call of function - ptr"
+
+set break "bkpt 2."
+gdb_breakpoint [gdb_get_line_number "${break}"]
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
+    bnd0 result in a convenience variable 1"
+
+gdb_test "return a" "#0  $hex in main.*" "First return"
+gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
+    "= 0" "return with bnd initialization off - ubound"
+gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
+    "= 0" "return with bnd initialization off - lbound"
+
+# Retesting with initialization off. 
+# All breakpoints were deleted need to recreate what we need.
+runto_main
+gdb_test_no_output "set mpx-bnd-init-on-return off"\
+    "Turn off initialization on return"
+
+set break "bkpt 3."
+gdb_breakpoint [gdb_get_line_number "${break}"]
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
+    bnd0 result in a convenience variable 2"
+gdb_test "return a" "#0  $hex in main.*" "Second return"
+gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
+    "= 1" "return with bnd initialization on - ubound"
+gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
+    "= 1" "return with bnd initialization on - lbound"
+
+gdb_test "show mpx-bnd-init-on-return"\
+    "BND registers will not be initialized on return\\."\
+    "double check initialization on return"
-- 
2.1.4

Re: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

[Eli Zaretskii]

> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Cc: gdb-patches@sourceware.org, Walfred Tedeschi <walfred.tedeschi@intel.com>
> Date: Wed, 13 Jan 2016 13:39:59 +0100
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 7.10
>  
> +* show mpx-bnd-init-on-return
> +  set mpx-bnd-init-on-return on i386 and amd64
> +   Support for set bound registers to INIT state when using the command return.

Please quote "return" in the last sentence.

> +While the using the @command{return} bounds can propagate through
         ^^^
This "the" should be deleted.  Also, @command is for external
commands, not GDB commands.

> +execution causing a boundary violation.

Hmm... "propagate through execution" is not really clear.  How about
simplifying this, like below?

  When you use the @code{return} command, the bound registers might
  cause boundary violations because they were not updated for the
  early return from the function.

> +The behaviour of initializing bounds when using @command{return}
> +can be controlled and vizualized via the following commands:

I think we should explain how initializing these registers comes into
play.  Like this, for example:

  To countermand that, @value{GDBN} can force initialization of the
  bound registers when it performs the @code{return} command.  This is
  controlled by the following option:

The documentation parts are OK with these fixes.

Thanks.

RE: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

[Tedeschi, Walfred]

Eli,

I will add those fixes. Thanks for your review!

Best regards,
-Fred

-----Original Message-----
From: Eli Zaretskii [mailto:eliz@gnu.org] 
Sent: Wednesday, January 13, 2016 5:10 PM
To: Tedeschi, Walfred
Cc: brobecker@adacore.com; gdb-patches@sourceware.org
Subject: Re: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Cc: gdb-patches@sourceware.org, Walfred Tedeschi 
> <walfred.tedeschi@intel.com>
> Date: Wed, 13 Jan 2016 13:39:59 +0100
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 7.10
>  
> +* show mpx-bnd-init-on-return
> +  set mpx-bnd-init-on-return on i386 and amd64
> +   Support for set bound registers to INIT state when using the command return.

Please quote "return" in the last sentence.

> +While the using the @command{return} bounds can propagate through
         ^^^
This "the" should be deleted.  Also, @command is for external commands, not GDB commands.

> +execution causing a boundary violation.

Hmm... "propagate through execution" is not really clear.  How about simplifying this, like below?

  When you use the @code{return} command, the bound registers might
  cause boundary violations because they were not updated for the
  early return from the function.

> +The behaviour of initializing bounds when using @command{return} can 
> +be controlled and vizualized via the following commands:

I think we should explain how initializing these registers comes into play.  Like this, for example:

  To countermand that, @value{GDBN} can force initialization of the
  bound registers when it performs the @code{return} command.  This is
  controlled by the following option:

The documentation parts are OK with these fixes.

Thanks.
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

RE: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

[Tedeschi, Walfred]

Joel,

I know that we are under release right now. 
But, in any case, have you had the chance to take a look on this patch?
Should we wait post release for seeing this one?

Also this one:
https://sourceware.org/ml/gdb-patches/2016-01/msg00254.html


Thanks and regards,
-Fred

-----Original Message-----
From: Tedeschi, Walfred 
Sent: Wednesday, January 13, 2016 1:40 PM
To: brobecker@adacore.com; eliz@gnu.org
Cc: gdb-patches@sourceware.org; Tedeschi, Walfred
Subject: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

When using the return command, execution of a function is aborted and present values are returned from that point.  That can cause bound violations in the MPX context.  To avoid such side-effects a new set variable was added "mpx-bnd-init-on-return" which controls the initialization of bound register when using the return command.

As bound initialization it is understood the set of the BND register to zero allowing the associated pointer to access the whole memory.

As default the value of "mpx-bnd-init-on-return" is set to 1.  So bound register are initilized when using the "return" command.

2016-01-13  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* i386-tdep.c (mpx_bnd_init_on_return): new external variable.
	(show_mpx_init_on_return): New function.
	(_initialize_i386_tdepamd64_init_abi): Add new commands set/show
	mpx-bnd-init-on-return.
	* amd64-tdep.c (amd64_return_value): Use mpx-bnd-init-on-return.
	* NEWS: Add entry for set/show mpx-bnd-init-on-return command.

gdb/doc/ChangeLog:

	* gdb.texinfo (Intel Memory Protection Extensions): Add
	documentation on using set/show mpx-bnd-init-on-return.

gdb/testsuite/ChangeLog:

	* amd64-mpx-call.c: New file.
	* amd64-mpx-call.exp: New file.

---
 gdb/NEWS                                  |   4 +
 gdb/amd64-tdep.c                          |  32 ++++++++
 gdb/doc/gdb.texinfo                       |  12 +++
 gdb/i386-tdep.c                           |  27 +++++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 126 ++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  90 +++++++++++++++++++++
 6 files changed, 291 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index a222dfb..80022ec 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,10 @@
 
 *** Changes since GDB 7.10
 
+* show mpx-bnd-init-on-return
+  set mpx-bnd-init-on-return on i386 and amd64
+   Support for set bound registers to INIT state when using the command return.
+
 * Record btrace now supports non-stop mode.
 
 * Support for tracepoints on aarch64-linux was added in GDBserver.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c index 732e91b..0ce41d1 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -708,12 +708,14 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 		    gdb_byte *readbuf, const gdb_byte *writebuf)  {
   enum amd64_reg_class theclass[2];
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   int len = TYPE_LENGTH (type);
   static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM };
   static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM };
   int integer_reg = 0;
   int sse_reg = 0;
   int i;
+  extern int mpx_bnd_init_on_return;
 
   gdb_assert (!(readbuf && writebuf));
 
@@ -739,6 +741,25 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 
 	  regcache_raw_read_unsigned (regcache, AMD64_RAX_REGNUM, &addr);
 	  read_memory (addr, readbuf, TYPE_LENGTH (type));
+
+	  /* In MPX-enabled programs, if the class is MEMORY, boundary is
+	     expected to be stored in bnd0 register.
+
+	     if the return value we are setting is due to the use of the
+	     "return" command (WRITEBUF is not NULL), it is likely that
+	     the return is made prematurely, and therefore bnd0 register
+	     unset could then cause the program to receive a spurious
+	     bound violation, but this is only done if
+	     "set mpx-bnd-init-on-return" is true.  */
+
+	  if (writebuf != NULL
+	      && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
+	    {
+	      gdb_byte bnd_buf[16];
+
+	      memset (bnd_buf, 0, 16);
+	      regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep), bnd_buf);
+	    }
 	}
 
       return RETURN_VALUE_ABI_RETURNS_ADDRESS; @@ -833,6 +854,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 				 writebuf + i * 8);
     }
 
+  if (I387_BND0R_REGNUM (tdep) > 0)
+    {
+      gdb_byte bnd_buf[16];
+      int i, init_bnd;
+
+      memset (bnd_buf, 0, 16);
+      if (writebuf && mpx_bnd_init_on_return)
+	for (i = 0; i < I387_NUM_BND_REGS; i++)
+	  regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
+    }
+
   return RETURN_VALUE_REGISTER_CONVENTION;  }
 

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index ccde84a..27020ee 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22072,6 +22072,18 @@ can access the whole memory, in this case the register bound register  associated to it has value 0, e.g. if the register associated is bnd0raw  its value will be @{0x0, 0x0@}.
 
+While the using the @command{return} bounds can propagate through 
+execution causing a boundary violation.
+The behaviour of initializing bounds when using @command{return} can be 
+controlled and vizualized via the following commands:
+@table @code
+@kindex set mpx-bnd-init-on-return
+When set to true bound registers will be set to the INIT state when 
+using the "return" command.
+@kindex show mpx-bnd-init-on-return
+Show the state of mpx-bnd-init-on-return.
+@end table
+
 @node Alpha
 @subsection Alpha
 
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index ebb21fc..4c3fa02 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8864,6 +8864,24 @@ i386_mpx_set_bounds (char *args, int from_tty)
 				   bt_entry[i]);
 }
 
+/* The value of the "set mpx-bnd-init-on-return setting.  */
+
+int mpx_bnd_init_on_return = 1;
+
+/*  Show state of the setting variable mpx-bnd-init-on-return.  */
+
+static void
+show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty,
+			     struct cmd_list_element *c, const char *value) {
+  if (mpx_bnd_init_on_return)
+    fprintf_filtered (file,
+		      _("BND registers will be initialized on return.\n"));
+  else
+    fprintf_filtered (file,
+		      _("BND registers will not be initialized on return.\n")); }
+
 static struct cmd_list_element *mpx_set_cmdlist, *mpx_show_cmdlist;
 
 /* Helper function for the CLI commands.  */ @@ -8940,6 +8958,15 @@ Show Intel(R) Memory Protection Extensions specific variables."),
  in the bound table.",
 	   &mpx_set_cmdlist);
 
+  add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class,
+			   &mpx_bnd_init_on_return, _("\
+ Set the bnd registers to INIT state when returning from a call."), 
+_("\  Show the state of the mpx-bnd-init-on-return."),
+			   NULL,
+			   NULL,
+			   show_mpx_bnd_init_on_return,
+			   &setlist, &showlist);
+
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_coff_flavour,
 				  i386_coff_osabi_sniffer);
 
diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.c b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
new file mode 100644
index 0000000..d05b8a8
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
@@ -0,0 +1,126 @@
+/* Test for inferior function calls MPX context.
+
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp.  <walfred.tedeschi@intel.com>
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see 
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include "x86-cpuid.h"
+
+#define OUR_SIZE    5
+
+int gx[OUR_SIZE];
+int ga[OUR_SIZE];
+int gb[OUR_SIZE];
+int gc[OUR_SIZE];
+int gd[OUR_SIZE];
+
+unsigned int
+have_mpx (void)
+{
+  unsigned int eax, ebx, ecx, edx;
+
+  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
+    return 0;
+
+  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
+    {
+      if (__get_cpuid_max (0, NULL) < 7)
+	return 0;
+
+      __cpuid_count (7, 0, eax, ebx, ecx, edx);
+
+      if ((ebx & bit_MPX) == bit_MPX)
+	return 1;
+      else
+	return 0;
+    }
+  return 0;
+}
+
+int
+bp1 (int value)
+{
+  return 1;
+}
+
+int
+bp2 (int value)
+{
+  return 1;
+}
+
+int
+upper (int *p, int *a, int *b, int *c, int *d, int len) {
+  int value;
+
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);
+  value = *(d + len);
+
+  value = value - value + 1;
+  return value;
+}
+
+int *
+upper_ptr (int *p, int *a, int *b, int *c, int *d, int len) {
+  int value;
+
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);  /* bkpt 2.  */
+  value = *(d + len);  /* bkpt 3.  */
+  free (p);
+  p = calloc (OUR_SIZE * 2, sizeof (int));
+
+  return ++p;
+}
+
+
+int
+main (void)
+{
+  if (have_mpx ())
+    {
+      int sx[OUR_SIZE];
+      int sa[OUR_SIZE];
+      int sb[OUR_SIZE];
+      int sc[OUR_SIZE];
+      int sd[OUR_SIZE];
+      int *x, *a, *b, *c, *d;
+
+      x = calloc (OUR_SIZE, sizeof (int));
+      a = calloc (OUR_SIZE, sizeof (int));
+      b = calloc (OUR_SIZE, sizeof (int));
+      c = calloc (OUR_SIZE, sizeof (int));
+      d = calloc (OUR_SIZE, sizeof (int));
+
+      upper (sx, sa, sb, sc, sd, 0);  /* bkpt 1.  */
+      x = upper_ptr (sx, sa, sb, sc, sd, 0);
+
+      free (x);
+      free (a);
+      free (b);
+      free (c);
+      free (d);
+    }
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.exp b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
new file mode 100644
index 0000000..d9c6eba
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
@@ -0,0 +1,90 @@
+# Copyright (C) 2016 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <walfred.tedeschi@intel.com> # # This 
+program is free software; you can redistribute it and/or modify # it 
+under the terms of the GNU General Public License as published by # the 
+Free Software Foundation; either version 3 of the License, or # (at 
+your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, # but 
+WITHOUT ANY WARRANTY; without even the implied warranty of # 
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the # GNU 
+General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License # 
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
+    verbose "Skipping x86 MPX tests."
+    return
+}
+
+standard_testfile
+
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
+
+if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+    [list debug nowarnings additional_flags=${comp_flags}]] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test_multiple "print have_mpx ()" "have mpx" {
+    -re ".*= 1\r\n$gdb_prompt " {
+        pass "check whether processor supports MPX"
+    }
+    -re ".*= 0\r\n$gdb_prompt " {
+        verbose "processor does not support MPX; skipping MPX tests"
+        return
+    }
+}
+
+# Needed by the return command.
+gdb_test_no_output "set confirm off"
+
+set break "bkpt 1."
+gdb_breakpoint [gdb_get_line_number "${break}"] 
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+gdb_test "p upper (x, a, b, c, d, 0)" "= 1"\
+    "test the call of int function - int"
+gdb_test "p upper_ptr (x, a, b, c, d, 0)"\
+    "= \\\(int \\\*\\\) $hex" "test the call of function - ptr"
+
+set break "bkpt 2."
+gdb_breakpoint [gdb_get_line_number "${break}"] 
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
+    bnd0 result in a convenience variable 1"
+
+gdb_test "return a" "#0  $hex in main.*" "First return"
+gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
+    "= 0" "return with bnd initialization off - ubound"
+gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
+    "= 0" "return with bnd initialization off - lbound"
+
+# Retesting with initialization off. 
+# All breakpoints were deleted need to recreate what we need.
+runto_main
+gdb_test_no_output "set mpx-bnd-init-on-return off"\
+    "Turn off initialization on return"
+
+set break "bkpt 3."
+gdb_breakpoint [gdb_get_line_number "${break}"] 
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
+    bnd0 result in a convenience variable 2"
+gdb_test "return a" "#0  $hex in main.*" "Second return"
+gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
+    "= 1" "return with bnd initialization on - ubound"
+gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
+    "= 1" "return with bnd initialization on - lbound"
+
+gdb_test "show mpx-bnd-init-on-return"\
+    "BND registers will not be initialized on return\\."\
+    "double check initialization on return"
--
2.1.4

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

Re: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

[Joel Brobecker]

On Wed, Jan 13, 2016 at 01:39:59PM +0100, Walfred Tedeschi wrote:
> When using the return command, execution of a function is aborted
> and present values are returned from that point.  That can cause
> bound violations in the MPX context.  To avoid such side-effects

comma at the end of this line: "To avoid such side-effects,"...

> a new set variable was added "mpx-bnd-init-on-return" which controls
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

a new setting ("mpx-bnd-init-on-return") was added to control

> the initialization of bound register when using the return command.
> 
> As bound initialization it is understood the set of the BND register
> to zero allowing the associated pointer to access the whole memory.

This sentence is not correct English, I don't think. I think you can
remove it entirely if you modify the next sentences as follow (note,
this is a boolean setting, so use "true" or "false" to describe its
value rather than numeric values)...

> As default the value of "mpx-bnd-init-on-return" is set to 1.  So
> bound register are initilized when using the "return" command.

If set to True (the default), all bound registers are set to zero
when using the "return" command.

> 2016-01-13  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* i386-tdep.c (mpx_bnd_init_on_return): new external variable.
> 	(show_mpx_init_on_return): New function.
> 	(_initialize_i386_tdepamd64_init_abi): Add new commands set/show
> 	mpx-bnd-init-on-return.
> 	* amd64-tdep.c (amd64_return_value): Use mpx-bnd-init-on-return.
> 	* NEWS: Add entry for set/show mpx-bnd-init-on-return command.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Intel Memory Protection Extensions): Add
> 	documentation on using set/show mpx-bnd-init-on-return.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* amd64-mpx-call.c: New file.
> 	* amd64-mpx-call.exp: New file.
> 
> ---
>  gdb/NEWS                                  |   4 +
>  gdb/amd64-tdep.c                          |  32 ++++++++
>  gdb/doc/gdb.texinfo                       |  12 +++
>  gdb/i386-tdep.c                           |  27 +++++++
>  gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 126 ++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  90 +++++++++++++++++++++
>  6 files changed, 291 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a222dfb..80022ec 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 7.10
>  
> +* show mpx-bnd-init-on-return
> +  set mpx-bnd-init-on-return on i386 and amd64
> +   Support for set bound registers to INIT state when using the command return.
> +
>  * Record btrace now supports non-stop mode.
>  
>  * Support for tracepoints on aarch64-linux was added in GDBserver.
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 732e91b..0ce41d1 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -708,12 +708,14 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>  		    gdb_byte *readbuf, const gdb_byte *writebuf)
>  {
>    enum amd64_reg_class theclass[2];
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    int len = TYPE_LENGTH (type);
>    static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM };
>    static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM };
>    int integer_reg = 0;
>    int sse_reg = 0;
>    int i;
> +  extern int mpx_bnd_init_on_return;

Please avoid these kinds of declarations like the pest. Normally,
what we should be doing to prevent this is to declare this extern
in a .h file, and then include that .h file from all the .c file
referencing that global. This way, the compiler can keep all the
uses consistent. Otherwise, some can change the type of that
variable and the compiler would compile the above incorrectly.

In this case, since this setting is only accessed from amd64-tdep,
I suggest you move this global, as well as the code creating the new
settting, inside amd64-tdep.c. This will avoid the need for an extern.
Please make mpx_bnd_init_on_return static as well.

> +	     if the return value we are setting is due to the use of the
> +	     "return" command (WRITEBUF is not NULL), it is likely that
> +	     the return is made prematurely, and therefore bnd0 register
> +	     unset could then cause the program to receive a spurious
> +	     bound violation, but this is only done if
> +	     "set mpx-bnd-init-on-return" is true.  */

Small nit (reminder): All sentences should start with a capital letter.
Thus "If the...".

I think some bits were lost in translation, though. Please replace
the text above by the following:

   	     If the return value we are setting is due to the use of the
   	     "return" command (WRITEBUF is not NULL), it is likely that
   	     the return is made prematurely.  In that situation, it is
             possible that the bnd0 register could still be unset, thus
             causing the program to receive a spurious bound violation.

             When "set mpx-bnd-init-on-return" is true, prevent this
             from happening by setting the bnd0 register to zero.  */


> +	  if (writebuf != NULL
> +	      && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
> +	    {
> +	      gdb_byte bnd_buf[16];
> +
> +	      memset (bnd_buf, 0, 16);
> +	      regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep), bnd_buf);
> +	    }
>  	}
>  
>        return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> @@ -833,6 +854,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>  				 writebuf + i * 8);
>      }
>  
> +  if (I387_BND0R_REGNUM (tdep) > 0)
> +    {
> +      gdb_byte bnd_buf[16];
> +      int i, init_bnd;
> +
> +      memset (bnd_buf, 0, 16);
> +      if (writebuf && mpx_bnd_init_on_return)
> +	for (i = 0; i < I387_NUM_BND_REGS; i++)
> +	  regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
> +    }

I suggest you use the same pattern as above (move the "if (writebuf
&& mpx_bnd_init_on_return)" to the first condition. It will be more
consistent with the other block of code you added, making it easier
to understand, and will also avoid a call to memset for nothing if
mpx_bnd_init_on_return is unset. Please also add a small comment
explaining why you do this. Thus:

          /* Same as in the class AMD64_MEMORY case above, where
             we set bnd0 to zero when doing a premature "return"
             and "set mpx-bnd-init-on-return" is true: Set all bound
             registers to zero in order to avoid spurious bound
             violations.  */

          if (writebuf != NULL
              && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
            {
              ...
            }

> +/* The value of the "set mpx-bnd-init-on-return setting.  */
> +
> +int mpx_bnd_init_on_return = 1;
> +
> +/*  Show state of the setting variable mpx-bnd-init-on-return.  */
> +
> +static void
> +show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty,
> +			     struct cmd_list_element *c, const char *value)
> +{
> +  if (mpx_bnd_init_on_return)
> +    fprintf_filtered (file,
> +		      _("BND registers will be initialized on return.\n"));
> +  else
> +    fprintf_filtered (file,
> +		      _("BND registers will not be initialized on return.\n"));
> +}
> +
>  static struct cmd_list_element *mpx_set_cmdlist, *mpx_show_cmdlist;
>  
>  /* Helper function for the CLI commands.  */
> @@ -8940,6 +8958,15 @@ Show Intel(R) Memory Protection Extensions specific variables."),
>   in the bound table.",
>  	   &mpx_set_cmdlist);
>  
> +  add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class,
> +			   &mpx_bnd_init_on_return, _("\
> + Set the bnd registers to INIT state when returning from a call."), _("\
> + Show the state of the mpx-bnd-init-on-return."),
> +			   NULL,
> +			   NULL,
> +			   show_mpx_bnd_init_on_return,
> +			   &setlist, &showlist);

As suggested above, please move all this code to amd64-tdep.c instead.

> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping x86 MPX tests."
> +    return

There is something I don't understand. Why are we running this test
on i?x86 if the bnd register handling is done in amd64-tdep (and
therefore seems limited to x86_64?

> +# Needed by the return command.
> +gdb_test_no_output "set confirm off"
> +
> +set break "bkpt 1."
> +gdb_breakpoint [gdb_get_line_number "${break}"]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +gdb_test "p upper (x, a, b, c, d, 0)" "= 1"\

Add one space before the equal sign, please; here and in all tests
below. Thank you!

> +    "test the call of int function - int"
> +gdb_test "p upper_ptr (x, a, b, c, d, 0)"\
> +    "= \\\(int \\\*\\\) $hex" "test the call of function - ptr"
> +
> +set break "bkpt 2."
> +gdb_breakpoint [gdb_get_line_number "${break}"]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
> +    bnd0 result in a convenience variable 1"

Let's not use the empty string for the match, but instead just
verify that the command returned some reasonable that looks reasonable.
Also, I would avoid splitting the test label (string) in two, by
formatting the test as:

gdb_test "p \$bound0 = \$bnd0" " = {<fill the correct regexp here>}" \
    "Saving the bnd0 result in a convenience variable 1"

As a side-effect, this will avoid having 4 spaces between "the" and
"bnd0".

Please apply these two recommendations in the tests below as well.

> +gdb_test "return a" "#0  $hex in main.*" "First return"
> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
> +    "= 0" "return with bnd initialization off - ubound"
> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
> +    "= 0" "return with bnd initialization off - lbound"
> +
> +# Retesting with initialization off. 
> +# All breakpoints were deleted need to recreate what we need.
> +runto_main
> +gdb_test_no_output "set mpx-bnd-init-on-return off"\
> +    "Turn off initialization on return"
> +
> +set break "bkpt 3."
> +gdb_breakpoint [gdb_get_line_number "${break}"]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
> +    bnd0 result in a convenience variable 2"
> +gdb_test "return a" "#0  $hex in main.*" "Second return"
> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
> +    "= 1" "return with bnd initialization on - ubound"
> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
> +    "= 1" "return with bnd initialization on - lbound"
> +
> +gdb_test "show mpx-bnd-init-on-return"\
> +    "BND registers will not be initialized on return\\."\
> +    "double check initialization on return"

Thank you,
-- 
Joel

Re: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls.

[Walfred Tedeschi]

Am 2/7/2016 um 8:06 AM schrieb Joel Brobecker:
> On Wed, Jan 13, 2016 at 01:39:59PM +0100, Walfred Tedeschi wrote:
>> When using the return command, execution of a function is aborted
>> and present values are returned from that point.  That can cause
>> bound violations in the MPX context.  To avoid such side-effects
>
> comma at the end of this line: "To avoid such side-effects,"...
>
>> a new set variable was added "mpx-bnd-init-on-return" which controls
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> a new setting ("mpx-bnd-init-on-return") was added to control
>
>> the initialization of bound register when using the return command.
>>
>> As bound initialization it is understood the set of the BND register
>> to zero allowing the associated pointer to access the whole memory.
>
> This sentence is not correct English, I don't think. I think you can
> remove it entirely if you modify the next sentences as follow (note,
> this is a boolean setting, so use "true" or "false" to describe its
> value rather than numeric values)...
>
>> As default the value of "mpx-bnd-init-on-return" is set to 1.  So
>> bound register are initilized when using the "return" command.
>
> If set to True (the default), all bound registers are set to zero
> when using the "return" command.
>
>> 2016-01-13  Walfred Tedeschi  <walfred.tedeschi@intel.com>
>>
>> 	* i386-tdep.c (mpx_bnd_init_on_return): new external variable.
>> 	(show_mpx_init_on_return): New function.
>> 	(_initialize_i386_tdepamd64_init_abi): Add new commands set/show
>> 	mpx-bnd-init-on-return.
>> 	* amd64-tdep.c (amd64_return_value): Use mpx-bnd-init-on-return.
>> 	* NEWS: Add entry for set/show mpx-bnd-init-on-return command.
>>
>> gdb/doc/ChangeLog:
>>
>> 	* gdb.texinfo (Intel Memory Protection Extensions): Add
>> 	documentation on using set/show mpx-bnd-init-on-return.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* amd64-mpx-call.c: New file.
>> 	* amd64-mpx-call.exp: New file.
>>
>> ---
>>   gdb/NEWS                                  |   4 +
>>   gdb/amd64-tdep.c                          |  32 ++++++++
>>   gdb/doc/gdb.texinfo                       |  12 +++
>>   gdb/i386-tdep.c                           |  27 +++++++
>>   gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 126 ++++++++++++++++++++++++++++++
>>   gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  90 +++++++++++++++++++++
>>   6 files changed, 291 insertions(+)
>>   create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
>>   create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index a222dfb..80022ec 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,10 @@
>>
>>   *** Changes since GDB 7.10
>>
>> +* show mpx-bnd-init-on-return
>> +  set mpx-bnd-init-on-return on i386 and amd64
>> +   Support for set bound registers to INIT state when using the command return.
>> +
>>   * Record btrace now supports non-stop mode.
>>
>>   * Support for tracepoints on aarch64-linux was added in GDBserver.
>> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
>> index 732e91b..0ce41d1 100644
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -708,12 +708,14 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>>   		    gdb_byte *readbuf, const gdb_byte *writebuf)
>>   {
>>     enum amd64_reg_class theclass[2];
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>>     int len = TYPE_LENGTH (type);
>>     static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM };
>>     static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM };
>>     int integer_reg = 0;
>>     int sse_reg = 0;
>>     int i;
>> +  extern int mpx_bnd_init_on_return;
>
> Please avoid these kinds of declarations like the pest. Normally,
> what we should be doing to prevent this is to declare this extern
> in a .h file, and then include that .h file from all the .c file
> referencing that global. This way, the compiler can keep all the
> uses consistent. Otherwise, some can change the type of that
> variable and the compiler would compile the above incorrectly.
>
> In this case, since this setting is only accessed from amd64-tdep,
> I suggest you move this global, as well as the code creating the new
> settting, inside amd64-tdep.c. This will avoid the need for an extern.
> Please make mpx_bnd_init_on_return static as well.
>
>> +	     if the return value we are setting is due to the use of the
>> +	     "return" command (WRITEBUF is not NULL), it is likely that
>> +	     the return is made prematurely, and therefore bnd0 register
>> +	     unset could then cause the program to receive a spurious
>> +	     bound violation, but this is only done if
>> +	     "set mpx-bnd-init-on-return" is true.  */
>
> Small nit (reminder): All sentences should start with a capital letter.
> Thus "If the...".
>
> I think some bits were lost in translation, though. Please replace
> the text above by the following:
>
>     	     If the return value we are setting is due to the use of the
>     	     "return" command (WRITEBUF is not NULL), it is likely that
>     	     the return is made prematurely.  In that situation, it is
>               possible that the bnd0 register could still be unset, thus
>               causing the program to receive a spurious bound violation.
>
>               When "set mpx-bnd-init-on-return" is true, prevent this
>               from happening by setting the bnd0 register to zero.  */
>
>
>> +	  if (writebuf != NULL
>> +	      && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
>> +	    {
>> +	      gdb_byte bnd_buf[16];
>> +
>> +	      memset (bnd_buf, 0, 16);
>> +	      regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep), bnd_buf);
>> +	    }
>>   	}
>>
>>         return RETURN_VALUE_ABI_RETURNS_ADDRESS;
>> @@ -833,6 +854,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>>   				 writebuf + i * 8);
>>       }
>>
>> +  if (I387_BND0R_REGNUM (tdep) > 0)
>> +    {
>> +      gdb_byte bnd_buf[16];
>> +      int i, init_bnd;
>> +
>> +      memset (bnd_buf, 0, 16);
>> +      if (writebuf && mpx_bnd_init_on_return)
>> +	for (i = 0; i < I387_NUM_BND_REGS; i++)
>> +	  regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
>> +    }
>
> I suggest you use the same pattern as above (move the "if (writebuf
> && mpx_bnd_init_on_return)" to the first condition. It will be more
> consistent with the other block of code you added, making it easier
> to understand, and will also avoid a call to memset for nothing if
> mpx_bnd_init_on_return is unset. Please also add a small comment
> explaining why you do this. Thus:
>
>            /* Same as in the class AMD64_MEMORY case above, where
>               we set bnd0 to zero when doing a premature "return"
>               and "set mpx-bnd-init-on-return" is true: Set all bound
>               registers to zero in order to avoid spurious bound
>               violations.  */
>
>            if (writebuf != NULL
>                && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0)
>              {
>                ...
>              }
>
>> +/* The value of the "set mpx-bnd-init-on-return setting.  */
>> +
>> +int mpx_bnd_init_on_return = 1;
>> +
>> +/*  Show state of the setting variable mpx-bnd-init-on-return.  */
>> +
>> +static void
>> +show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty,
>> +			     struct cmd_list_element *c, const char *value)
>> +{
>> +  if (mpx_bnd_init_on_return)
>> +    fprintf_filtered (file,
>> +		      _("BND registers will be initialized on return.\n"));
>> +  else
>> +    fprintf_filtered (file,
>> +		      _("BND registers will not be initialized on return.\n"));
>> +}
>> +
>>   static struct cmd_list_element *mpx_set_cmdlist, *mpx_show_cmdlist;
>>
>>   /* Helper function for the CLI commands.  */
>> @@ -8940,6 +8958,15 @@ Show Intel(R) Memory Protection Extensions specific variables."),
>>    in the bound table.",
>>   	   &mpx_set_cmdlist);
>>
>> +  add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class,
>> +			   &mpx_bnd_init_on_return, _("\
>> + Set the bnd registers to INIT state when returning from a call."), _("\
>> + Show the state of the mpx-bnd-init-on-return."),
>> +			   NULL,
>> +			   NULL,
>> +			   show_mpx_bnd_init_on_return,
>> +			   &setlist, &showlist);
>
> As suggested above, please move all this code to amd64-tdep.c instead.
>
>> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
>> +    verbose "Skipping x86 MPX tests."
>> +    return
>
> There is something I don't understand. Why are we running this test
> on i?x86 if the bnd register handling is done in amd64-tdep (and
> therefore seems limited to x86_64?
>
>> +# Needed by the return command.
>> +gdb_test_no_output "set confirm off"
>> +
>> +set break "bkpt 1."
>> +gdb_breakpoint [gdb_get_line_number "${break}"]
>> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
>> +gdb_test "p upper (x, a, b, c, d, 0)" "= 1"\
>
> Add one space before the equal sign, please; here and in all tests
> below. Thank you!
>
>> +    "test the call of int function - int"
>> +gdb_test "p upper_ptr (x, a, b, c, d, 0)"\
>> +    "= \\\(int \\\*\\\) $hex" "test the call of function - ptr"
>> +
>> +set break "bkpt 2."
>> +gdb_breakpoint [gdb_get_line_number "${break}"]
>> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
>> +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
>> +    bnd0 result in a convenience variable 1"
>
> Let's not use the empty string for the match, but instead just
> verify that the command returned some reasonable that looks reasonable.
> Also, I would avoid splitting the test label (string) in two, by
> formatting the test as:
>
> gdb_test "p \$bound0 = \$bnd0" " = {<fill the correct regexp here>}" \
>      "Saving the bnd0 result in a convenience variable 1"
>
> As a side-effect, this will avoid having 4 spaces between "the" and
> "bnd0".
>
> Please apply these two recommendations in the tests below as well.
>
>> +gdb_test "return a" "#0  $hex in main.*" "First return"
>> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
>> +    "= 0" "return with bnd initialization off - ubound"
>> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
>> +    "= 0" "return with bnd initialization off - lbound"
>> +
>> +# Retesting with initialization off.
>> +# All breakpoints were deleted need to recreate what we need.
>> +runto_main
>> +gdb_test_no_output "set mpx-bnd-init-on-return off"\
>> +    "Turn off initialization on return"
>> +
>> +set break "bkpt 3."
>> +gdb_breakpoint [gdb_get_line_number "${break}"]
>> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
>> +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
>> +    bnd0 result in a convenience variable 2"
>> +gdb_test "return a" "#0  $hex in main.*" "Second return"
>> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
>> +    "= 1" "return with bnd initialization on - ubound"
>> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
>> +    "= 1" "return with bnd initialization on - lbound"
>> +
>> +gdb_test "show mpx-bnd-init-on-return"\
>> +    "BND registers will not be initialized on return\\."\
>> +    "double check initialization on return"
>
> Thank you,
>
Joel,

Thanks for your review!
I will address your comments for the next version.

Regards,
-Fred
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

[PATCH V3 1/2] ABI changes for MPX.

[Walfred Tedeschi]

Code reflects what is presented in the ABI document:
https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI
Here new class POINTER was added.  GDB code is modified to mirror
this new class. (page 134)

In addition to that set bound registers to INIT state (access to
all memory) before performing the inferior function call.

2016-01-13  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* amd64-tdep.c (amd64_reg_class): Add new class AMD64_POINTER.
	(amd64_merge_classes): Add AMD64_POINTER to merging classes rules.
	(amd64_classify): Add AMD64_POINTER.
	(amd64_return_value): Add AMD64_POINTER.
	(amd64_push_arguments): Add new AMD64_POINTER class.
	(amd64_push_dummy_call): Set bound registers to INIT state before
	performing the call.

gdb/doc/ChangeLog:

	* gdb.texinfo (Intel Memory Protection Extension): Add entry for
	inferior function calls for MPX.

---
 gdb/amd64-tdep.c    | 44 +++++++++++++++++++++++++++++++++++---------
 gdb/doc/gdb.texinfo |  8 ++++++++
 2 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 6096ce9..732e91b 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -461,6 +461,7 @@ amd64_pseudo_register_write (struct gdbarch *gdbarch,
 
 enum amd64_reg_class
 {
+  AMD64_POINTER,
   AMD64_INTEGER,
   AMD64_SSE,
   AMD64_SSEUP,
@@ -492,18 +493,22 @@ amd64_merge_classes (enum amd64_reg_class class1, enum amd64_reg_class class2)
   if (class1 == AMD64_MEMORY || class2 == AMD64_MEMORY)
     return AMD64_MEMORY;
 
-  /* Rule (d): If one of the classes is INTEGER, the result is INTEGER.  */
+  /* Rule (d): If one of the classes is POINTER, the result is POINTER.  */
+  if (class1 == AMD64_POINTER || class2 == AMD64_POINTER)
+    return AMD64_POINTER;
+
+  /* Rule (e): If one of the classes is INTEGER, the result is INTEGER.  */
   if (class1 == AMD64_INTEGER || class2 == AMD64_INTEGER)
     return AMD64_INTEGER;
 
-  /* Rule (e): If one of the classes is X87, X87UP, COMPLEX_X87 class,
+  /* Rule (f): If one of the classes is X87, X87UP, COMPLEX_X87 class,
      MEMORY is used as class.  */
   if (class1 == AMD64_X87 || class1 == AMD64_X87UP
       || class1 == AMD64_COMPLEX_X87 || class2 == AMD64_X87
       || class2 == AMD64_X87UP || class2 == AMD64_COMPLEX_X87)
     return AMD64_MEMORY;
 
-  /* Rule (f): Otherwise class SSE is used.  */
+  /* Rule (g): Otherwise class SSE is used.  */
   return AMD64_SSE;
 }
 
@@ -636,14 +641,17 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
 
   theclass[0] = theclass[1] = AMD64_NO_CLASS;
 
+  /* Pointer and reference are of the POINTER class.  */
+  if (code == TYPE_CODE_PTR || code == TYPE_CODE_REF)
+    theclass[0] = AMD64_POINTER;
+
   /* Arguments of types (signed and unsigned) _Bool, char, short, int,
      long, long long, and pointers are in the INTEGER class.  Similarly,
      range types, used by languages such as Ada, are also in the INTEGER
      class.  */
-  if ((code == TYPE_CODE_INT || code == TYPE_CODE_ENUM
+  else if ((code == TYPE_CODE_INT || code == TYPE_CODE_ENUM
        || code == TYPE_CODE_BOOL || code == TYPE_CODE_RANGE
-       || code == TYPE_CODE_CHAR
-       || code == TYPE_CODE_PTR || code == TYPE_CODE_REF)
+       || code == TYPE_CODE_CHAR)
       && (len == 1 || len == 2 || len == 4 || len == 8))
     theclass[0] = AMD64_INTEGER;
 
@@ -693,6 +701,7 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
     amd64_classify_aggregate (type, theclass);
 }
 
+
 static enum return_value_convention
 amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 		    struct type *type, struct regcache *regcache,
@@ -770,8 +779,9 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
       switch (theclass[i])
 	{
 	case AMD64_INTEGER:
-	  /* 3. If the class is INTEGER, the next available register
-	     of the sequence %rax, %rdx is used.  */
+	case AMD64_POINTER:
+	  /* 3. If the class is INTEGER or POINTER, the next available
+	     register of the sequence %rax, %rdx is used.  */
 	  regnum = integer_regnum[integer_reg++];
 	  break;
 
@@ -876,7 +886,7 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
          this argument.  */
       for (j = 0; j < 2; j++)
 	{
-	  if (theclass[j] == AMD64_INTEGER)
+	  if (theclass[j] == AMD64_INTEGER || theclass[j] == AMD64_POINTER)
 	    needed_integer_regs++;
 	  else if (theclass[j] == AMD64_SSE)
 	    needed_sse_regs++;
@@ -907,6 +917,7 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
 
 	      switch (theclass[j])
 		{
+		case AMD64_POINTER:
 		case AMD64_INTEGER:
 		  regnum = integer_regnum[integer_reg++];
 		  break;
@@ -966,8 +977,23 @@ amd64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		       int struct_return, CORE_ADDR struct_addr)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   gdb_byte buf[8];
 
+  /* When MPX is enabled, all bnd registers have to be initialized
+     before the call.  This avoids an undesired bound violation
+     during the function's execution.  */
+
+  if (I387_BND0R_REGNUM (tdep) > 0)
+    {
+      gdb_byte bnd_buf[16];
+      int i;
+
+      memset (bnd_buf, 0, 16);
+      for (i = 0; i < I387_BND0R_REGNUM (tdep); i++)
+	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
+    }
+
   /* Pass arguments.  */
   sp = amd64_push_arguments (regcache, nargs, args, sp, struct_return);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b82f3c6..ccde84a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22064,6 +22064,14 @@ whose bounds are to be changed, @var{lbound} and @var{ubound} are new values
 for lower and upper bounds respectively.
 @end table
 
+While calling functions from the debugger, of an Intel MPX enabled program,
+boundary registers have to be set to the INIT state before performing the
+call, to avoid boundary violations while performing the call.  A bound is
+defined to be in the INIT state when the pointer associated to that boundary
+can access the whole memory, in this case the register bound register
+associated to it has value 0, e.g. if the register associated is bnd0raw
+its value will be @{0x0, 0x0@}.
+
 @node Alpha
 @subsection Alpha
 
-- 
2.1.4

Re: [PATCH V3 1/2] ABI changes for MPX.

[Eli Zaretskii]

> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Cc: gdb-patches@sourceware.org, Walfred Tedeschi <walfred.tedeschi@intel.com>
> Date: Wed, 13 Jan 2016 13:39:58 +0100
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Intel Memory Protection Extension): Add entry for
> 	inferior function calls for MPX.

This part is OK, thanks.

Re: [PATCH V3 1/2] ABI changes for MPX.

[Joel Brobecker]

Walfred,

On Wed, Jan 13, 2016 at 01:39:58PM +0100, Walfred Tedeschi wrote:
> Code reflects what is presented in the ABI document:
> https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI
> Here new class POINTER was added.  GDB code is modified to mirror
> this new class. (page 134)
> 
> In addition to that set bound registers to INIT state (access to
> all memory) before performing the inferior function call.
> 
> 2016-01-13  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* amd64-tdep.c (amd64_reg_class): Add new class AMD64_POINTER.
> 	(amd64_merge_classes): Add AMD64_POINTER to merging classes rules.
> 	(amd64_classify): Add AMD64_POINTER.
> 	(amd64_return_value): Add AMD64_POINTER.
> 	(amd64_push_arguments): Add new AMD64_POINTER class.
> 	(amd64_push_dummy_call): Set bound registers to INIT state before
> 	performing the call.

I think you need to update the following piece of code as well (in
amd64_push_arguments):

      /* Calculate the number of integer and SSE registers needed for
         this argument.  */
      for (j = 0; j < 2; j++)
        {
          if (theclass[j] == AMD64_INTEGER)
            needed_integer_regs++;

Otherwise, you'll have a discrepancy between needed_integer_regs
and integer_reg.

As far as I understand, POINTER and INTEGER arguments are passed
exactly the same way. The distinction appears to only be useful
for bound register handling. And since this patch unilaterally
sets the bound registers to zero, we could have done it without
adding the extra register classification (and thus reducing
the maintenance cost of having to handle both). I'm really tempted
to suggest we drop the new class altogether, avoiding the risk
of having missed another spot where we needd to add POINTER class
handling, and just keep the change which sets the bound registers
to zero. If, one day, someone decides to enhance the debugger
to handle the bound registers more closely to what the ABI suggests,
I think we could have a separate function that would tell us if
a given type is of class POINTER or not, rather than adding a class.

Am I understanding the situation correctly?

> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Intel Memory Protection Extension): Add entry for
> 	inferior function calls for MPX.


> @@ -693,6 +701,7 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
>      amd64_classify_aggregate (type, theclass);
>  }
>  
> +
>  static enum return_value_convention
>  amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>  		    struct type *type, struct regcache *regcache,

There is no reason to add a new line, here, so let's avoid this
unnecessary change.

-- 
Joel