From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19574 invoked by alias); 5 May 2015 08:06:07 -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 19533 invoked by uid 89); 5 May 2015 08:06:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.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-oi0-f41.google.com Received: from mail-oi0-f41.google.com (HELO mail-oi0-f41.google.com) (209.85.218.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 05 May 2015 08:06:02 +0000 Received: by oica37 with SMTP id a37so135074467oic.0 for ; Tue, 05 May 2015 01:06:00 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.182.119.199 with SMTP id kw7mr659733obb.28.1430813159744; Tue, 05 May 2015 01:05:59 -0700 (PDT) Received: by 10.202.71.193 with HTTP; Tue, 5 May 2015 01:05:59 -0700 (PDT) In-Reply-To: <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> <20150414143506.GB50171@msticlxl57.ims.intel.com> Date: Tue, 05 May 2015 08:06:00 -0000 Message-ID: Subject: Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers From: Ilya Enkovich To: Jan Hubicka Cc: Jakub Jelinek , gcc-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00280.txt.bz2 Ping 2015-04-14 17:35 GMT+03:00 Ilya Enkovich : > 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 */