[PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Walfred Tedeschi]

This patch initializes the bnd registers before executing the inferior
call.  BND registers can be in arbitrary values at the moment of the
inferior call.  In case the function being called uses as part of the
parameters bnd register, e.g. when passing a pointer as parameter, the
current value of the register will be used.  This can cause boundary
violations that are not due to a real bug or even desired by the user.
In this sense the best to be done is set the bnd registers to allow
access to the whole memory, i.e. initialized state, before pushing the
inferior call.

Comments:
* Patch has been discussed together with an additional patch.
Since it can be reviewed and approved separately I am resubmitting it.
* The present patch is the last version already posted.
* Commit comment was also improved, former commit message was not
explaining the real reason behind the patch.
* Added a test related to this commit only (test was only on the second
ABI patch).
* I realized also that i was owning some aswer to Yao, related to:
https://sourceware.org/ml/gdb-patches/2016-04/msg00574.html

Thanks for the review!

In case i could not answer your comments with the rewriting of the
commit message.

For the breakpoint case pointed by Yao, user has added intentionally
where it should be hit. GDB behavior at this point is completely right.

In terms of the initialization of the bnd registers the situations is a
bit different.

Lets say so:
1. User has stopped at the middle function foo, state of the register BND0
is in a state that is right for the current code being executed.  It can be
holding any boundary for the current pointers in the scope of foo.

2. User performs an inferior call to bar, one of the parameters of bar is
a pointer.  Without the current patch the inferior call will use the value
of BND0 and very probably will cause a false boundary violation.

Yao, sorry also for forwarding instead of answering. Internal issues
with e-mail client, finally i was able to configure my client again.

From V6 answering comments from Luis Machado:
https://sourceware.org/ml/gdb-patches/2017-01/msg00327.html

I modified all except those:

* Identation off above?
- This was ok.

* Why define a test name of "have mpx" and not use it below?
- I haven't understood this comment.

Thanks a lot for your review!


2017-01-12  Walfred Tedeschi <walfred.tedeschi@intel.com>

gdb/ChangeLog:

	* i387-tdep.h (i387_reset_bnd_regs): Add function definition.
	* i387-tdep.c (i387_reset_bnd_regs): Add function implementation.
	* i386-tdep.c (i386_push_dummy_call): Call i387_reset_bnd_regs.
	* amd64-tdep (amd64_push_dummy_call): Call i387_reset_bnd_regs.

gdb/testsuite/ChangeLog:

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

---
 gdb/amd64-tdep.c                         |   3 +
 gdb/i386-tdep.c                          |   3 +
 gdb/i387-tdep.c                          |  18 ++++++
 gdb/i387-tdep.h                          |   4 ++
 gdb/testsuite/gdb.arch/i386-mpx-call.c   | 106 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/i386-mpx-call.exp |  58 +++++++++++++++++
 6 files changed, 192 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-call.c
 create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-call.exp

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 233c65a..54cc17d 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -999,6 +999,9 @@ amd64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   gdb_byte buf[8];
 
+  /* Reset bound registers.  */
+  i387_reset_bnd_regs (gdbarch, regcache);
+
   /* Pass arguments.  */
   sp = amd64_push_arguments (regcache, nargs, args, sp, struct_return);
 
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 8a4d59f..861122b 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2663,6 +2663,9 @@ i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   int write_pass;
   int args_space = 0;
 
+  /* Reset bound registers.  */
+  i387_reset_bnd_regs (gdbarch, regcache);
+
   /* Determine the total space required for arguments and struct
      return address in a first pass (allowing for 16-byte-aligned
      arguments), then push arguments in a second pass.  */
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index adbe721..155a106 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -1772,3 +1772,21 @@ i387_return_value (struct gdbarch *gdbarch, struct regcache *regcache)
   regcache_raw_write_unsigned (regcache, I387_FTAG_REGNUM (tdep), 0x3fff);
 
 }
