From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66752 invoked by alias); 7 Feb 2017 08:56:35 -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 66742 invoked by uid 89); 7 Feb 2017 08:56:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 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.41, explanations, Hx-languages-length:4728, H*RU: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; Tue, 07 Feb 2017 08:56:24 +0000 Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP; 07 Feb 2017 00:56:22 -0800 X-ExtLoop1: 1 Received: from ullv-win8-gdb-jenkins.wtedesch-mobl (HELO [172.28.205.157]) ([172.28.205.157]) by orsmga005.jf.intel.com with ESMTP; 07 Feb 2017 00:56:20 -0800 Subject: Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls. To: Pedro Alves , qiyaoltc@gmail.com, brobecker@adacore.com References: <1485875613-31975-1-git-send-email-walfred.tedeschi@intel.com> <53d42bb6-3b83-6213-4087-6d30e7d837de@redhat.com> Cc: gdb-patches@sourceware.org From: "Tedeschi, Walfred" Message-ID: <217a8c13-b7d0-7fe6-56b5-85ff53ce097a@intel.com> Date: Tue, 07 Feb 2017 08:56: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: <53d42bb6-3b83-6213-4087-6d30e7d837de@redhat.com> Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00142.txt.bz2 Pedro, Thanks a lot for your review! On 01/31/2017 03:13 PM, Walfred Tedeschi wrote: >> 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. > This explains the reason for clearing better ... Yes, it was my intention. Do you see value to have the patch in then? >> + >> + /* 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) > ... than this, IMO. The comment in the code doesn't talk about > "arbitrary values", for example. In any case, this comment should be nex= t to > the infcall code in question, not here, since it won't make sense for > any other call site that decided to call this function in the future, > unrelated to inferior function calls. Note how "the call" is > assuming this is talking about an infcall, but that's only clear because > we have the context of the patch; it won't be clear to anyone reading > the code after if is merged. > > Also, comment is oddly indented to two spaces too much. Agreed, I will change the comments and explanations! >> +/* 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); > s/all memory range/all memory/ I think. Yes! Thanks! >> 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. > s/Add/New/ > >> * i386-tdep.c (i386_push_dummy_call): Call i387_reset_bnd_regs. >> * amd64-tdep (amd64_push_dummy_call): Call i387_reset_bnd_regs. >> >> #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. > ... >> + >> +#include > Do we need to include stdio.h? Would stdlib.h instead do? I will try to eliminate this dependency. >> +#include "x86-cpuid.h" >> + >> +#define OUR_SIZE 5 > Should this gain a describing comment? Might not be > clear what this is about. Yes! Better naming and a comment should help. >> + >> +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}]] } { > Why "nowarnings" ? I will also modify it! (Old commit) >> + 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" > All tests test something, so the "test the" is redundant. > Also doesn't "int function - int" have a redundant "int" ? Will change as well! > Thanks, > Pedro Alves > Thanks again! Best 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