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"
+