From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 114563 invoked by alias); 24 Feb 2017 15:03:00 -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 114542 invoked by uid 89); 24 Feb 2017 15:02:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Period, Procedure, H*RU:HELO, Hx-spam-relays-external:HELO X-HELO: mga05.intel.com Received: from mga05.intel.com (HELO mga05.intel.com) (192.55.52.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Feb 2017 15:02:56 +0000 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP; 24 Feb 2017 07:02:55 -0800 X-ExtLoop1: 1 Received: from ullv-win8-gdb-jenkins.wtedesch-mobl (HELO [172.28.205.157]) ([172.28.205.157]) by fmsmga001.fm.intel.com with ESMTP; 24 Feb 2017 07:02:53 -0800 Subject: Re: [PATCH V8] amd64-mpx: initialize BND register before performing inferior calls. To: Pedro Alves , eliz@gnu.org, qiyaoltc@gmail.com, brobecker@adacore.com References: <1487695238-15286-1-git-send-email-walfred.tedeschi@intel.com> Cc: gdb-patches@sourceware.org From: "Tedeschi, Walfred" Message-ID: Date: Fri, 24 Feb 2017 15:03:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00677.txt.bz2 Hi Pedro, Thanks a lot for your review! Am 2/24/2017 um 2:40 PM schrieb Pedro Alves: > On 02/21/2017 04:40 PM, Walfred Tedeschi wrote: > >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -22531,6 +22531,36 @@ whose bounds are to be changed, @var{lbound} an= d @var{ubound} are new values >> for lower and upper bounds respectively. >> @end table >>=20=20=20 >> +Calling inferior functions on Intel MPX enabled program can generate bo= und > Either "on an Intel MPX enabled program" or > "on Intel MPX enabled programs". > >> +violations. > This sentence is scary. Makes me not ever want to call functions. :-) > They don't generate violations, because you're fixing GDB to not do that! > I think it'd be better if the manual spoke to the user, not to > the implementer. > > Here's an alternative version of your text changed in that direction, > along with a few adjustments and grammar tweaks: > > ~~~ > When you call an inferior function on an Intel MPX enabled program, > GDB sets the inferior's bound registers to the init (disabled) state > before calling the function. As a consequence, bounds checks for the > pointer arguments passed to the function will always pass. > > This is necessary because when you call an inferior function, the > program is usually in the middle of the execution of other function. > Since at that point bound registers are in an arbitrary state, not > clearing them would lead to random bound violations in the called > function. > > You can still examine the influence of the bound registers on the > execution of the called function by stopping the execution of the > called function at its prologue, setting bound registers, and > continuing the execution. For example: > ~~~ Thanks i will take your version. > On the GDB code itself, seems like you missed/forgot most of > my comments on v7. > > So I'll just focus on the new things. Please go back and > make sure the comments are addressed. Sorry, got to excited with the testing itself. >> + >> + >> +char char_upper (char *str, int lenght) > Line break before char_upper. Typo: "length". > >> +{ >> + char ch; >> + ch =3D *(str + lenght); >> + >> + return ch; >> +} >> + >> +char char_lower (char *str, int lenght) > Line break before char_lower. Typo: "length". > >> +{ >> + char ch; >> + ch =3D *(str - lenght); >> + >> + return ch; >> +} >> + > >> + >> +# Need for returning from an inferior call that causes a BND violation. > "Needed". It's not really needed though... Just convenient here. > >> +gdb_test_no_output "set confirm off" >> + >> +# Convenience variable. >> +# >> +set bound_reg " =3D \\\{lbound =3D $hex, ubound =3D $hex\\\}.*" >> +set int_braw_reg " =3D \\\{lbound =3D 0x0, ubound_raw =3D 0x0\\\}.*" >> +set bndcfg_reg " =3D \\\{raw =3D $hex, config =3D \\\{base =3D $hex, re= served =3D $hex,\ >> + preserved =3D $hex, enabled =3D $hex\\\}\\\}" >> +set bndstatus_reg " =3D \\\{raw =3D $hex, status =3D \\\{bde =3D $hex,\ >> + error =3D $hex\\\}\\\}" >> +set u_fault [multi_line "Program received signal SIGSEGV, Segmentation = fault" \ >> + "Upper bound violation while accessing address = $hex" \ >> + "Bounds: \\\[lower =3D $hex, upper =3D $hex\\\]= "] >> + >> + >> + >> +# Simplify the tests below. >> +# >> +proc sanity_check_bndregs {arglist} { >> + >> + global int_braw_reg >> + >> + foreach a $arglist { >> + gdb_test "p /x $a" "$int_braw_reg"\ >> + "$a" >> + } >> +} >> + >> +# Set bnd register to have no access to memory. >> +# >> +proc remove_memory_access {reg} { >> + global hex >> + >> + sanity_check_bndregs {"\$bnd0raw" "\$bnd1raw" "\$bnd2raw" "\$bnd3ra= w"} >> + >> + gdb_test "p /x $reg.lbound =3D $reg.ubound" "=3D $hex"\ >> + "$reg changed" >> + gdb_test "p /x $reg.ubound =3D 0" " =3D 0x0"\ >> + "$reg changed" > Duplicate test messages: > > https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_mes= sages_are_unique Yes, sorry. >> +} >> + >> + >> +# Prepare convenience variables for bndconfig and status >> +# for posterior comparison > Period at the end of sentences. > >> +# >> +proc prepare_bndcfg_bndstatus { } { >> + >> + global bndcfg_reg >> + global bndstatus_reg >> + >> + gdb_test "p /x \$temp_bndcfgu =3D \$bndcfgu" "$bndcfg_reg"\ >> + "bndcfgu should not change" >> + >> + gdb_test "p /x \$temp_bndstatus =3D \$bndstatus" "$bndstatus_reg"\ >> + "bndstatus should not change" >> +} >> + >> +# Compare values set for convenience variables and actual values of bnd= config >> +# and bndstatus registers. >> +# >> +proc compare_bndstatus_with_convenience {} { >> + >> + gdb_test "p \$temp_bndcfgu =3D=3D \$bndcfgu" "=3D 1"\ >> + "bndcfgu compare before and after" >> + gdb_test "p \$temp_bndstatus =3D=3D \$bndstatus" "=3D 1"\ >> + "bndstatus compare before and after" >> + >> +} >> + >> +# Perform an inferior call defined in func. >> +# >> +proc perform_a_call { func } { >> + >> + global inf_call_stopped >> + global gdb_prompt >> + >> + gdb_test_multiple "p /x $func" "Inferior call test" { > lowercase. > >> + -re $inf_call_stopped { > $inf_call_stopped doesn't end in $gdb_prompt. > >> + pass "stopped within an inferior call" > It's better to make the messages consistent. ("Inferior call test" > vs "stopped within an inferior call". > >> + } >> + -re ".. =3D 0\r\n$gdb_prompt " { >> + verbose "stopped within an inferior call" > Why is this verbose instead of fail? > >> + return -1 >> + } > This is not returning -1 (explicitly anyway) if gdb_test_multiple > detects a fail in its internal matches. Not clear whether you > mean to propagate the result of gdb_test_multiple implicitly. > But in any case, nothing is using the result of perform_a_call, > so just remove the return. > > And with all that, ISTM to that this can be a plain gdb_test. > And while at it, $inf_call_stopped isn't used anywhere else, > and could thus be inlined here. > >> + >> +} >> + >> + >> +# Perform an inferior call defined in func. >> +# >> +proc perform_a_call { func } { >> + >> + global inf_call_stopped >> + global gdb_prompt >> + >> + gdb_test_multiple "p /x $func" "Inferior call test" { >> + -re $inf_call_stopped { >> + pass "stopped within an inferior call" >> + } >> + -re ".. =3D 0\r\n$gdb_prompt " { >> + verbose "stopped within an inferior call" >> + return -1 >> + } >> + } >> + >> +} > Procedure redefined? I think so copy and paste error. > >> + >> + >> +# Perform an inferior call defined in func. >> +# > Copy/pasted comment doesn't make sense for this procedure. > >> +proc check_bound_violation { parm parm_type is_positive} { > Inconsistent whitespace around params. > >> + >> + global u_fault >> + >> + gdb_test "continue" "$u_fault.*" "Check bnd violation" > Lower case test messages. > >> + >> + if {$is_positive =3D=3D 1} { > Drop the =3D=3D 1. > >> + gdb_test "p (((void *)\$_siginfo._sifields._sigfault.si_addr\ >> + - (void*)$parm))/sizeof($parm_type) =3D=3D 1"\ >> + " =3D 1" "Accessing only one position" >> + } else { >> + gdb_test "p ((void*)$parm\ >> + - (void *)\$_siginfo._sifields._sigfault.si_addr)\ >> + /sizeof($parm_type) =3D=3D 1"\ >> + " =3D 1" "Accessing only one position" >> + } > Lowercase. "accessing" -> "access". > >> + gdb_test "return" "\\\#.*main.*i386-mpx-call\\\.c:.*" "return from = the fault" >> +} >> + >> + >> +# Start testing! >> +# >> + >> +# Set up just stop in middle of main for call function in the inferior. > # Set up for stopping in the middle of main for calling a function in the= inferior. > > >> +# >> +with_test_prefix "default_run" { >> + >> + gdb_test "p \$keep_bnd0_value=3D\$bnd0" $bound_reg\ >> + "Store bnd0 register in a convenience variable" > Lowercase. > >> + >> + gdb_test "p /x upper (a, b, c, d, 0)" " =3D $hex"\ >> + "Inferior call test" > Lowercase. But "test" is redundant, these are all tests. > But better say "call inferior function" instead. > >> + >> + gdb_test "p ((\$bnd0.lbound=3D=3D\$keep_bnd0_value.lbound) &&\ >> + (\$bnd0.ubound=3D=3D\$keep_bnd0_value.ubound))" "=3D 1" "Keep B= ND register\ >> + value calls" > Lowercase. Breaking the line before the last string starts reads > a bit better: > > gdb_test "p ((\$bnd0.lbound=3D=3D\$keep_bnd0_value.lbound) &&\ > (\$bnd0.ubound=3D=3D\$keep_bnd0_value.ubound))" "=3D 1" \ > "keep BND register value calls" > >> +} >> + >> +# Consistency: Examine bnd registers values before and after the call. >> +# >> +# >> +with_test_prefix "verify_default_values" { >> + >> + prepare_bndcfg_bndstatus >> + >> + gdb_breakpoint "*upper" >> + perform_a_call "upper (a, b, c, d, 1)" >> + >> + sanity_check_bndregs {"\$bnd0raw" "\$bnd1raw" "\$bnd2raw" "\$bnd3ra= w"} >> + >> + compare_bndstatus_with_convenience >> + >> + gdb_test_multiple "continue" "Inferior call test" { >> + -re ".*Continuing.\r\n$gdb_prompt " { >> + pass "normal continue" > Inconsistent: "Inferior call test" vs "normal continue". > >> + } >> + } > >> +} >> + >> +# Examine: Cause a upper bound violation changing BND0. > "an upper" > > Thanks, > Pedro Alves Will address all comments. 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