From patchwork Tue Apr 14 14:35:06 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Enkovich X-Patchwork-Id: 461156 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id B3BD7140271 for ; Wed, 15 Apr 2015 00:35:38 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=av5rCsK/; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=ndPunHeFaCRQOo4s5 e9WXvg+0+665C6r7MlkBV1lO6HawEU2PLzUDcIbFKGrLSMXb1lF7DQq/BEprAz3P tp3DmSqjqYWuB5UtwoSodRV++9BW24yqmF1xp1SquUB2nvs7sL9nAh53S18DWTld 9FNKIPvTN35WPeXO6ff6mMQ5lc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=ZgE2iTsR2WvBP2UczyKSQT4 GEsY=; b=av5rCsK/7Bh0X1q+Kvyt3Me03AmoVtknfGV25yPONzYCpI7C8A0TLW9 qVdWPoRJGwL4zJ0+RitA5U6SR8BdB9GzlHBTFXdajPJkKaT2ot20YekonNugzd7K hROKenqb2CyyU9189edh673N3x5HjpmYwA0thOnBx3jAOWArvDV4= Received: (qmail 33807 invoked by alias); 14 Apr 2015 14:35:30 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 33792 invoked by uid 89); 14 Apr 2015 14:35:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ie0-f174.google.com Received: from mail-ie0-f174.google.com (HELO mail-ie0-f174.google.com) (209.85.223.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 14 Apr 2015 14:35:28 +0000 Received: by iejt8 with SMTP id t8so16260661iej.2 for ; Tue, 14 Apr 2015 07:35:26 -0700 (PDT) X-Received: by 10.43.13.71 with SMTP id pl7mr26668017icb.31.1429022126204; Tue, 14 Apr 2015 07:35:26 -0700 (PDT) Received: from msticlxl57.ims.intel.com (fmdmzpr02-ext.fm.intel.com. [192.55.55.37]) by mx.google.com with ESMTPSA id hh9sm7512253igb.1.2015.04.14.07.35.23 (version=TLSv1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 14 Apr 2015 07:35:25 -0700 (PDT) Date: Tue, 14 Apr 2015 17:35:06 +0300 From: Ilya Enkovich To: Jan Hubicka Cc: Jakub Jelinek , gcc-patches Subject: Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers Message-ID: <20150414143506.GB50171@msticlxl57.ims.intel.com> References: <20150312100931.GK27860@msticlxl57.ims.intel.com> <20150319082944.GC64546@msticlxl57.ims.intel.com> <20150324083325.GC1746@tucnak.redhat.com> <20150324140619.GE1746@tucnak.redhat.com> <20150402162754.GE6244@msticlxl57.ims.intel.com> <20150410012759.GB2320@atrey.karlin.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150410012759.GB2320@atrey.karlin.mff.cuni.cz> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes On 10 Apr 03:27, Jan Hubicka wrote: > > > > + /* We might propagate instrumented function pointer into > > + not instrumented function and vice versa. In such a > > + case we need to either fix function declaration or > > + remove bounds from call statement. */ > > + if (flag_check_pointer_bounds && callee) > > + skip_bounds = chkp_redirect_edge (e); > > I think this gets wrong the case where the edge is speculative and the new > direct call needs adjustement. You probably need to do the right think in > the if (e->speculative) branch so direct call is updated by indirect is not > or at least give an explanation why this is not needed :) > > The speculative edge handling works in a way that the runtime conditoinal is > built and then the edge is updated to the direct path, so perhaps > you can just move all this after the ocnditoinal? I think you are right, it should be OK to move it after apeculative call processing. > > > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c > > index 404cb68..ffb6ad7 100644 > > --- a/gcc/lto-wrapper.c > > +++ b/gcc/lto-wrapper.c > > @@ -273,6 +273,7 @@ merge_and_complain (struct cl_decoded_option **decoded_options, > > case OPT_fwrapv: > > case OPT_fopenmp: > > case OPT_fopenacc: > > + case OPT_fcheck_pointer_bounds: > > /* For selected options we can merge conservatively. */ > > for (j = 0; j < *decoded_options_count; ++j) > > if ((*decoded_options)[j].opt_index == foption->opt_index) > > @@ -503,6 +504,7 @@ append_compiler_options (obstack *argv_obstack, struct cl_decoded_option *opts, > > case OPT_Ofast: > > case OPT_Og: > > case OPT_Os: > > + case OPT_fcheck_pointer_bounds: > > Hmm, will this make mixed units contaiing some check bounds and some non-check bounds to work? > Perhaps these should be function specific? Does things like inlining bounds checked function into > non-checked function work? This actually should make it work better because solves a possible problem with uninitialized static bounds data (chkp static constructors are generated only when OPT_fcheck_pointer_bounds is passed). Inlining of instrumentation thunks is not supported (similar to all other thunks). But we may have a not instrumented call in an instrumented function and do inlining for it. > > Otherwise the patch seems resonable. > Honza Here is a fixed version with chkp redirection moved. Bootstrap and testing passed. Is it OK for trunk and later for GCC 5? Thanks, Ilya --- gcc/ 2015-04-14 Ilya Enkovich PR target/65527 * cgraph.c (cgraph_edge::redirect_call_stmt_to_callee): Add redirection for instrumented calls. * lto-wrapper.c (merge_and_complain): Merge -fcheck-pointer-bounds. (append_compiler_options): Append -fcheck-pointer-bounds. * tree-chkp.h (chkp_copy_call_skip_bounds): New. (chkp_redirect_edge): New. * tree-chkp.c (chkp_copy_call_skip_bounds): New. (chkp_redirect_edge): New. gcc/testsuite/ 2015-04-14 Ilya Enkovich PR target/65527 * gcc.target/i386/mpx/chkp-fix-calls-1.c: New. * gcc.target/i386/mpx/chkp-fix-calls-2.c: New. * gcc.target/i386/mpx/chkp-fix-calls-3.c: New. * gcc.target/i386/mpx/chkp-fix-calls-4.c: New. diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 85531c8..38e71fc 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -1281,6 +1281,7 @@ cgraph_edge::redirect_call_stmt_to_callee (void) tree lhs = gimple_call_lhs (e->call_stmt); gcall *new_stmt; gimple_stmt_iterator gsi; + bool skip_bounds = false; #ifdef ENABLE_CHECKING cgraph_node *node; #endif @@ -1389,8 +1390,16 @@ cgraph_edge::redirect_call_stmt_to_callee (void) } } + /* We might propagate instrumented function pointer into + not instrumented function and vice versa. In such a + case we need to either fix function declaration or + remove bounds from call statement. */ + if (flag_check_pointer_bounds && e->callee) + skip_bounds = chkp_redirect_edge (e); + if (e->indirect_unknown_callee - || decl == e->callee->decl) + || (decl == e->callee->decl + && !skip_bounds)) return e->call_stmt; #ifdef ENABLE_CHECKING @@ -1415,13 +1424,19 @@ cgraph_edge::redirect_call_stmt_to_callee (void) } } - if (e->callee->clone.combined_args_to_skip) + if (e->callee->clone.combined_args_to_skip + || skip_bounds) { int lp_nr; - new_stmt - = gimple_call_copy_skip_args (e->call_stmt, - e->callee->clone.combined_args_to_skip); + new_stmt = e->call_stmt; + if (e->callee->clone.combined_args_to_skip) + new_stmt + = gimple_call_copy_skip_args (new_stmt, + e->callee->clone.combined_args_to_skip); + if (skip_bounds) + new_stmt = chkp_copy_call_skip_bounds (new_stmt); + gimple_call_set_fndecl (new_stmt, e->callee->decl); gimple_call_set_fntype (new_stmt, gimple_call_fntype (e->call_stmt)); diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c index 404cb68..ffb6ad7 100644 --- a/gcc/lto-wrapper.c +++ b/gcc/lto-wrapper.c @@ -273,6 +273,7 @@ merge_and_complain (struct cl_decoded_option **decoded_options, case OPT_fwrapv: case OPT_fopenmp: case OPT_fopenacc: + case OPT_fcheck_pointer_bounds: /* For selected options we can merge conservatively. */ for (j = 0; j < *decoded_options_count; ++j) if ((*decoded_options)[j].opt_index == foption->opt_index) @@ -503,6 +504,7 @@ append_compiler_options (obstack *argv_obstack, struct cl_decoded_option *opts, case OPT_Ofast: case OPT_Og: case OPT_Os: + case OPT_fcheck_pointer_bounds: break; default: diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-1.c b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-1.c new file mode 100644 index 0000000..cb4d229 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-1.c @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcheck-pointer-bounds -mmpx" } */ + +#include "math.h" + +double +test1 (double x, double y, double (*fn)(double, double)) +{ + return fn (x, y); +} + +double +test2 (double x, double y) +{ + return test1 (x, y, copysign); +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-2.c b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-2.c new file mode 100644 index 0000000..951e7de --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-2.c @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -fcheck-pointer-bounds -mmpx -fno-inline" } */ + +#include "math.h" + +double +test1 (double x, double y, double (*fn)(double, double)) +{ + return fn (x, y); +} + +double +test2 (double x, double y) +{ + return test1 (x, y, copysign); +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-3.c b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-3.c new file mode 100755 index 0000000..439f631 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-3.c @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fexceptions -fcheck-pointer-bounds -mmpx" } */ + +extern int f2 (const char*, int, ...); +extern long int f3 (int *); +extern void err (void) __attribute__((__error__("error"))); + +extern __inline __attribute__ ((__always_inline__)) int +f1 (int i, ...) +{ + if (__builtin_constant_p (i)) + { + if (i) + err (); + return f2 ("", i); + } + + return f2 ("", i); +} + +int +test () +{ + int i; + + if (f1 (0)) + if (f3 (&i)) + i = 0; + + return i; +} + + diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-4.c b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-4.c new file mode 100644 index 0000000..1b7d703 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-fix-calls-4.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-Os -fcheck-pointer-bounds -mmpx" } */ + +typedef void (func) (int *); + +static inline void +bar (func f) +{ + int i; + f (&i); +} + +void +foo () +{ + bar (0); +} diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index 8c5a628..c2d9e94 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -529,6 +529,71 @@ chkp_insert_retbnd_call (tree bndval, tree retval, return bndval; } +/* Build a GIMPLE_CALL identical to CALL but skipping bounds + arguments. */ + +gcall * +chkp_copy_call_skip_bounds (gcall *call) +{ + bitmap bounds; + unsigned i; + + bitmap_obstack_initialize (NULL); + bounds = BITMAP_ALLOC (NULL); + + for (i = 0; i < gimple_call_num_args (call); i++) + if (POINTER_BOUNDS_P (gimple_call_arg (call, i))) + bitmap_set_bit (bounds, i); + + if (!bitmap_empty_p (bounds)) + call = gimple_call_copy_skip_args (call, bounds); + gimple_call_set_with_bounds (call, false); + + BITMAP_FREE (bounds); + bitmap_obstack_release (NULL); + + return call; +} + +/* Redirect edge E to the correct node according to call_stmt. + Return 1 if bounds removal from call_stmt should be done + instead of redirection. */ + +bool +chkp_redirect_edge (cgraph_edge *e) +{ + bool instrumented = false; + tree decl = e->callee->decl; + + if (e->callee->instrumentation_clone + || chkp_function_instrumented_p (decl)) + instrumented = true; + + if (instrumented + && !gimple_call_with_bounds_p (e->call_stmt)) + e->redirect_callee (cgraph_node::get_create (e->callee->orig_decl)); + else if (!instrumented + && gimple_call_with_bounds_p (e->call_stmt) + && !chkp_gimple_call_builtin_p (e->call_stmt, BUILT_IN_CHKP_BNDCL) + && !chkp_gimple_call_builtin_p (e->call_stmt, BUILT_IN_CHKP_BNDCU) + && !chkp_gimple_call_builtin_p (e->call_stmt, BUILT_IN_CHKP_BNDSTX)) + { + if (e->callee->instrumented_version) + e->redirect_callee (e->callee->instrumented_version); + else + { + tree args = TYPE_ARG_TYPES (TREE_TYPE (decl)); + /* Avoid bounds removal if all args will be removed. */ + if (!args || TREE_VALUE (args) != void_type_node) + return true; + else + gimple_call_set_with_bounds (e->call_stmt, false); + } + } + + return false; +} + /* Mark statement S to not be instrumented. */ static void chkp_mark_stmt (gimple s) diff --git a/gcc/tree-chkp.h b/gcc/tree-chkp.h index 1bafe99..b5ab562 100644 --- a/gcc/tree-chkp.h +++ b/gcc/tree-chkp.h @@ -56,5 +56,7 @@ extern bool chkp_gimple_call_builtin_p (gimple call, extern void chkp_expand_bounds_reset_for_mem (tree mem, tree ptr); extern tree chkp_insert_retbnd_call (tree bndval, tree retval, gimple_stmt_iterator *gsi); +extern gcall *chkp_copy_call_skip_bounds (gcall *call); +extern bool chkp_redirect_edge (cgraph_edge *e); #endif /* GCC_TREE_CHKP_H */