From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121865 invoked by alias); 6 Feb 2017 17:05:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 121443 invoked by uid 89); 6 Feb 2017 17:05:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.5 required=5.0 tests=AWL,BAYES_50,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=9999, explaining, violations, *function X-HELO: mga01.intel.com Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 06 Feb 2017 17:05:07 +0000 Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP; 06 Feb 2017 09:05:05 -0800 X-ExtLoop1: 1 Received: from ullv-win8-gdb-jenkins.wtedesch-mobl (HELO [172.28.205.157]) ([172.28.205.157]) by fmsmga006.fm.intel.com with ESMTP; 06 Feb 2017 09:05:04 -0800 Subject: [ping] [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls. To: palves@redhat.com, qiyaoltc@gmail.com, brobecker@adacore.com References: <1485875613-31975-1-git-send-email-walfred.tedeschi@intel.com> Cc: gdb-patches@sourceware.org From: "Tedeschi, Walfred" Message-ID: <4e97dc69-7058-5da2-6f7e-d7ae615b0b34@intel.com> Date: Mon, 06 Feb 2017 17:05:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1485875613-31975-1-git-send-email-walfred.tedeschi@intel.com> Content-Type: text/plain; charset="iso-8859-15"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00125.txt.bz2 Hello all, Do you see value on this this patch or you consider that the user has to=20 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=20 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 > > 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, struc= t value *function, > enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > gdb_byte buf[8]; >=20=20=20 > + /* Reset bound registers. */ > + i387_reset_bnd_regs (gdbarch, regcache); > + > /* Pass arguments. */ > sp =3D amd64_push_arguments (regcache, nargs, args, sp, struct_return= ); >=20=20=20 > 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, stru= ct value *function, > int write_pass; > int args_space =3D 0; >=20=20=20 > + /* 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), 0x3ff= f); >=20=20=20 > } > + > + /* 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 =3D gdbarch_tdep (gdbarch); > + > + if (I387_BND0R_REGNUM (tdep) > 0) > + { > + gdb_byte bnd_buf[16]; > + > + memset (bnd_buf, 0, 16); > + for (int i =3D 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); >=20=20=20 > +/* 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.a= rch/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 .= */ > + > +#include > +#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) =3D=3D bit_OSXSAVE) > + { > + if (__get_cpuid_max (0, NULL) < 7) > + return 0; > + > + __cpuid_count (7, 0, eax, ebx, ecx, edx); > + > + if ((ebx & bit_MPX) =3D=3D 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 =3D *(p + len); > + value =3D *(a + len); > + value =3D *(b + len); > + value =3D *(c + len); > + value =3D *(d + len); > + > + value =3D value - value + 1; > + return value; > +} > + > +int * > +upper_ptr (int *p, int *a, int *b, int *c, int *d, int len) > +{ > + int value; > + > + value =3D *(p + len); > + value =3D *(a + len); > + value =3D *(b + len); > + value =3D *(c + len); > + value =3D *(d + len); > + free (p); > + p =3D 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 =3D calloc (OUR_SIZE, sizeof (int)); > + a =3D calloc (OUR_SIZE, sizeof (int)); > + b =3D calloc (OUR_SIZE, sizeof (int)); > + c =3D calloc (OUR_SIZE, sizeof (int)); > + d =3D calloc (OUR_SIZE, sizeof (int)); > + > + upper (sx, sa, sb, sc, sd, 0); /* bkpt 1. */ > + x =3D 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 . > + > + > +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=3D${comp_flags}]] } { > + return -1 > +} > + > +if ![runto_main] { > + untested "could not run to main" > + return -1 > +} > + > +gdb_test_multiple "print have_mpx ()" "have mpx" { > + -re ".*=3D 1\r\n$gdb_prompt " { > + pass "check whether processor supports MPX" > + } > + -re ".*=3D 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 " =3D \\\{lbound =3D $hex, ubound =3D $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)" " =3D 1"\ > + "test the call of int function - int" > +gdb_test "p upper_ptr (x, a, b, c, d, 0)"\ > + " =3D \\\(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