From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 72283 invoked by alias); 7 Feb 2016 07:06:39 -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 72261 invoked by uid 89); 7 Feb 2016 07:06:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=pest, UD:gdb.arch, gdb.arch, UD:arch X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sun, 07 Feb 2016 07:06:35 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 87BF1116727; Sun, 7 Feb 2016 02:06:33 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id r5GKrIZE0vAY; Sun, 7 Feb 2016 02:06:33 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id F32F811671D; Sun, 7 Feb 2016 02:06:32 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id CEE1D406C6; Sun, 7 Feb 2016 11:06:28 +0400 (RET) Date: Sun, 07 Feb 2016 07:06:00 -0000 From: Joel Brobecker To: Walfred Tedeschi Cc: eliz@gnu.org, gdb-patches@sourceware.org Subject: Re: [PATCH V3 2/2] Add mpx-bnd-init-on-return set/show command for inferior calls. Message-ID: <20160207070628.GB15342@adacore.com> References: <1452688799-15633-1-git-send-email-walfred.tedeschi@intel.com> <1452688799-15633-3-git-send-email-walfred.tedeschi@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1452688799-15633-3-git-send-email-walfred.tedeschi@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2016-02/txt/msg00171.txt.bz2 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, struct value *function, > gdb_byte *readbuf, const gdb_byte *writebuf) > { > enum amd64_reg_class theclass[2]; > + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > int len = TYPE_LENGTH (type); > static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM }; > static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM }; > int integer_reg = 0; > int sse_reg = 0; > int i; > + extern int mpx_bnd_init_on_return; Please avoid these kinds of declarations like the pest. Normally, what we should be doing to prevent this is to declare this extern in a .h file, and then include that .h file from all the .c file referencing that global. This way, the compiler can keep all the uses consistent. Otherwise, some can change the type of that variable and the compiler would compile the above incorrectly. In this case, since this setting is only accessed from amd64-tdep, I suggest you move this global, as well as the code creating the new settting, inside amd64-tdep.c. This will avoid the need for an extern. Please make mpx_bnd_init_on_return static as well. > + if the return value we are setting is due to the use of the > + "return" command (WRITEBUF is not NULL), it is likely that > + the return is made prematurely, and therefore bnd0 register > + unset could then cause the program to receive a spurious > + bound violation, but this is only done if > + "set mpx-bnd-init-on-return" is true. */ Small nit (reminder): All sentences should start with a capital letter. Thus "If the...". I think some bits were lost in translation, though. Please replace the text above by the following: If the return value we are setting is due to the use of the "return" command (WRITEBUF is not NULL), it is likely that the return is made prematurely. In that situation, it is possible that the bnd0 register could still be unset, thus causing the program to receive a spurious bound violation. When "set mpx-bnd-init-on-return" is true, prevent this from happening by setting the bnd0 register to zero. */ > + if (writebuf != NULL > + && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0) > + { > + gdb_byte bnd_buf[16]; > + > + memset (bnd_buf, 0, 16); > + regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep), bnd_buf); > + } > } > > return RETURN_VALUE_ABI_RETURNS_ADDRESS; > @@ -833,6 +854,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function, > writebuf + i * 8); > } > > + if (I387_BND0R_REGNUM (tdep) > 0) > + { > + gdb_byte bnd_buf[16]; > + int i, init_bnd; > + > + memset (bnd_buf, 0, 16); > + if (writebuf && mpx_bnd_init_on_return) > + for (i = 0; i < I387_NUM_BND_REGS; i++) > + regcache_raw_write (regcache, I387_BND0R_REGNUM (tdep) + i, bnd_buf); > + } I suggest you use the same pattern as above (move the "if (writebuf && mpx_bnd_init_on_return)" to the first condition. It will be more consistent with the other block of code you added, making it easier to understand, and will also avoid a call to memset for nothing if mpx_bnd_init_on_return is unset. Please also add a small comment explaining why you do this. Thus: /* Same as in the class AMD64_MEMORY case above, where we set bnd0 to zero when doing a premature "return" and "set mpx-bnd-init-on-return" is true: Set all bound registers to zero in order to avoid spurious bound violations. */ if (writebuf != NULL && mpx_bnd_init_on_return && I387_BND0R_REGNUM (tdep) > 0) { ... } > +/* The value of the "set mpx-bnd-init-on-return setting. */ > + > +int mpx_bnd_init_on_return = 1; > + > +/* Show state of the setting variable mpx-bnd-init-on-return. */ > + > +static void > +show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, const char *value) > +{ > + if (mpx_bnd_init_on_return) > + fprintf_filtered (file, > + _("BND registers will be initialized on return.\n")); > + else > + fprintf_filtered (file, > + _("BND registers will not be initialized on return.\n")); > +} > + > static struct cmd_list_element *mpx_set_cmdlist, *mpx_show_cmdlist; > > /* Helper function for the CLI commands. */ > @@ -8940,6 +8958,15 @@ Show Intel(R) Memory Protection Extensions specific variables."), > in the bound table.", > &mpx_set_cmdlist); > > + add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class, > + &mpx_bnd_init_on_return, _("\ > + Set the bnd registers to INIT state when returning from a call."), _("\ > + Show the state of the mpx-bnd-init-on-return."), > + NULL, > + NULL, > + show_mpx_bnd_init_on_return, > + &setlist, &showlist); As suggested above, please move all this code to amd64-tdep.c instead. > +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } { > + verbose "Skipping x86 MPX tests." > + return There is something I don't understand. Why are we running this test on i?x86 if the bnd register handling is done in amd64-tdep (and therefore seems limited to x86_64? > +# Needed by the return command. > +gdb_test_no_output "set confirm off" > + > +set break "bkpt 1." > +gdb_breakpoint [gdb_get_line_number "${break}"] > +gdb_continue_to_breakpoint "${break}" ".*${break}.*" > +gdb_test "p upper (x, a, b, c, d, 0)" "= 1"\ Add one space before the equal sign, please; here and in all tests below. Thank you! > + "test the call of int function - int" > +gdb_test "p upper_ptr (x, a, b, c, d, 0)"\ > + "= \\\(int \\\*\\\) $hex" "test the call of function - ptr" > + > +set break "bkpt 2." > +gdb_breakpoint [gdb_get_line_number "${break}"] > +gdb_continue_to_breakpoint "${break}" ".*${break}.*" > +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\ > + bnd0 result in a convenience variable 1" Let's not use the empty string for the match, but instead just verify that the command returned some reasonable that looks reasonable. Also, I would avoid splitting the test label (string) in two, by formatting the test as: gdb_test "p \$bound0 = \$bnd0" " = {}" \ "Saving the bnd0 result in a convenience variable 1" As a side-effect, this will avoid having 4 spaces between "the" and "bnd0". Please apply these two recommendations in the tests below as well. > +gdb_test "return a" "#0 $hex in main.*" "First return" > +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\ > + "= 0" "return with bnd initialization off - ubound" > +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\ > + "= 0" "return with bnd initialization off - lbound" > + > +# Retesting with initialization off. > +# All breakpoints were deleted need to recreate what we need. > +runto_main > +gdb_test_no_output "set mpx-bnd-init-on-return off"\ > + "Turn off initialization on return" > + > +set break "bkpt 3." > +gdb_breakpoint [gdb_get_line_number "${break}"] > +gdb_continue_to_breakpoint "${break}" ".*${break}.*" > +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\ > + bnd0 result in a convenience variable 2" > +gdb_test "return a" "#0 $hex in main.*" "Second return" > +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\ > + "= 1" "return with bnd initialization on - ubound" > +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\ > + "= 1" "return with bnd initialization on - lbound" > + > +gdb_test "show mpx-bnd-init-on-return"\ > + "BND registers will not be initialized on return\\."\ > + "double check initialization on return" Thank you, -- Joel