From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99890 invoked by alias); 2 Apr 2015 16:28:25 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 99874 invoked by uid 89); 2 Apr 2015 16:28:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ig0-f175.google.com Received: from mail-ig0-f175.google.com (HELO mail-ig0-f175.google.com) (209.85.213.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 02 Apr 2015 16:28:13 +0000 Received: by igcau2 with SMTP id au2so80911635igc.0 for ; Thu, 02 Apr 2015 09:28:11 -0700 (PDT) X-Received: by 10.107.133.27 with SMTP id h27mr2372756iod.31.1427992091720; Thu, 02 Apr 2015 09:28:11 -0700 (PDT) Received: from msticlxl57.ims.intel.com (fmdmzpr03-ext.fm.intel.com. [192.55.54.38]) by mx.google.com with ESMTPSA id t76sm3575423ioi.4.2015.04.02.09.28.06 (version=TLSv1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 02 Apr 2015 09:28:07 -0700 (PDT) Date: Thu, 02 Apr 2015 16:28:00 -0000 From: Ilya Enkovich To: Jakub Jelinek Cc: gcc-patches Subject: Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers Message-ID: <20150402162754.GE6244@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150324140619.GE1746@tucnak.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00069.txt.bz2 On 24 Mar 15:06, Jakub Jelinek wrote: > On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote: > > 2015-03-24 11:33 GMT+03:00 Jakub Jelinek : > > > On Thu, Mar 19, 2015 at 11:29:44AM +0300, Ilya Enkovich 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 (callee) > > >> + skip_bounds = chkp_redirect_edge (e); > > > > > > I just want to say that I'm not really excited about all this compile time > > > cost that is added everywhere unconditionally for chkp. > > > I think much better would be to guard most of it with proper option check > > > first and only do the more expensive part if the option has been used. > > > > Agree, overhead for not instrumented code should be minimized. > > Unfortunately there is no option check I can use to guard chkp codes > > due to LTO. Currently it is allowed to pass -fcheck-pointer-bounds for > > IL generation and don't pass it for final code generation. I suppose I > > may set this (or some new) flag if see instrumented node when read > > cgraph and then use it to guard chkp related codes. Would it be > > acceptable? > > The question is what you want to do in the LTO case for the different cases, > in particular a TU compiled with -fcheck-pointer-bounds and LTO link without > that, or TU compiled without -fcheck-pointer-bounds and LTO link with it. > It could be handled as LTO incompatible option, where lto1 would error out > if you try to mix -fcheck-pointer-bounds with -fno-check-pointer-bounds > code, or e.g. similar to var-tracking, you could consider adjusting the IL > upon LTO reading if if some TU has been built with -fcheck-pointer-bounds > and the LTO link is -fno-check-pointer-bounds. Dunno what will happen > with -fno-check-pointer-bounds TUs LTO linked with -fcheck-pointer-bounds. > Or another possibility is to or in -fcheck-pointer-bounds from all TUs. > > > Maybe replace attribute usage with a new flag in tree_decl_with_vis structure? > > Depends, might be better to stick it into cgraph_node instead, depends on > whether you are querying it already early in the FEs or just during GIMPLE > when the cgraph node should be created too. > > Jakub Hi, Here is an updated version of the patch. I added merge for -fcheck-pointer-bounds option in LTO and used it as a guard for new code. Testing is in progress. Does this version look OK? Thanks, Ilya -- gcc/ 2015-04-02 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-02 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..28b5996 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -1277,14 +1277,25 @@ cgraph_edge::redirect_call_stmt_to_callee (void) { cgraph_edge *e = this; - tree decl = gimple_call_fndecl (e->call_stmt); - tree lhs = gimple_call_lhs (e->call_stmt); + tree decl; + tree lhs; gcall *new_stmt; gimple_stmt_iterator gsi; + bool skip_bounds = false; #ifdef ENABLE_CHECKING cgraph_node *node; #endif + /* 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); + + decl = gimple_call_fndecl (e->call_stmt); + lhs = gimple_call_lhs (e->call_stmt); + if (e->speculative) { cgraph_edge *e2; @@ -1390,7 +1401,8 @@ cgraph_edge::redirect_call_stmt_to_callee (void) } if (e->indirect_unknown_callee - || decl == e->callee->decl) + || (decl == e->callee->decl + && !skip_bounds)) return e->call_stmt; #ifdef ENABLE_CHECKING @@ -1415,13 +1427,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 03f75b3..d6ed1f5 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -500,6 +500,71 @@ chkp_expand_bounds_reset_for_mem (tree mem, tree ptr) expand_normal (bndstx); } +/* 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 86f3618..40e2489 100644 --- a/gcc/tree-chkp.h +++ b/gcc/tree-chkp.h @@ -54,5 +54,7 @@ extern void chkp_copy_bounds_for_assign (gimple assign, extern bool chkp_gimple_call_builtin_p (gimple call, enum built_in_function code); extern void chkp_expand_bounds_reset_for_mem (tree mem, tree ptr); +extern gcall *chkp_copy_call_skip_bounds (gcall *call); +extern bool chkp_redirect_edge (cgraph_edge *e); #endif /* GCC_TREE_CHKP_H */