+
+  /* 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.  */
+void
+i387_reset_bnd_regs (struct gdbarch *gdbarch, struct regcache *regcache)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  if (I387_BND0R_REGNUM (tdep) > 0)
+    {
+      gdb_byte bnd_buf[16];
+
+      memset (bnd_buf, 0, 16);
+      for (int i = 0; i < I387_BND0R_REGNUM (tdep); i++)
+	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
+    }
+}
diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h
index 81d45b7..4e4c47c 100644
--- a/gdb/i387-tdep.h
+++ b/gdb/i387-tdep.h
@@ -156,4 +156,8 @@ extern void i387_collect_xsave (const struct regcache *regcache,
 extern void i387_return_value (struct gdbarch *gdbarch,
 			       struct regcache *regcache);
 
+/* Set all bnd registers to the INIT state.  INIT state means
+   all memory range can be accessed.  */
+extern void i387_reset_bnd_regs (struct gdbarch *gdbarch,
+			         struct regcache *regcache);
 #endif /* i387-tdep.h */
diff --git a/gdb/testsuite/gdb.arch/i386-mpx-call.c b/gdb/testsuite/gdb.arch/i386-mpx-call.c
new file mode 100644
index 0000000..896e63d
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-call.c
@@ -0,0 +1,106 @@
+/* Test for inferior function calls MPX context.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   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
+
+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
+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);
+  value = *(d + len);
+  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/i386-mpx-call.exp b/gdb/testsuite/gdb.arch/i386-mpx-call.exp
new file mode 100644
index 0000000..812489b
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-call.exp
@@ -0,0 +1,58 @@
+# Copyright (C) 2017 Free Software Foundation, Inc.
+#
+# 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-*-* ] } {
+    untested "skipping x86 MPX tests."
+    return
+}
+
+standard_testfile
+
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
+
+if {[prepare_for_testing "failed to prepare" ${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 " {
+        untested "processor does not support MPX; skipping tests"
+        return
+    }
+}
+
+# Needed by the return command.
+gdb_test_no_output "set confirm off"
+
+set bound_reg " = \\\{lbound = $hex, ubound = $hex\\\}.*"
+
+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"
+
-- 
2.5.5

[ping] [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

Hello all,

Do you see value on this this patch or you consider that the user has to 
take care himself on the bound registers before doing the inferior call?


With this patch my intention was that with or without MPX support the 
look and feel for the user would be the same.

Thanks and regards,

/Fred

Am 1/31/2017 um 3:13 PM schrieb Walfred Tedeschi:
> This patch initializes the bnd registers before executing the inferior
> call.  BND registers can be in arbitrary values at the moment of the
> inferior call.  In case the function being called uses as part of the
> parameters bnd register, e.g. when passing a pointer as parameter, the
> current value of the register will be used.  This can cause boundary
> violations that are not due to a real bug or even desired by the user.
> In this sense the best to be done is set the bnd registers to allow
> access to the whole memory, i.e. initialized state, before pushing the
> inferior call.
>
> Comments:
> * Patch has been discussed together with an additional patch.
> Since it can be reviewed and approved separately I am resubmitting it.
> * The present patch is the last version already posted.
> * Commit comment was also improved, former commit message was not
> explaining the real reason behind the patch.
> * Added a test related to this commit only (test was only on the second
> ABI patch).
> * I realized also that i was owning some aswer to Yao, related to:
> https://sourceware.org/ml/gdb-patches/2016-04/msg00574.html
>
> Thanks for the review!
>
> In case i could not answer your comments with the rewriting of the
> commit message.
>
> For the breakpoint case pointed by Yao, user has added intentionally
> where it should be hit. GDB behavior at this point is completely right.
>
> In terms of the initialization of the bnd registers the situations is a
> bit different.
>
> Lets say so:
> 1. User has stopped at the middle function foo, state of the register BND0
> is in a state that is right for the current code being executed.  It can be
> holding any boundary for the current pointers in the scope of foo.
>
> 2. User performs an inferior call to bar, one of the parameters of bar is
> a pointer.  Without the current patch the inferior call will use the value
> of BND0 and very probably will cause a false boundary violation.
>
> Yao, sorry also for forwarding instead of answering. Internal issues
> with e-mail client, finally i was able to configure my client again.
>
>  From V6 answering comments from Luis Machado:
> https://sourceware.org/ml/gdb-patches/2017-01/msg00327.html
>
> I modified all except those:
>
> * Identation off above?
> - This was ok.
>
> * Why define a test name of "have mpx" and not use it below?
> - I haven't understood this comment.
>
> Thanks a lot for your review!
>
>
> 2017-01-12  Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> gdb/ChangeLog:
>
> 	* i387-tdep.h (i387_reset_bnd_regs): Add function definition.
> 	* i387-tdep.c (i387_reset_bnd_regs): Add function implementation.
> 	* i386-tdep.c (i386_push_dummy_call): Call i387_reset_bnd_regs.
> 	* amd64-tdep (amd64_push_dummy_call): Call i387_reset_bnd_regs.
>
> gdb/testsuite/ChangeLog:
>
> 	* i386-mpx-call.c: New file.
> 	* i386-mpx-call.exp: New file.
>
> ---
>   gdb/amd64-tdep.c                         |   3 +
>   gdb/i386-tdep.c                          |   3 +
>   gdb/i387-tdep.c                          |  18 ++++++
>   gdb/i387-tdep.h                          |   4 ++
>   gdb/testsuite/gdb.arch/i386-mpx-call.c   | 106 +++++++++++++++++++++++++++++++
>   gdb/testsuite/gdb.arch/i386-mpx-call.exp |  58 +++++++++++++++++
>   6 files changed, 192 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-call.c
>   create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-call.exp
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 233c65a..54cc17d 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -999,6 +999,9 @@ amd64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>     enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>     gdb_byte buf[8];
>   
> +  /* Reset bound registers.  */
> +  i387_reset_bnd_regs (gdbarch, regcache);
> +
>     /* Pass arguments.  */
>     sp = amd64_push_arguments (regcache, nargs, args, sp, struct_return);
>   
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 8a4d59f..861122b 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -2663,6 +2663,9 @@ i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>     int write_pass;
>     int args_space = 0;
>   
> +  /* Reset bound registers.  */
> +  i387_reset_bnd_regs (gdbarch, regcache);
> +
>     /* Determine the total space required for arguments and struct
>        return address in a first pass (allowing for 16-byte-aligned
>        arguments), then push arguments in a second pass.  */
> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index adbe721..155a106 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -1772,3 +1772,21 @@ i387_return_value (struct gdbarch *gdbarch, struct regcache *regcache)
>     regcache_raw_write_unsigned (regcache, I387_FTAG_REGNUM (tdep), 0x3fff);
>   
>   }
> +
> +  /* 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.  */
> +void
> +i387_reset_bnd_regs (struct gdbarch *gdbarch, struct regcache *regcache)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (I387_BND0R_REGNUM (tdep) > 0)
> +    {
> +      gdb_byte bnd_buf[16];
> +
> +      memset (bnd_buf, 0, 16);
> +      for (int i = 0; i < I387_BND0R_REGNUM (tdep); i++)
> +	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
> +    }
> +}
> diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h
> index 81d45b7..4e4c47c 100644
> --- a/gdb/i387-tdep.h
> +++ b/gdb/i387-tdep.h
> @@ -156,4 +156,8 @@ extern void i387_collect_xsave (const struct regcache *regcache,
>   extern void i387_return_value (struct gdbarch *gdbarch,
>   			       struct regcache *regcache);
>   
> +/* Set all bnd registers to the INIT state.  INIT state means
> +   all memory range can be accessed.  */
> +extern void i387_reset_bnd_regs (struct gdbarch *gdbarch,
> +			         struct regcache *regcache);
>   #endif /* i387-tdep.h */
> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-call.c b/gdb/testsuite/gdb.arch/i386-mpx-call.c
> new file mode 100644
> index 0000000..896e63d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-call.c
> @@ -0,0 +1,106 @@
> +/* Test for inferior function calls MPX context.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   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
> +
> +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
> +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);
> +  value = *(d + len);
> +  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/i386-mpx-call.exp b/gdb/testsuite/gdb.arch/i386-mpx-call.exp
> new file mode 100644
> index 0000000..812489b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-call.exp
> @@ -0,0 +1,58 @@
> +# Copyright (C) 2017 Free Software Foundation, Inc.
> +#
> +# 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-*-* ] } {
> +    untested "skipping x86 MPX tests."
> +    return
> +}
> +
> +standard_testfile
> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
> +
> +if {[prepare_for_testing "failed to prepare" ${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 " {
> +        untested "processor does not support MPX; skipping tests"
> +        return
> +    }
> +}
> +
> +# Needed by the return command.
> +gdb_test_no_output "set confirm off"
> +
> +set bound_reg " = \\\{lbound = $hex, ubound = $hex\\\}.*"
> +
> +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"
> +

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 V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

On 01/31/2017 03:13 PM, Walfred Tedeschi wrote:
> This patch initializes the bnd registers before executing the inferior
> call.  BND registers can be in arbitrary values at the moment of the
> inferior call.  In case the function being called uses as part of the
> parameters bnd register, e.g. when passing a pointer as parameter, the
> current value of the register will be used.  This can cause boundary
> violations that are not due to a real bug or even desired by the user.
> In this sense the best to be done is set the bnd registers to allow
> access to the whole memory, i.e. initialized state, before pushing the
> inferior call.

This explains the reason for clearing better ...

> +
> +  /* 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.  */
> +void
> +i387_reset_bnd_regs (struct gdbarch *gdbarch, struct regcache *regcache)

... than this, IMO.  The comment in the code doesn't talk about
"arbitrary values", for example.  In any case, this comment should be next to
the infcall code in question, not here, since it won't make sense for
any other call site that decided to call this function in the future,
unrelated to inferior function calls.  Note how "the call" is
assuming this is talking about an infcall, but that's only clear because
we have the context of the patch; it won't be clear to anyone reading
the code after if is merged.

Also, comment is oddly indented to two spaces too much.

> +/* Set all bnd registers to the INIT state.  INIT state means
> +   all memory range can be accessed.  */
> +extern void i387_reset_bnd_regs (struct gdbarch *gdbarch,
> +			         struct regcache *regcache);

s/all memory range/all memory/  I think.

> 
> 2017-01-12  Walfred Tedeschi <walfred.tedeschi@intel.com>
> 
> gdb/ChangeLog:
> 
> 	* i387-tdep.h (i387_reset_bnd_regs): Add function definition.
> 	* i387-tdep.c (i387_reset_bnd_regs): Add function implementation.

s/Add/New/

> 	* i386-tdep.c (i386_push_dummy_call): Call i387_reset_bnd_regs.
> 	* amd64-tdep (amd64_push_dummy_call): Call i387_reset_bnd_regs.
> 

>  #endif /* i387-tdep.h */


> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-call.c b/gdb/testsuite/gdb.arch/i386-mpx-call.c
> new file mode 100644
> index 0000000..896e63d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-call.c
> @@ -0,0 +1,106 @@
> +/* Test for inferior function calls MPX context.
...
> +
> +#include <stdio.h>

Do we need to include stdio.h?  Would stdlib.h instead do?

> +#include "x86-cpuid.h"
> +
> +#define OUR_SIZE    5

Should this gain a describing comment?   Might not be
clear what this is about.

> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +    [list debug nowarnings additional_flags=${comp_flags}]] } {

Why "nowarnings" ?

> +    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 " {
> +        untested "processor does not support MPX; skipping tests"
> +        return
> +    }
> +}
> +
> +# Needed by the return command.
> +gdb_test_no_output "set confirm off"
> +
> +set bound_reg " = \\\{lbound = $hex, ubound = $hex\\\}.*"
> +
> +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"

All tests test something, so the "test the" is redundant.
Also doesn't "int function - int" have a redundant "int" ?

Thanks,
Pedro Alves

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

Pedro,

Thanks a lot for your review!

On 01/31/2017 03:13 PM, Walfred Tedeschi wrote:
>> This patch initializes the bnd registers before executing the inferior
>> call.  BND registers can be in arbitrary values at the moment of the
>> inferior call.  In case the function being called uses as part of the
>> parameters bnd register, e.g. when passing a pointer as parameter, the
>> current value of the register will be used.  This can cause boundary
>> violations that are not due to a real bug or even desired by the user.
>> In this sense the best to be done is set the bnd registers to allow
>> access to the whole memory, i.e. initialized state, before pushing the
>> inferior call.
> This explains the reason for clearing better ...
Yes, it was my intention. Do you see value to have the patch in then?
>> +
>> +  /* 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.  */
>> +void
>> +i387_reset_bnd_regs (struct gdbarch *gdbarch, struct regcache *regcache)
> ... than this, IMO.  The comment in the code doesn't talk about
> "arbitrary values", for example.  In any case, this comment should be next to
> the infcall code in question, not here, since it won't make sense for
> any other call site that decided to call this function in the future,
> unrelated to inferior function calls.  Note how "the call" is
> assuming this is talking about an infcall, but that's only clear because
> we have the context of the patch; it won't be clear to anyone reading
> the code after if is merged.
>
> Also, comment is oddly indented to two spaces too much.
Agreed, I will change the comments and explanations!
>> +/* Set all bnd registers to the INIT state.  INIT state means
>> +   all memory range can be accessed.  */
>> +extern void i387_reset_bnd_regs (struct gdbarch *gdbarch,
>> +			         struct regcache *regcache);
> s/all memory range/all memory/  I think.
Yes! Thanks!
>> 2017-01-12  Walfred Tedeschi <walfred.tedeschi@intel.com>
>>
>> gdb/ChangeLog:
>>
>> 	* i387-tdep.h (i387_reset_bnd_regs): Add function definition.
>> 	* i387-tdep.c (i387_reset_bnd_regs): Add function implementation.
> s/Add/New/
>
>> 	* i386-tdep.c (i386_push_dummy_call): Call i387_reset_bnd_regs.
>> 	* amd64-tdep (amd64_push_dummy_call): Call i387_reset_bnd_regs.
>>
>>   #endif /* i387-tdep.h */
>
>> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-call.c b/gdb/testsuite/gdb.arch/i386-mpx-call.c
>> new file mode 100644
>> index 0000000..896e63d
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/i386-mpx-call.c
>> @@ -0,0 +1,106 @@
>> +/* Test for inferior function calls MPX context.
> ...
>> +
>> +#include <stdio.h>
> Do we need to include stdio.h?  Would stdlib.h instead do?
I will try to eliminate this dependency.
>> +#include "x86-cpuid.h"
>> +
>> +#define OUR_SIZE    5
> Should this gain a describing comment?   Might not be
> clear what this is about.
Yes! Better naming and a comment should help.
>> +
>> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
>> +
>> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
>> +    [list debug nowarnings additional_flags=${comp_flags}]] } {
> Why "nowarnings" ?
I will also modify it! (Old commit)

>> +    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 " {
>> +        untested "processor does not support MPX; skipping tests"
>> +        return
>> +    }
>> +}
>> +
>> +# Needed by the return command.
>> +gdb_test_no_output "set confirm off"
>> +
>> +set bound_reg " = \\\{lbound = $hex, ubound = $hex\\\}.*"
>> +
>> +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"
> All tests test something, so the "test the" is redundant.
> Also doesn't "int function - int" have a redundant "int" ?
Will change as well!
> Thanks,
> Pedro Alves
>

Thanks again!
Best 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

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

On 02/07/2017 08:56 AM, Tedeschi, Walfred wrote:
> On 01/31/2017 03:13 PM, Walfred Tedeschi wrote:
>>> This patch initializes the bnd registers before executing the inferior
>>> call.  BND registers can be in arbitrary values at the moment of the
>>> inferior call.  In case the function being called uses as part of the
>>> parameters bnd register, e.g. when passing a pointer as parameter, the
>>> current value of the register will be used.  This can cause boundary
>>> violations that are not due to a real bug or even desired by the user.
>>> In this sense the best to be done is set the bnd registers to allow
>>> access to the whole memory, i.e. initialized state, before pushing the
>>> inferior call.
>> This explains the reason for clearing better ...
> Yes, it was my intention. Do you see value to have the patch in then?

I think I do.

For passing local pointers to some function, it might be
that GDB could be able to figure out which bound registers
contains the bound for a given variable, or if spilled, where
to find then, and set up the call to use the right bounds, but
I have no idea of how to retrieve that information.  I suspect
that it's not a mapping we could retrieve from the dwarf?  And
then there's also the case of passing pointers to global
variables, and pointers to memory that gdb malloc's into the
inferior, like for array/string coercion:

(gdb) p strlen ("hello")

this will alloc a block of memory in the inferior for
"hello", by calling malloc in the inferior.  If strlen
is compiled to do BND checks, would we need to setup the
BND registers to the range of the pointer returned by malloc ?

I've not used BND myself, so I don't have any experience
with it.  But my impression is that disabling BND for
infcalls makes infcalls work again on BND-enabled systems, and
that we could perhaps try to do something smart to re-enable it
in some infcall cases, if there's sufficient benefit.

Now, a question is: could this auto-clearing of BND registers
get in the way of some use cases?  E.g., could a user want to poke
at the BND registers manually before calling a function in the
inferior, in order to debug BND-related code.
If so, that may call for a new option users could tweak
if necessary.

Thanks,
Pedro Alves

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

I just occurred to me that I think it's important that
we test what happens to the BND registers _after_ the
infcall finishes:

 #1 - by normal completion

 #2 - when it is interrupted midway, e.g., because the
      called function trips on a breakpoint.

I didn't find this in the current test.

I expect that in case #1, the BND registers are restored to
what they were before the infcall.  The test should exercise
this, by continuing execution after the infcall returns, and
checking that a bounds violation / lack of bounds violation,
is still detected / not detected as if the infcall didn't
happen.

Similarly, for #2, once gdb "silently stops" after being
resumed from the breakpoint hit, I think all registers are
restored, so the same testing would apply then.

You may also think about testing what should happen
to bounds checks done by the function that had the
breakpoint that interrupted the infcall, if you say, step
over some code inside that interrupted function.

Comprehensive testing like that would make it clearer how
the feature is meant to behave, and ensure it continues
working like that going forward.

Lastly, shouldn't the manual say something about all this?

Thanks,
Pedro Alves

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

Am 2/8/2017 um 4:21 PM schrieb Pedro Alves:
> I just occurred to me that I think it's important that
> we test what happens to the BND registers _after_ the
> infcall finishes:
>
>   #1 - by normal completion
>
>   #2 - when it is interrupted midway, e.g., because the
>        called function trips on a breakpoint.
>
> I didn't find this in the current test.
>
> I expect that in case #1, the BND registers are restored to
> what they were before the infcall.  The test should exercise
> this, by continuing execution after the infcall returns, and
> checking that a bounds violation / lack of bounds violation,
> is still detected / not detected as if the infcall didn't
> happen.
BND registers should be restored to the values they had previous to the 
inferior call.
AFAIR that is the behaviour.

I will change the test to reflect more what you are proposing. Nice 
ideas have to be integrated!
Thanks a lot! :)
> Similarly, for #2, once gdb "silently stops" after being
> resumed from the breakpoint hit, I think all registers are
> restored, so the same testing would apply then.
Yes, will add the test!
> You may also think about testing what should happen
> to bounds checks done by the function that had the
> breakpoint that interrupted the infcall, if you say, step
> over some code inside that interrupted function.
>
> Comprehensive testing like that would make it clearer how
> the feature is meant to behave, and ensure it continues
> working like that going forward.
Agreed! I was considering that was default GDB functionality before.
But it is indeed interesting to test those cases. Being sure that also 
in this context
those functionalities are not broken. Thanks again!
> Lastly, shouldn't the manual say something about all this?
I think so, explaing what is expected for inferior calls in presence of 
MPX in general.
The entry should explain the case that you have pointed out above, right?
> Thanks,
> Pedro Alves
>

Thanks a lot for the valuable contribution,
/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

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

On 02/08/2017 01:27 PM, Pedro Alves wrote:
> On 02/07/2017 08:56 AM, Tedeschi, Walfred wrote:
>> On 01/31/2017 03:13 PM, Walfred Tedeschi wrote:
>>>> This patch initializes the bnd registers before executing the inferior
>>>> call.  BND registers can be in arbitrary values at the moment of the
>>>> inferior call.  In case the function being called uses as part of the
>>>> parameters bnd register, e.g. when passing a pointer as parameter, the
>>>> current value of the register will be used.  This can cause boundary
>>>> violations that are not due to a real bug or even desired by the user.
>>>> In this sense the best to be done is set the bnd registers to allow
>>>> access to the whole memory, i.e. initialized state, before pushing the
>>>> inferior call.
>>> This explains the reason for clearing better ...
>> Yes, it was my intention. Do you see value to have the patch in then?
> I think I do.
At this point i think it is woth to mention a bit more on the usage 
model I had in mind.

At the inferior call time all BND registers are set to the INIT state.
That covers 90% of the usage case and keeps the actual behavior of GDB.
In case it is desirable to investigate any  bound violation user can add 
a breakpoint on the prolog of the function and
set the BND register there, I understand that this is the normal use 
case for the debugger as well.
> For passing local pointers to some function, it might be
> that GDB could be able to figure out which bound registers
> contains the bound for a given variable, or if spilled, where
> to find then, and set up the call to use the right bounds, but
> I have no idea of how to retrieve that information.  I suspect
> that it's not a mapping we could retrieve from the dwarf?  And
> then there's also the case of passing pointers to global
> variables, and pointers to memory that gdb malloc's into the
> inferior, like for array/string coercion:
ABI defines which BND is used for which parameter and what to do when it 
is needed to pass more bounds than BND registers available.

> (gdb) p strlen ("hello")
>
> this will alloc a block of memory in the inferior for
> "hello", by calling malloc in the inferior.  If strlen
> is compiled to do BND checks, would we need to setup the
> BND registers to the range of the pointer returned by malloc ?
Yes we would need to set the BND in GDB in this scenario.
However we could simply let that attribution to the user.
My point of view is that this could complicate a lot the code to cover 
very little use.

>
> I've not used BND myself, so I don't have any experience
> with it.  But my impression is that disabling BND for
> infcalls makes infcalls work again on BND-enabled systems, and
> that we could perhaps try to do something smart to re-enable it
> in some infcall cases, if there's sufficient benefit.
>
> Now, a question is: could this auto-clearing of BND registers
> get in the way of some use cases?  E.g., could a user want to poke
> at the BND registers manually before calling a function in the
> inferior, in order to debug BND-related code.
> If so, that may call for a new option users could tweak
> if necessary.
I was also considering this model as my first implementation.
But this way I suppose that it will be even more complicated.
We will need special commands for the architecture, special documentation...

if we set afterwards: Inferior call + Break point + register set.
we should not need any additional set and show for the architecture.
Hover as you pointed out in the other e-mail it is better to increase 
testing and document it.
> Thanks,
> Pedro Alves
>
Thanks a lot for your review!

/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

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

On 02/13/2017 08:33 AM, Tedeschi, Walfred wrote:

> On 02/08/2017 01:27 PM, Pedro Alves wrote:

>> For passing local pointers to some function, it might be
>> that GDB could be able to figure out which bound registers
>> contains the bound for a given variable, or if spilled, where
>> to find then, and set up the call to use the right bounds, but
>> I have no idea of how to retrieve that information.  I suspect
>> that it's not a mapping we could retrieve from the dwarf?  And
>> then there's also the case of passing pointers to global
>> variables, and pointers to memory that gdb malloc's into the
>> inferior, like for array/string coercion:
> ABI defines which BND is used for which parameter and what to do when it
> is needed to pass more bounds than BND registers available.

Sure, but ABI only specifies calling convention, not
whatever the compiler decides to do inside the function bodies,
right?  Say:

extern void bar (int *ptr);

void foo (int *ptr)
{
  // lots of code code here.
  [...]

  // PTR now lives in memory, or in a different register
  // The corresponding BND register could have been
  // spilled/reused too.

  // do "call bar (ptr)" from the debugger while stopped here.
  // How would GDB determine where the correct corresponding
  // BND value is?
}

I haven't studied the BND documention in detail, but I don't
imagine how the information necessary to be able to answer the
question in the comment above, in the general case, could be
determined from ABI-awareness alone, and I don't believe it's
something that could be retrieved from DWARF either.  But
I'd gladly be shown wrong.

> if we set afterwards: Inferior call + Break point + register set.
> we should not need any additional set and show for the architecture.
> Hover as you pointed out in the other e-mail it is better to increase
> testing and document it.

Agreed, that sounds like a reasonable way to handle that use case.
I think mentioning it in the documentation would be good.

Thanks,
Pedro Alves

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

On 02/13/2017 01:03 PM, Pedro Alves wrote:
> On 02/13/2017 08:33 AM, Tedeschi, Walfred wrote:
>
>> On 02/08/2017 01:27 PM, Pedro Alves wrote:
>>> For passing local pointers to some function, it might be
>>> that GDB could be able to figure out which bound registers
>>> contains the bound for a given variable, or if spilled, where
>>> to find then, and set up the call to use the right bounds, but
>>> I have no idea of how to retrieve that information.  I suspect
>>> that it's not a mapping we could retrieve from the dwarf?  And
>>> then there's also the case of passing pointers to global
>>> variables, and pointers to memory that gdb malloc's into the
>>> inferior, like for array/string coercion:
>> ABI defines which BND is used for which parameter and what to do when it
>> is needed to pass more bounds than BND registers available.
> Sure, but ABI only specifies calling convention, not
> whatever the compiler decides to do inside the function bodies,
> right?  Say:
>
> extern void bar (int *ptr);
>
> void foo (int *ptr)
> {
>    // lots of code code here.
>    [...]
>
>    // PTR now lives in memory, or in a different register
>    // The corresponding BND register could have been
>    // spilled/reused too.
>
>    // do "call bar (ptr)" from the debugger while stopped here.
>    // How would GDB determine where the correct corresponding
>    // BND value is?
> }
>
> I haven't studied the BND documention in detail, but I don't
> imagine how the information necessary to be able to answer the
> question in the comment above, in the general case, could be
> determined from ABI-awareness alone, and I don't believe it's
> something that could be retrieved from DWARF either.  But
> I'd gladly be shown wrong.
You are absolutely right! there is no way to know what goes on with the 
BND register within the code itself.
Internally we have discussed how to represent the bound information on 
DWARF, but for one reason or another we did not
arrive at the point to formalize it and bring it to the committee.
Basically idea was to add as a new DW_AT that would work as a location 
list for the bounds of any type that would point to a DW_TAG_POINTER_TYPE,
DW_TAG_REFERENCE_TYPE or DW_rvalue_reference_type.

We still intend to bring this proposal to the committee.
>> if we set afterwards: Inferior call + Break point + register set.
>> we should not need any additional set and show for the architecture.
>> Hover as you pointed out in the other e-mail it is better to increase
>> testing and document it.
> Agreed, that sounds like a reasonable way to handle that use case.
> I think mentioning it in the documentation would be good.
>
> Thanks,
> Pedro Alves
>
I am working on the tests right now!

Thanks again for your review and best 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

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

On 02/13/2017 09:33 AM, Tedeschi, Walfred wrote:
>
>
> On 02/08/2017 01:27 PM, Pedro Alves wrote:
>> On 02/07/2017 08:56 AM, Tedeschi, Walfred wrote:
>>> On 01/31/2017 03:13 PM, Walfred Tedeschi wrote:
>>>>> This patch initializes the bnd registers before executing the 
>>>>> inferior
>>>>> call.  BND registers can be in arbitrary values at the moment of the
>>>>> inferior call.  In case the function being called uses as part of the
>>>>> parameters bnd register, e.g. when passing a pointer as parameter, 
>>>>> the
>>>>> current value of the register will be used.  This can cause boundary
>>>>> violations that are not due to a real bug or even desired by the 
>>>>> user.
>>>>> In this sense the best to be done is set the bnd registers to allow
>>>>> access to the whole memory, i.e. initialized state, before pushing 
>>>>> the
>>>>> inferior call.
>>>> This explains the reason for clearing better ...
>>> Yes, it was my intention. Do you see value to have the patch in then?
>> I think I do.
> At this point i think it is woth to mention a bit more on the usage 
> model I had in mind.
>
> At the inferior call time all BND registers are set to the INIT state.
> That covers 90% of the usage case and keeps the actual behavior of GDB.
> In case it is desirable to investigate any  bound violation user can 
> add a breakpoint on the prolog of the function and
> set the BND register there, I understand that this is the normal use 
> case for the debugger as well.
In reality it did not work as expected. When an inferior call is done 
and there is a breakpoint in the executed GDB issues a message like:

"The program being debugged stopped while in a function called from GDB.
Evaluation of the expression containing the function
(foo) will be abandoned.
When the function is done executing, GDB will silently stop."

Also after that no signal generated within the function call is reported.
Not sure if this a limitation or a bug. In case you think it is a bug i 
can add some entries in Bugzilla.

On the other hand i see then the following option:
Add the code to INIT the BND regs based on a set/show option.
In case not set user will have to take care of setting and restoring the 
register himself.

As optimal solution we could also provide 4 debugger variables, or 
convenience variables to keep the register values chosen by the user.
Use those to set the registers before the inferior call.
  I have the impression that this option is an overkill.

Would you agree with the creation of the new set and show variable to 
INIT the BND registers before the inferior call?


Thanks again and best regards,
/Fred

>> For passing local pointers to some function, it might be
>> that GDB could be able to figure out which bound registers
>> contains the bound for a given variable, or if spilled, where
>> to find then, and set up the call to use the right bounds, but
>> I have no idea of how to retrieve that information.  I suspect
>> that it's not a mapping we could retrieve from the dwarf?  And
>> then there's also the case of passing pointers to global
>> variables, and pointers to memory that gdb malloc's into the
>> inferior, like for array/string coercion:
> ABI defines which BND is used for which parameter and what to do when 
> it is needed to pass more bounds than BND registers available.
>
>> (gdb) p strlen ("hello")
>>
>> this will alloc a block of memory in the inferior for
>> "hello", by calling malloc in the inferior.  If strlen
>> is compiled to do BND checks, would we need to setup the
>> BND registers to the range of the pointer returned by malloc ?
> Yes we would need to set the BND in GDB in this scenario.
> However we could simply let that attribution to the user.
> My point of view is that this could complicate a lot the code to cover 
> very little use.
>
>>
>> I've not used BND myself, so I don't have any experience
>> with it.  But my impression is that disabling BND for
>> infcalls makes infcalls work again on BND-enabled systems, and
>> that we could perhaps try to do something smart to re-enable it
>> in some infcall cases, if there's sufficient benefit.
>>
>> Now, a question is: could this auto-clearing of BND registers
>> get in the way of some use cases?  E.g., could a user want to poke
>> at the BND registers manually before calling a function in the
>> inferior, in order to debug BND-related code.
>> If so, that may call for a new option users could tweak
>> if necessary.
> I was also considering this model as my first implementation.
> But this way I suppose that it will be even more complicated.
> We will need special commands for the architecture, special 
> documentation...
>
> if we set afterwards: Inferior call + Break point + register set.
> we should not need any additional set and show for the architecture.
> Hover as you pointed out in the other e-mail it is better to increase 
> testing and document it.
>> Thanks,
>> Pedro Alves
>>
> Thanks a lot for your review!
>
> /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

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

On 02/14/2017 01:34 PM, Tedeschi, Walfred wrote:
> On 02/13/2017 09:33 AM, Tedeschi, Walfred wrote:
>> At the inferior call time all BND registers are set to the INIT state.
>> That covers 90% of the usage case and keeps the actual behavior of GDB.
>> In case it is desirable to investigate any  bound violation user can
>> add a breakpoint on the prolog of the function and
>> set the BND register there, I understand that this is the normal use
>> case for the debugger as well.
> In reality it did not work as expected. When an inferior call is done
> and there is a breakpoint in the executed GDB issues a message like:
> 
> "The program being debugged stopped while in a function called from GDB.
> Evaluation of the expression containing the function
> (foo) will be abandoned.
> When the function is done executing, GDB will silently stop."

This looks normal.  What were you expecting instead?

> Also after that no signal generated within the function call is reported.

Please clarify.

> Not sure if this a limitation or a bug. In case you think it is a bug i
> can add some entries in Bugzilla.

Thanks,
Pedro Alves

RE: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Tuesday, February 14, 2017 1:59 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; qiyaoltc@gmail.com; brobecker@adacore.com
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

Hello Pedro,


On 02/14/2017 01:34 PM, Tedeschi, Walfred wrote:
> On 02/13/2017 09:33 AM, Tedeschi, Walfred wrote:
>> At the inferior call time all BND registers are set to the INIT state.
>> That covers 90% of the usage case and keeps the actual behavior of GDB.
>> In case it is desirable to investigate any  bound violation user can 
>> add a breakpoint on the prolog of the function and set the BND 
>> register there, I understand that this is the normal use case for the 
>> debugger as well.
> In reality it did not work as expected. When an inferior call is done 
> and there is a breakpoint in the executed GDB issues a message like:
> 
> "The program being debugged stopped while in a function called from GDB.
> Evaluation of the expression containing the function
> (foo) will be abandoned.
> When the function is done executing, GDB will silently stop."

This looks normal.  What were you expecting instead?

--
Fred: 
Message per se is fine. I was expecting that the inferior call should run isolated, exemplifying:

1. Inferior is stopped.
2. inferior call is done.
3. breakpoint in inferior call is triggered.
4. continue is issued.
5. signal is presented to the user.
6. continue is issued again.
7. expected - control should be back to 1, i.e. on stop mode.
7. actual behavior - application finishes with the signal
8. actual behavior - after the breakpoint no result will be displayed to the user (about the inf call)

In 6 if I use return instead I fall back to my expected behavior. In this sense, I think the user can work in that way.

I will continue a bit more testing now. I have got confused with an intermediate version 01/27/2017.
It was not displaying the signal step (5), now it is on again. 
--

> Also after that no signal generated within the function call is reported.

Please clarify.

Fred: I tried to summarize above, not sure I succeeded. 

Fred: Additional comment:
Trying that now I am considering that this as a whole is more complicated than expected. 
Too many actions to realize the simple thing that is set a register before an inferior call and perform the call.

I am inclined to give it a try in creating set/show for the feature and see how it behaves. 
In fact I will do it and submit the V8 based on that let's see how it looks like.

Thanks a lot for the review!

Best regards,
/Fred

> Not sure if this a limitation or a bug. In case you think it is a bug 
> i can add some entries in Bugzilla.

Thanks,
Pedro Alves

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

On 02/15/2017 01:01 PM, Tedeschi, Walfred wrote:

> Pedro wrote:
>> In reality it did not work as expected. When an inferior call is done 
>> and there is a breakpoint in the executed GDB issues a message like:
>>
>> "The program being debugged stopped while in a function called from GDB.
>> Evaluation of the expression containing the function
>> (foo) will be abandoned.
>> When the function is done executing, GDB will silently stop."
> 
> This looks normal.  What were you expecting instead?
> 
> --
> Fred: 
> Message per se is fine. I was expecting that the inferior call should run isolated, exemplifying:
> 
> 1. Inferior is stopped.
> 2. inferior call is done.
> 3. breakpoint in inferior call is triggered.
> 4. continue is issued.
> 5. signal is presented to the user.
> 6. continue is issued again.
> 7. expected - control should be back to 1, i.e. on stop mode.
> 7. actual behavior - application finishes with the signal
> 8. actual behavior - after the breakpoint no result will be displayed to the user (about the inf call)

Just to be clear, I assume that between 3 and 4 there was
a 3.5 step:

  3.5. poke bnd registers manually, such that the infcall
       crashes with a bounds violation.

and that in step "5.", the signal you mention is a SIGSEGV
with bound violation info.

Correct?

I'm not seeing how an option to clear/not clear the bnd
registers changes anything for the parts you found unexpected.
All steps from 5. onward will be the same, won't they?

To not pass the SIGSEGV to the inferior, you can also
resume with "signal 0", or do "handle SIGSEGV nopass" before
the continue.

As for 8. being silent, that's something that I think would
be nice to change, though that's not specific to bnd at
all.

Thanks,
Pedro Alves

> In 6 if I use return instead I fall back to my expected behavior. In this sense, I think the user can work in that way.
> 
> I will continue a bit more testing now. I have got confused with an intermediate version 01/27/2017.
> It was not displaying the signal step (5), now it is on again. 
> --
> 
>> Also after that no signal generated within the function call is reported.
> 
> Please clarify.
> 
> Fred: I tried to summarize above, not sure I succeeded. 
> 
> Fred: Additional comment:
> Trying that now I am considering that this as a whole is more complicated than expected. 
> Too many actions to realize the simple thing that is set a register before an inferior call and perform the call.
> 
> I am inclined to give it a try in creating set/show for the feature and see how it behaves. 
> In fact I will do it and submit the V8 based on that let's see how it looks like.
> 
> Thanks a lot for the review!
> 
> Best regards,
> /Fred
> 
>> Not sure if this a limitation or a bug. In case you think it is a bug 
>> i can add some entries in Bugzilla.
> 
> Thanks,
> Pedro Alves

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

Hello Pedro,


On 02/15/2017 02:15 PM, Pedro Alves wrote:
> On 02/15/2017 01:01 PM, Tedeschi, Walfred wrote:
>
>> Pedro wrote:
>>> In reality it did not work as expected. When an inferior call is done
>>> and there is a breakpoint in the executed GDB issues a message like:
>>>
>>> "The program being debugged stopped while in a function called from GDB.
>>> Evaluation of the expression containing the function
>>> (foo) will be abandoned.
>>> When the function is done executing, GDB will silently stop."
>> This looks normal.  What were you expecting instead?
>>
>> --
>> Fred:
>> Message per se is fine. I was expecting that the inferior call should run isolated, exemplifying:
>>
>> 1. Inferior is stopped.
>> 2. inferior call is done.
>> 3. breakpoint in inferior call is triggered.
>> 4. continue is issued.
>> 5. signal is presented to the user.
>> 6. continue is issued again.
>> 7. expected - control should be back to 1, i.e. on stop mode.
>> 7. actual behavior - application finishes with the signal
>> 8. actual behavior - after the breakpoint no result will be displayed to the user (about the inf call)
> Just to be clear, I assume that between 3 and 4 there was
> a 3.5 step:
>
>    3.5. poke bnd registers manually, such that the infcall
>         crashes with a bounds violation.
>
> and that in step "5.", the signal you mention is a SIGSEGV
> with bound violation info.
>
> Correct?
Yes.
But actual behavior at 7 has an issue!

When we set the BND registers from gdb itself (applying the patch) it 
looks like changing the values of BND again while in the prolog have no 
effect.
Lets go to the reproducer:

The inferior call i want to do is "upper (x, a, b, c, d, 100)".
it has the following relevant prolog:

   0x0000000000400a0b <+1>:    mov    %rsp,%rbp
    0x0000000000400a0e <+4>:    sub    $0x18,%rsp
    0x0000000000400a12 <+8>:    mov    %rdi,-0x18(%rbp)
    0x0000000000400a16 <+12>:    mov    %rsi,-0x20(%rbp)
    0x0000000000400a1a <+16>:    mov    %rdx,-0x28(%rbp)
    0x0000000000400a1e <+20>:    mov    %rcx,-0x30(%rbp)
    0x0000000000400a22 <+24>:    mov    %r8,-0x38(%rbp)
    0x0000000000400a26 <+28>:    mov    %r9d,-0x3c(%rbp)
    0x0000000000400a2a <+32>:    bndmov %bnd0,-0x50(%rbp)
    0x0000000000400a2f <+37>:    bndmov %bnd1,-0x60(%rbp)
    0x0000000000400a34 <+42>:    bndmov %bnd2,-0x70(%rbp)
    0x0000000000400a39 <+47>:    bndmov %bnd3,-0x80(%rbp)

I can stop at the first instruction of "upper" by issuing b (void*)&upper.
In order to verify the effective change in the BND i have printed 
bnd0..bnd3. Register values were same as entered with the GDB command.
Other way is to do instruction stepping till " bndmov %bnd3,-0x80(%rbp)" 
and examine the memory at the indicated places.

Surprise! In the gdb i have applied the patch though changing the 
BND0..BND3 values at 0x0400a0b value present on memory was still set to 
the init state.
In the version without applying the patch it i could see the value i 
entered while stopped at the first instruction.

I am assuming that this is a problem or i am wrong?



> I'm not seeing how an option to clear/not clear the bnd
> registers changes anything for the parts you found unexpected.
> All steps from 5. onward will be the same, won't they?
Message would already scare the user at first and the absence of output 
would cause confusion. It caused in me.
Option to set or not will let it more clear. However there was an 
additional fact in my confusion caused by what i described above.
> To not pass the SIGSEGV to the inferior, you can also
> resume with "signal 0", or do "handle SIGSEGV nopass" before
> the continue.
Yes, my thought was only if it should assume that automatically or not 
while performing an inferior call.
On the other hand too many automatism will also cause issues! :)

> As for 8. being silent, that's something that I think would
> be nice to change, though that's not specific to bnd at
> all.
I agree! It would need to investigate though!
> Thanks,
> Pedro Alves
>
>> In 6 if I use return instead I fall back to my expected behavior. In this sense, I think the user can work in that way.
>>
>> I will continue a bit more testing now. I have got confused with an intermediate version 01/27/2017.
>> It was not displaying the signal step (5), now it is on again.
>> --
>>
>>> Also after that no signal generated within the function call is reported.
>> Please clarify.
>>
>> Fred: I tried to summarize above, not sure I succeeded.
>>
>> Fred: Additional comment:
>> Trying that now I am considering that this as a whole is more complicated than expected.
>> Too many actions to realize the simple thing that is set a register before an inferior call and perform the call.
>>
>> I am inclined to give it a try in creating set/show for the feature and see how it behaves.
>> In fact I will do it and submit the V8 based on that let's see how it looks like.
>>
>> Thanks a lot for the review!
>>
>> Best regards,
>> /Fred
>>
>>> Not sure if this a limitation or a bug. In case you think it is a bug
>>> i can add some entries in Bugzilla.
>> Thanks,
>> Pedro Alves
Thanks again!
/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

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

On 02/16/2017 01:49 PM, Tedeschi, Walfred wrote:

>> Correct?
> Yes.
> But actual behavior at 7 has an issue!

7. is:

>>> 7. expected - control should be back to 1, i.e. on stop mode.
>>> 7. actual behavior - application finishes with the signal

But the rest of your email doesn't talk about this at all.
I'm confused....

> 
> When we set the BND registers from gdb itself (applying the patch) it
> looks like changing the values of BND again while in the prolog have no
> effect.
> Lets go to the reproducer:
> 
> The inferior call i want to do is "upper (x, a, b, c, d, 100)".
> it has the following relevant prolog:
> 
>   0x0000000000400a0b <+1>:    mov    %rsp,%rbp
>    0x0000000000400a0e <+4>:    sub    $0x18,%rsp
>    0x0000000000400a12 <+8>:    mov    %rdi,-0x18(%rbp)
>    0x0000000000400a16 <+12>:    mov    %rsi,-0x20(%rbp)
>    0x0000000000400a1a <+16>:    mov    %rdx,-0x28(%rbp)
>    0x0000000000400a1e <+20>:    mov    %rcx,-0x30(%rbp)
>    0x0000000000400a22 <+24>:    mov    %r8,-0x38(%rbp)
>    0x0000000000400a26 <+28>:    mov    %r9d,-0x3c(%rbp)
>    0x0000000000400a2a <+32>:    bndmov %bnd0,-0x50(%rbp)
>    0x0000000000400a2f <+37>:    bndmov %bnd1,-0x60(%rbp)
>    0x0000000000400a34 <+42>:    bndmov %bnd2,-0x70(%rbp)
>    0x0000000000400a39 <+47>:    bndmov %bnd3,-0x80(%rbp)
> 
> I can stop at the first instruction of "upper" by issuing b (void*)&upper.

FYI, the usual way to do that is with "b *upper".

> In order to verify the effective change in the BND i have printed
> bnd0..bnd3. Register values were same as entered with the GDB command.

printed how?  and printed when exactly?

> Other way is to do instruction stepping till " bndmov %bnd3,-0x80(%rbp)"
> and examine the memory at the indicated places.

Memory?  I thought you'd examine the registers?  What indicated
places, BTW?

> 
> Surprise! In the gdb i have applied the patch though changing the
> BND0..BND3 values at 0x0400a0b value present on memory was still set to
> the init state.

_memory_ set to the init state?

Can you please explain what you're seeing in a bit more detail?
You're leaving out details I'm finding myself needing to guess,
and I'd probably guess wrong.

But still, if I have to guess, I'd think that the problem with
stopping at function entry and poking the bnd registers
_before_ the prologue runs, would be that whatever bnd register value
you patch in, would be overridden by the bndmov instructions in the
prologue.  I.e., you need to single-step past those bndmov
instructions, and patch the bnd registers _then_, otherwise
the bndmovs undo your patching.

But this comment:

> In the version without applying the patch it i could see the value i
> entered while stopped at the first instruction.

... seems to contradict that.  So I'm double confused.

Still, I don't see what does this have to do with point 7.

Thanks,
Pedro Alves

RE: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Thursday, February 16, 2017 2:52 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; qiyaoltc@gmail.com; brobecker@adacore.com
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

On 02/16/2017 01:49 PM, Tedeschi, Walfred wrote:

>> Correct?
> Yes.
> But actual behavior at 7 has an issue!

7. is:

>>> 7. expected - control should be back to 1, i.e. on stop mode.
>>> 7. actual behavior - application finishes with the signal

But the rest of your email doesn't talk about this at all.
I'm confused....

FRED:
Issue is that I forgot to say in here that segv is not presented when we set the BND register in the prolog.
(The manual set is meant) 
I intended to force a boundary violation. But did not work.

> 
> When we set the BND registers from gdb itself (applying the patch) it 
> looks like changing the values of BND again while in the prolog have 
> no effect.
> Lets go to the reproducer:
> 
> The inferior call i want to do is "upper (x, a, b, c, d, 100)".
> it has the following relevant prolog:
> 
>   0x0000000000400a0b <+1>:    mov    %rsp,%rbp
>    0x0000000000400a0e <+4>:    sub    $0x18,%rsp
>    0x0000000000400a12 <+8>:    mov    %rdi,-0x18(%rbp)
>    0x0000000000400a16 <+12>:    mov    %rsi,-0x20(%rbp)
>    0x0000000000400a1a <+16>:    mov    %rdx,-0x28(%rbp)
>    0x0000000000400a1e <+20>:    mov    %rcx,-0x30(%rbp)
>    0x0000000000400a22 <+24>:    mov    %r8,-0x38(%rbp)
>    0x0000000000400a26 <+28>:    mov    %r9d,-0x3c(%rbp)
>    0x0000000000400a2a <+32>:    bndmov %bnd0,-0x50(%rbp)
>    0x0000000000400a2f <+37>:    bndmov %bnd1,-0x60(%rbp)
>    0x0000000000400a34 <+42>:    bndmov %bnd2,-0x70(%rbp)
>    0x0000000000400a39 <+47>:    bndmov %bnd3,-0x80(%rbp)
> 
> I can stop at the first instruction of "upper" by issuing b (void*)&upper.

FYI, the usual way to do that is with "b *upper".


> In order to verify the effective change in the BND i have printed 
> bnd0..bnd3. Register values were same as entered with the GDB command.

printed how?  and printed when exactly?

FRED:
So, stopped at 0x0000000000400a0b set BND0 and did a step instruction and printed BND0
Commands were: 
P $bnd0={0xFFFFF..., 0} 
Si
P $bnd0 (output was the same as input)

For the value I set I should have got an segv. But I did not. 
> Other way is to do instruction stepping till " bndmov %bnd3,-0x80(%rbp)"
> and examine the memory at the indicated places.

Memory?  I thought you'd examine the registers?  What indicated places, BTW?

FRED:
Yes, tring to discover what happened to the set I stepped instructions till 0x0000000000400a2f
This tells me that the BND0 should be at  0x50(%rbp) but value was not the one I added by hand but the automatically set (with the patch)
Interestingly without the patch, and repeating the same procedure, the value is the one I have set by hand while stopped at the prolog.

 
> 
> Surprise! In the gdb i have applied the patch though changing the
> BND0..BND3 values at 0x0400a0b value present on memory was still set 
> to the init state.

_memory_ set to the init state?

FRED:
X /4w ($rbp -0x50)

When stopped at 0x0000000000400a2f we should see the value set of $BND0 what we see is the automatic set value and not the value I set when at the prolog.

Pedro:
Can you please explain what you're seeing in a bit more detail?
You're leaving out details I'm finding myself needing to guess, and I'd probably guess wrong.

But still, if I have to guess, I'd think that the problem with stopping at function entry and poking the bnd registers _before_ the prologue runs, would be that whatever bnd register value you patch in, would be overridden by the bndmov instructions in the prologue.  I.e., you need to single-step past those bndmov instructions, and patch the bnd registers _then_, otherwise the bndmovs undo your patching.


But this comment:

> In the version without applying the patch it i could see the value i 
> entered while stopped at the first instruction.

... seems to contradict that.  So I'm double confused.

Still, I don't see what does this have to do with point 7.

Thanks,
Pedro Alves


Fred:
Thanks a lot again! I hope I could get the message trough now. With memory registers set and not set the setup was a bit complicated in case it is still not clear, I will send the full log.

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

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves]

On 02/16/2017 03:36 PM, Tedeschi, Walfred wrote:

> FRED:
> So, stopped at 0x0000000000400a0b set BND0 and did a step instruction and printed BND0
> Commands were: 
> P $bnd0={0xFFFFF..., 0} 
> Si
> P $bnd0 (output was the same as input)

That seems expected.

> 
> For the value I set I should have got an segv. But I did not. 
>> Other way is to do instruction stepping till " bndmov %bnd3,-0x80(%rbp)"
>> and examine the memory at the indicated places.
> 
> Memory?  I thought you'd examine the registers?  What indicated places, BTW?
> 
> FRED:
> Yes, tring to discover what happened to the set I stepped instructions till 0x0000000000400a2f
> This tells me that the BND0 should be at  0x50(%rbp)

Ah, yes I see what you mean now.  Well, actually it should be
at -0x50(%rbp) [negative].  Now I don't know whether that
was just a typo.  :-P  Otherwise something's very odd indeed.

> but value was not the one I added by hand but the automatically set (with the patch)
> Interestingly without the patch, and repeating the same procedure, the value is the one I have set by hand while stopped at the prolog.

Could your patch be clearing something that disables bnd-related
instructions completely?  Perhaps %bndcfgu?

In your patch you have:

      for (int i = 0; i < I387_BND0R_REGNUM (tdep); i++)
	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);

What does the "I387_BND0R_REGNUM (tdep)" in the for loop
condition mean?

That should have been "the number of bnd registers" I suppose, but
doesn't look like that's what the code is really doing, as
I387_BND0R_REGNUM is really the number of the _first_ bnd register,
AFAICS.  Shouldn't that be I387_NUM_BND_REGS instead, like:

      for (int i = 0; i < I387_NUM_BND_REGS; i++)
	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);

?

Thanks,
Pedro Alves

Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Tedeschi, Walfred]

Am 2/16/2017 um 4:15 PM schrieb Pedro Alves:
> On 02/16/2017 03:36 PM, Tedeschi, Walfred wrote:
>
>> FRED:
>> So, stopped at 0x0000000000400a0b set BND0 and did a step instruction and printed BND0
>> Commands were:
>> P $bnd0={0xFFFFF..., 0}
>> Si
>> P $bnd0 (output was the same as input)
> That seems expected.
>
>> For the value I set I should have got an segv. But I did not.
>>> Other way is to do instruction stepping till " bndmov %bnd3,-0x80(%rbp)"
>>> and examine the memory at the indicated places.
>> Memory?  I thought you'd examine the registers?  What indicated places, BTW?
>>
>> FRED:
>> Yes, tring to discover what happened to the set I stepped instructions till 0x0000000000400a2f
>> This tells me that the BND0 should be at  0x50(%rbp)
> Ah, yes I see what you mean now.  Well, actually it should be
> at -0x50(%rbp) [negative].  Now I don't know whether that
> was just a typo.  :-P  Otherwise something's very odd indeed.
>
>> but value was not the one I added by hand but the automatically set (with the patch)
>> Interestingly without the patch, and repeating the same procedure, the value is the one I have set by hand while stopped at the prolog.
> Could your patch be clearing something that disables bnd-related
> instructions completely?  Perhaps %bndcfgu?
>
> In your patch you have:
>
>        for (int i = 0; i < I387_BND0R_REGNUM (tdep); i++)
> 	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
It could be let me check! This loop really looks odd.

> What does the "I387_BND0R_REGNUM (tdep)" in the for loop
> condition mean?
>
> That should have been "the number of bnd registers" I suppose, but
> doesn't look like that's what the code is really doing, as
> I387_BND0R_REGNUM is really the number of the _first_ bnd register,
> AFAICS.  Shouldn't that be I387_NUM_BND_REGS instead, like:
>
>        for (int i = 0; i < I387_NUM_BND_REGS; i++)
> 	regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf);
>
> ?

indeed! That was the issue.
Now it works...

I will prepare tests and doc now!

Thanks a lot indeed!
/Fred

>
> Thanks,
> Pedro Alves
>

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928