From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2826 invoked by alias); 6 Feb 2017 17:58:07 -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 2365 invoked by uid 89); 6 Feb 2017 17:58:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Might, describing, oddly, Hx-languages-length:3954 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 06 Feb 2017 17:57:55 +0000 Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B3ACD4D69D; Mon, 6 Feb 2017 17:57:55 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id A5AB96D9B1; Mon, 6 Feb 2017 17:57:51 +0000 (UTC) Subject: Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls. To: Walfred Tedeschi , qiyaoltc@gmail.com, brobecker@adacore.com References: <1485875613-31975-1-git-send-email-walfred.tedeschi@intel.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <53d42bb6-3b83-6213-4087-6d30e7d837de@redhat.com> Date: Mon, 06 Feb 2017 17:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1485875613-31975-1-git-send-email-walfred.tedeschi@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00127.txt.bz2 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 ... > + > + /* 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 next 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. > +/* 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. > > 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? > +#include "x86-cpuid.h" > + > +#define OUR_SIZE 5 Should this gain a describing comment? Might not be clear what this is about. > + > +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat" > + > +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \ > + [list debug nowarnings additional_flags=${comp_flags}]] } { Why "nowarnings" ? > + return -1 > +} > + > +if ![runto_main] { > + untested "could not run to main" > + return -1 > +} > + > +gdb_test_multiple "print have_mpx ()" "have mpx" { > + -re ".*= 1\r\n$gdb_prompt " { > + pass "check whether processor supports MPX" > + } > + -re ".*= 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 " = \\\{lbound = $hex, ubound = $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)" " = 1"\ > + "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" All tests test something, so the "test the" is redundant. Also doesn't "int function - int" have a redundant "int" ? Thanks, Pedro Alves