From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89853 invoked by alias); 8 Feb 2016 12:55:02 -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 89341 invoked by uid 89); 8 Feb 2016 12:55:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:ip*10.7.209.18, Small, pest, H*RU:HELO X-HELO: mga11.intel.com Received: from mga11.intel.com (HELO mga11.intel.com) (192.55.52.93) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Feb 2016 12:54:58 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP; 08 Feb 2016 04:54:57 -0800 X-ExtLoop1: 1 Received: from wtedesch-mobl2.ger.corp.intel.com (HELO [172.28.205.68]) ([172.28.205.68]) by orsmga001.jf.intel.com with ESMTP; 08 Feb 2016 04:54:55 -0800 Subject: Re: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls. To: Joel Brobecker References: <1452688799-15633-1-git-send-email-walfred.tedeschi@intel.com> <1452688799-15633-3-git-send-email-walfred.tedeschi@intel.com> <20160207070628.GB15342@adacore.com> Cc: eliz@gnu.org, gdb-patches@sourceware.org From: Walfred Tedeschi Message-ID: <56B8901F.9070508@intel.com> Date: Mon, 08 Feb 2016 12:55:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160207070628.GB15342@adacore.com> Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00191.txt.bz2 Am 2/7/2016 um 8:06 AM schrieb Joel Brobecker: > On Wed, Jan 13, 2016 at 01:39:59PM +0100, Walfred Tedeschi wrote: >> When using the return command, execution of a function is aborted >> and present values are returned from that point. That can cause >> bound violations in the MPX context. To avoid such side-effects > > comma at the end of this line: "To avoid such side-effects,"... > >> a new set variable was added "mpx-bnd-init-on-return" which controls > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > a new setting ("mpx-bnd-init-on-return") was added to control > >> the initialization of bound register when using the return command. >> >> As bound initialization it is understood the set of the BND register >> to zero allowing the associated pointer to access the whole memory. > > This sentence is not correct English, I don't think. I think you can > remove it entirely if you modify the next sentences as follow (note, > this is a boolean setting, so use "true" or "false" to describe its > value rather than numeric values)... > >> As default the value of "mpx-bnd-init-on-return" is set to 1. So >> bound register are initilized when using the "return" command. > > If set to True (the default), all bound registers are set to zero > when using the "return" command. > >> 2016-01-13 Walfred Tedeschi >> >> * i386-tdep.c (mpx_bnd_init_on_return): new external variable. >> (show_mpx_init_on_return): New function. >> (_initialize_i386_tdepamd64_init_abi): Add new commands set/show >> mpx-bnd-init-on-return. >> * amd64-tdep.c (amd64_return_value): Use mpx-bnd-init-on-return. >> * NEWS: Add entry for set/show mpx-bnd-init-on-return command. >> >> gdb/doc/ChangeLog: >> >> * gdb.texinfo (Intel Memory Protection Extensions): Add >> documentation on using set/show mpx-bnd-init-on-return. >> >> gdb/testsuite/ChangeLog: >> >> * amd64-mpx-call.c: New file. >> * amd64-mpx-call.exp: New file. >> >> --- >> gdb/NEWS | 4 + >> gdb/amd64-tdep.c | 32 ++++++++ >> gdb/doc/gdb.texinfo | 12 +++ >> gdb/i386-tdep.c | 27 +++++++ >> gdb/testsuite/gdb.arch/amd64-mpx-call.c | 126 ++++++++++++++++++++++= ++++++++ >> gdb/testsuite/gdb.arch/amd64-mpx-call.exp | 90 +++++++++++++++++++++ >> 6 files changed, 291 insertions(+) >> create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c >> create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp >> >> diff --git a/gdb/NEWS b/gdb/NEWS >> index a222dfb..80022ec 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -3,6 +3,10 @@ >> >> *** Changes since GDB 7.10 >> >> +* show mpx-bnd-init-on-return >> + set mpx-bnd-init-on-return on i386 and amd64 >> + Support for set bound registers to INIT state when using the command= return. >> + >> * Record btrace now supports non-stop mode. >> >> * Support for tracepoints on aarch64-linux was added in GDBserver. >> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c >> index 732e91b..0ce41d1 100644 >> --- a/gdb/amd64-tdep.c >> +++ b/gdb/amd64-tdep.c >> @@ -708,12 +708,14 @@ amd64_return_value (struct gdbarch *gdbarch, struc= t value *function, >> gdb_byte *readbuf, const gdb_byte *writebuf) >> { >> enum amd64_reg_class theclass[2]; >> + struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); >> int len =3D TYPE_LENGTH (type); >> static int integer_regnum[] =3D { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM= }; >> static int sse_regnum[] =3D { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM }; >> int integer_reg =3D 0; >> int sse_reg =3D 0; >> int i; >> + extern int mpx_bnd_init_on_return; > > Please avoid these kinds of declarations like the pest. Normally, > what we should be doing to prevent this is to declare this extern > in a .h file, and then include that .h file from all the .c file > referencing that global. This way, the compiler can keep all the > uses consistent. Otherwise, some can change the type of that > variable and the compiler would compile the above incorrectly. > > In this case, since this setting is only accessed from amd64-tdep, > I suggest you move this global, as well as the code creating the new > settting, inside amd64-tdep.c. This will avoid the need for an extern. > Please make mpx_bnd_init_on_return static as well. > >> + if the return value we are setting is due to the use of the >> + "return" command (WRITEBUF is not NULL), it is likely that >> + the return is made prematurely, and therefore bnd0 register >> + unset could then cause the program to receive a spurious >> + bound violation, but this is only done if >> + "set mpx-bnd-init-on-return" is true. */ > > Small nit (reminder): All sentences should start with a capital letter. > Thus "If the...". > > I think some bits were lost in translation, though. Please replace > the text above by the following: > > If the return value we are setting is due to the use of the > "return" command (WRITEBUF is not NULL), it is likely that > the return is made prematurely. In that situation, it is > possible that the bnd0 register could still be unset, thus > causing the program to receive a spurious bound violation. > > When "set mpx-bnd-init-on-return" is true, prevent this > from happening by setting the bnd0 register to zero. */ > > >> + if (writebuf !=3D NULL >> + && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0) >> + { >> + gdb_byte bnd_buf[16]; >> + >> + memset (bnd_buf, 0, 16); >> + regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep), bnd_buf); >> + } >> } >> >> return RETURN_VALUE_ABI_RETURNS_ADDRESS; >> @@ -833,6 +854,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct= value *function, >> writebuf + i * 8); >> } >> >> + if (I387_BND0R_REGNUM (tdep) > 0) >> + { >> + gdb_byte bnd_buf[16]; >> + int i, init_bnd; >> + >> + memset (bnd_buf, 0, 16); >> + if (writebuf && mpx_bnd_init_on_return) >> + for (i =3D 0; i < I387_NUM_BND_REGS; i++) >> + regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf); >> + } > > I suggest you use the same pattern as above (move the "if (writebuf > && mpx_bnd_init_on_return)" to the first condition. It will be more > consistent with the other block of code you added, making it easier > to understand, and will also avoid a call to memset for nothing if > mpx_bnd_init_on_return is unset. Please also add a small comment > explaining why you do this. Thus: > > /* Same as in the class AMD64_MEMORY case above, where > we set bnd0 to zero when doing a premature "return" > and "set mpx-bnd-init-on-return" is true: Set all bound > registers to zero in order to avoid spurious bound > violations. */ > > if (writebuf !=3D NULL > && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0) > { > ... > } > >> +/* The value of the "set mpx-bnd-init-on-return setting. */ >> + >> +int mpx_bnd_init_on_return =3D 1; >> + >> +/* Show state of the setting variable mpx-bnd-init-on-return. */ >> + >> +static void >> +show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty, >> + struct cmd_list_element *c, const char *value) >> +{ >> + if (mpx_bnd_init_on_return) >> + fprintf_filtered (file, >> + _("BND registers will be initialized on return.\n")); >> + else >> + fprintf_filtered (file, >> + _("BND registers will not be initialized on return.\n")); >> +} >> + >> static struct cmd_list_element *mpx_set_cmdlist, *mpx_show_cmdlist; >> >> /* Helper function for the CLI commands. */ >> @@ -8940,6 +8958,15 @@ Show Intel(R) Memory Protection Extensions specif= ic variables."), >> in the bound table.", >> &mpx_set_cmdlist); >> >> + add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class, >> + &mpx_bnd_init_on_return, _("\ >> + Set the bnd registers to INIT state when returning from a call."), _("\ >> + Show the state of the mpx-bnd-init-on-return."), >> + NULL, >> + NULL, >> + show_mpx_bnd_init_on_return, >> + &setlist, &showlist); > > As suggested above, please move all this code to amd64-tdep.c instead. > >> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } { >> + verbose "Skipping x86 MPX tests." >> + return > > There is something I don't understand. Why are we running this test > on i?x86 if the bnd register handling is done in amd64-tdep (and > therefore seems limited to x86_64? > >> +# Needed by the return command. >> +gdb_test_no_output "set confirm off" >> + >> +set break "bkpt 1." >> +gdb_breakpoint [gdb_get_line_number "${break}"] >> +gdb_continue_to_breakpoint "${break}" ".*${break}.*" >> +gdb_test "p upper (x, a, b, c, d, 0)" "=3D 1"\ > > Add one space before the equal sign, please; here and in all tests > below. Thank you! > >> + "test the call of int function - int" >> +gdb_test "p upper_ptr (x, a, b, c, d, 0)"\ >> + "=3D \\\(int \\\*\\\) $hex" "test the call of function - ptr" >> + >> +set break "bkpt 2." >> +gdb_breakpoint [gdb_get_line_number "${break}"] >> +gdb_continue_to_breakpoint "${break}" ".*${break}.*" >> +gdb_test "p \$bound0 =3D \$bnd0" "" "Saving the\ >> + bnd0 result in a convenience variable 1" > > Let's not use the empty string for the match, but instead just > verify that the command returned some reasonable that looks reasonable. > Also, I would avoid splitting the test label (string) in two, by > formatting the test as: > > gdb_test "p \$bound0 =3D \$bnd0" " =3D {}" \ > "Saving the bnd0 result in a convenience variable 1" > > As a side-effect, this will avoid having 4 spaces between "the" and > "bnd0". > > Please apply these two recommendations in the tests below as well. > >> +gdb_test "return a" "#0 $hex in main.*" "First return" >> +gdb_test "p \(\$bnd0\.ubound =3D=3D \$bound0\.ubound\)"\ >> + "=3D 0" "return with bnd initialization off - ubound" >> +gdb_test "p \(\$bnd0\.lbound =3D=3D\$bound0\.lbound\)"\ >> + "=3D 0" "return with bnd initialization off - lbound" >> + >> +# Retesting with initialization off. >> +# All breakpoints were deleted need to recreate what we need. >> +runto_main >> +gdb_test_no_output "set mpx-bnd-init-on-return off"\ >> + "Turn off initialization on return" >> + >> +set break "bkpt 3." >> +gdb_breakpoint [gdb_get_line_number "${break}"] >> +gdb_continue_to_breakpoint "${break}" ".*${break}.*" >> +gdb_test "p \$bound0 =3D \$bnd0" "" "Saving the\ >> + bnd0 result in a convenience variable 2" >> +gdb_test "return a" "#0 $hex in main.*" "Second return" >> +gdb_test "p \(\$bnd0\.ubound =3D=3D \$bound0\.ubound\)"\ >> + "=3D 1" "return with bnd initialization on - ubound" >> +gdb_test "p \(\$bnd0\.lbound =3D=3D\$bound0\.lbound\)"\ >> + "=3D 1" "return with bnd initialization on - lbound" >> + >> +gdb_test "show mpx-bnd-init-on-return"\ >> + "BND registers will not be initialized on return\\."\ >> + "double check initialization on return" > > Thank you, > Joel, Thanks for your review! I will address your comments for the next version. Regards, -Fred Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928