* [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers @ 2015-03-12 11:13 Ilya Enkovich 2015-03-19 8:30 ` Ilya Enkovich 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-03-12 11:13 UTC (permalink / raw) To: gcc-patches Hi, Instrumented function pointer may be propagated into not instrumented indirect call and vice versa. It requires additional call modifications (either remove bounds or change callee). Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? Thanks, Ilya -- gcc/ 2015-03-12 Ilya Enkovich <ilya.enkovich@intel.com> * cgraph.c (cgraph_edge::redirect_call_stmt_to_callee): Add redirection for instrumented calls. * 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-03-12 Ilya Enkovich <ilya.enkovich@intel.com> * gcc.target/i386/mpx/chkp-fix-calls-1.c: New. * gcc.target/i386/mpx/chkp-fix-calls-2.c: New. diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 5ca1901..a0b0465 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -1278,14 +1278,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 (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; @@ -1391,7 +1402,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 @@ -1416,13 +1428,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/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/tree-chkp.c b/gcc/tree-chkp.c index d2df4ba..2d2090f 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -500,6 +500,62 @@ 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); + + 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; + + if (e->callee->instrumentation_clone + || chkp_function_instrumented_p (e->callee->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 + && !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) + && gimple_call_with_bounds_p (e->call_stmt)) + { + if (e->callee->instrumented_version) + e->redirect_callee (e->callee->instrumented_version); + else + return true; + } + + 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-12 11:13 [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers Ilya Enkovich @ 2015-03-19 8:30 ` Ilya Enkovich 2015-03-24 8:33 ` Jakub Jelinek 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-03-19 8:30 UTC (permalink / raw) To: gcc-patches On 12 Mar 13:09, Ilya Enkovich wrote: > Hi, > > Instrumented function pointer may be propagated into not instrumented indirect call and vice versa. It requires additional call modifications (either remove bounds or change callee). Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? > > Here is an updated version where we don't remove bounds args in case all args are to be removed anyway. Otherwise new statement is created and EH is not cleaned for it in redirect_all_calls. New test is added. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? Thanks, Ilya -- gcc/ 2015-03-19 Ilya Enkovich <ilya.enkovich@intel.com> * cgraph.c (cgraph_edge::redirect_call_stmt_to_callee): Add redirection for instrumented calls. * 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-03-19 Ilya Enkovich <ilya.enkovich@intel.com> * 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. diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 5ca1901..a0b0465 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -1278,14 +1278,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 (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; @@ -1391,7 +1402,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 @@ -1416,13 +1428,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/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/tree-chkp.c b/gcc/tree-chkp.c index d2df4ba..b9508d8 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)) + return call; + + 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 + && !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) + && gimple_call_with_bounds_p (e->call_stmt)) + { + 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; + } + } + + 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-19 8:30 ` Ilya Enkovich @ 2015-03-24 8:33 ` Jakub Jelinek 2015-03-24 9:22 ` Ilya Enkovich 0 siblings, 1 reply; 24+ messages in thread From: Jakub Jelinek @ 2015-03-24 8:33 UTC (permalink / raw) To: Ilya Enkovich; +Cc: gcc-patches 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. In particular, the above call isn't inlined, > +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; Calls here for non-instrumented code another function that calls lookup_attribute (cheap if DECL_ATTRIBUTES is NULL, not really cheap otherwise). > + if (instrumented > + && !gimple_call_with_bounds_p (e->call_stmt)) > + e->redirect_callee (cgraph_node::get_create (e->callee->orig_decl)); > + else if (!instrumented > + && !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) > + && gimple_call_with_bounds_p (e->call_stmt)) Plus the ordering of the conditions above is bad, you first check for 3 out of a few thousands builtin and only then call the predicate, which should be probably done right after the !instrumented case. So, for the very likely case of -fcheck-pointer-bounds not being used at all, you've added at least 7-8 non-inlinable calls here. There are dozens of similar calls inserted just about everywhere. Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-24 8:33 ` Jakub Jelinek @ 2015-03-24 9:22 ` Ilya Enkovich 2015-03-24 14:06 ` Jakub Jelinek 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-03-24 9:22 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 2015-03-24 11:33 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: > 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? > > In particular, the above call isn't inlined, > >> +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; > > Calls here for non-instrumented code another function that calls > lookup_attribute (cheap if DECL_ATTRIBUTES is NULL, not really cheap > otherwise). Maybe replace attribute usage with a new flag in tree_decl_with_vis structure? > >> + if (instrumented >> + && !gimple_call_with_bounds_p (e->call_stmt)) >> + e->redirect_callee (cgraph_node::get_create (e->callee->orig_decl)); >> + else if (!instrumented >> + && !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) >> + && gimple_call_with_bounds_p (e->call_stmt)) > > Plus the ordering of the conditions above is bad, you first check > for 3 out of a few thousands builtin and only then call the predicate, > which should be probably done right after the !instrumented case. Will fix it. > > So, for the very likely case of -fcheck-pointer-bounds not being used at > all, you've added at least 7-8 non-inlinable calls here. > > There are dozens of similar calls inserted just about everywhere. The most popular guard call should be chkp_function_instrumented_p. Replacing attribute with a flag should help. Thanks for review! Ilya > > Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-24 9:22 ` Ilya Enkovich @ 2015-03-24 14:06 ` Jakub Jelinek 2015-03-24 14:40 ` Richard Biener ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Jakub Jelinek @ 2015-03-24 14:06 UTC (permalink / raw) To: Ilya Enkovich; +Cc: gcc-patches On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote: > 2015-03-24 11:33 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: > > 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-24 14:06 ` Jakub Jelinek @ 2015-03-24 14:40 ` Richard Biener 2015-03-25 8:50 ` Ilya Enkovich 2015-03-25 8:05 ` Ilya Enkovich 2015-04-02 16:28 ` Ilya Enkovich 2 siblings, 1 reply; 24+ messages in thread From: Richard Biener @ 2015-03-24 14:40 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Ilya Enkovich, gcc-patches On Tue, Mar 24, 2015 at 3:06 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote: >> 2015-03-24 11:33 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: >> > 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. I also wonder why it is necessary to execute pass_chkp_instrumentation_passes when mpx is not active. That is, can we guard that properly in void pass_manager::execute_early_local_passes () { execute_pass_list (cfun, pass_build_ssa_passes_1->sub); execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); execute_pass_list (cfun, pass_local_optimization_passes_1->sub); } (why's that so oddly wrapped?) class pass_chkp_instrumentation_passes also has no gate that guards with flag_mpx or so. That would save a IL walk over all functions (fixup_cfg) and a cgraph edge rebuild. Richard. > Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-24 14:40 ` Richard Biener @ 2015-03-25 8:50 ` Ilya Enkovich 2015-03-25 9:39 ` Richard Biener 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-03-25 8:50 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches 2015-03-24 17:40 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: > On Tue, Mar 24, 2015 at 3:06 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote: >> >> 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. > > I also wonder why it is necessary to execute pass_chkp_instrumentation_passes > when mpx is not active. > > That is, can we guard that properly in > > void > pass_manager::execute_early_local_passes () > { > execute_pass_list (cfun, pass_build_ssa_passes_1->sub); > execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); > execute_pass_list (cfun, pass_local_optimization_passes_1->sub); > } I'm worried about new functions generated in LTO. But with re-created flag_check_pointer_bounds it should be safe to guard it. > > (why's that so oddly wrapped?) > > class pass_chkp_instrumentation_passes > > also has no gate that guards with flag_mpx or so. > > That would save a IL walk over all functions (fixup_cfg) and a cgraph > edge rebuild. Right. Will fix it. Thanks, Ilya > > Richard. > >> Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 8:50 ` Ilya Enkovich @ 2015-03-25 9:39 ` Richard Biener 2015-03-25 9:50 ` Jakub Jelinek 0 siblings, 1 reply; 24+ messages in thread From: Richard Biener @ 2015-03-25 9:39 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Jakub Jelinek, gcc-patches On Wed, Mar 25, 2015 at 9:50 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2015-03-24 17:40 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >> On Tue, Mar 24, 2015 at 3:06 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote: >>> >>> 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. >> >> I also wonder why it is necessary to execute pass_chkp_instrumentation_passes >> when mpx is not active. >> >> That is, can we guard that properly in >> >> void >> pass_manager::execute_early_local_passes () >> { >> execute_pass_list (cfun, pass_build_ssa_passes_1->sub); >> execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); >> execute_pass_list (cfun, pass_local_optimization_passes_1->sub); >> } > > I'm worried about new functions generated in LTO. But with re-created > flag_check_pointer_bounds it should be safe to guard it. > >> >> (why's that so oddly wrapped?) >> >> class pass_chkp_instrumentation_passes >> >> also has no gate that guards with flag_mpx or so. >> >> That would save a IL walk over all functions (fixup_cfg) and a cgraph >> edge rebuild. > > Right. Will fix it. I am already testing Index: gcc/passes.c =================================================================== --- gcc/passes.c (revision 221633) +++ gcc/passes.c (working copy) @@ -156,7 +156,8 @@ void pass_manager::execute_early_local_passes () { execute_pass_list (cfun, pass_build_ssa_passes_1->sub); - execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); + if (flag_check_pointer_bounds) + execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); execute_pass_list (cfun, pass_local_optimization_passes_1->sub); } @@ -424,7 +425,8 @@ public: virtual bool gate (function *) { /* Don't bother doing anything if the program has errors. */ - return (!seen_error () && !in_lto_p); + return (flag_check_pointer_bounds + && !seen_error () && !in_lto_p); } }; // class pass_chkp_instrumentation_passes Richard. > Thanks, > Ilya > >> >> Richard. >> >>> Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 9:39 ` Richard Biener @ 2015-03-25 9:50 ` Jakub Jelinek 2015-03-25 10:06 ` Ilya Enkovich 2015-03-25 10:15 ` Richard Biener 0 siblings, 2 replies; 24+ messages in thread From: Jakub Jelinek @ 2015-03-25 9:50 UTC (permalink / raw) To: Richard Biener; +Cc: Ilya Enkovich, gcc-patches On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote: > --- gcc/passes.c (revision 221633) > +++ gcc/passes.c (working copy) > @@ -156,7 +156,8 @@ void > pass_manager::execute_early_local_passes () > { > execute_pass_list (cfun, pass_build_ssa_passes_1->sub); > - execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); > + if (flag_check_pointer_bounds) > + execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); > execute_pass_list (cfun, pass_local_optimization_passes_1->sub); > } > > @@ -424,7 +425,8 @@ public: > virtual bool gate (function *) > { > /* Don't bother doing anything if the program has errors. */ > - return (!seen_error () && !in_lto_p); > + return (flag_check_pointer_bounds > + && !seen_error () && !in_lto_p); > } > > }; // class pass_chkp_instrumentation_passes There is still the wasteful pass_fixup_cfg at the start of: PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes) NEXT_PASS (pass_fixup_cfg); which wasn't there before chkp. Perhaps this should be a different pass with the same execute method, but gate containing flag_check_pointer_bounds? Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 9:50 ` Jakub Jelinek @ 2015-03-25 10:06 ` Ilya Enkovich 2015-03-25 10:11 ` Jakub Jelinek 2015-03-25 10:15 ` Richard Biener 1 sibling, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-03-25 10:06 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Biener, gcc-patches 2015-03-25 12:50 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: > On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote: >> --- gcc/passes.c (revision 221633) >> +++ gcc/passes.c (working copy) >> @@ -156,7 +156,8 @@ void >> pass_manager::execute_early_local_passes () >> { >> execute_pass_list (cfun, pass_build_ssa_passes_1->sub); >> - execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); >> + if (flag_check_pointer_bounds) >> + execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); >> execute_pass_list (cfun, pass_local_optimization_passes_1->sub); >> } >> >> @@ -424,7 +425,8 @@ public: >> virtual bool gate (function *) >> { >> /* Don't bother doing anything if the program has errors. */ >> - return (!seen_error () && !in_lto_p); >> + return (flag_check_pointer_bounds >> + && !seen_error () && !in_lto_p); >> } >> >> }; // class pass_chkp_instrumentation_passes > > There is still the wasteful pass_fixup_cfg at the start of: > PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes) > NEXT_PASS (pass_fixup_cfg); > which wasn't there before chkp. Perhaps this should be a different > pass with the same execute method, but gate containing > flag_check_pointer_bounds? IIRC the reason for this pass was a different passes split, not instrumentation itself. Previously function processing always started with pass_fixup_cfg. Splitting processing into three stages we got three pass_fixup_cfg passes. Ilya > > Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 10:06 ` Ilya Enkovich @ 2015-03-25 10:11 ` Jakub Jelinek 2015-03-25 10:20 ` Richard Biener 0 siblings, 1 reply; 24+ messages in thread From: Jakub Jelinek @ 2015-03-25 10:11 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Richard Biener, gcc-patches On Wed, Mar 25, 2015 at 01:06:46PM +0300, Ilya Enkovich wrote: > > There is still the wasteful pass_fixup_cfg at the start of: > > PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes) > > NEXT_PASS (pass_fixup_cfg); > > which wasn't there before chkp. Perhaps this should be a different > > pass with the same execute method, but gate containing > > flag_check_pointer_bounds? > > IIRC the reason for this pass was a different passes split, not > instrumentation itself. Previously function processing always started > with pass_fixup_cfg. Splitting processing into three stages we got > three pass_fixup_cfg passes. Sure, but it would be really nice if for !flag_check_pointer_bounds we really could have just one stage again, rather than 3. When it is a global option, and for LTO ideally ored in from all the TUs, that shouldn't be that hard... Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 10:11 ` Jakub Jelinek @ 2015-03-25 10:20 ` Richard Biener 0 siblings, 0 replies; 24+ messages in thread From: Richard Biener @ 2015-03-25 10:20 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Ilya Enkovich, gcc-patches On Wed, Mar 25, 2015 at 11:11 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Mar 25, 2015 at 01:06:46PM +0300, Ilya Enkovich wrote: >> > There is still the wasteful pass_fixup_cfg at the start of: >> > PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes) >> > NEXT_PASS (pass_fixup_cfg); >> > which wasn't there before chkp. Perhaps this should be a different >> > pass with the same execute method, but gate containing >> > flag_check_pointer_bounds? >> >> IIRC the reason for this pass was a different passes split, not >> instrumentation itself. Previously function processing always started >> with pass_fixup_cfg. Splitting processing into three stages we got >> three pass_fixup_cfg passes. > > Sure, but it would be really nice if for !flag_check_pointer_bounds > we really could have just one stage again, rather than 3. > When it is a global option, and for LTO ideally ored in from all the TUs, > that shouldn't be that hard... LTO doesn't even run all this stuff at it only runs before LTO streaming. I don't think we want to go back to not going into SSA for all functions before early-opts (esp. early inlining). Which unfortunately won't get the EH cleanup related benefits. Btw, execute_fixup_cfg can be optimized as well - edge purging only needs to be done for the last stmt of a BB. Richard. > Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 9:50 ` Jakub Jelinek 2015-03-25 10:06 ` Ilya Enkovich @ 2015-03-25 10:15 ` Richard Biener 2015-03-25 10:24 ` Ilya Enkovich 1 sibling, 1 reply; 24+ messages in thread From: Richard Biener @ 2015-03-25 10:15 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Ilya Enkovich, gcc-patches On Wed, Mar 25, 2015 at 10:50 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote: >> --- gcc/passes.c (revision 221633) >> +++ gcc/passes.c (working copy) >> @@ -156,7 +156,8 @@ void >> pass_manager::execute_early_local_passes () >> { >> execute_pass_list (cfun, pass_build_ssa_passes_1->sub); >> - execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); >> + if (flag_check_pointer_bounds) >> + execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); >> execute_pass_list (cfun, pass_local_optimization_passes_1->sub); >> } >> >> @@ -424,7 +425,8 @@ public: >> virtual bool gate (function *) >> { >> /* Don't bother doing anything if the program has errors. */ >> - return (!seen_error () && !in_lto_p); >> + return (flag_check_pointer_bounds >> + && !seen_error () && !in_lto_p); >> } >> >> }; // class pass_chkp_instrumentation_passes > > There is still the wasteful pass_fixup_cfg at the start of: > PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes) > NEXT_PASS (pass_fixup_cfg); > which wasn't there before chkp. Perhaps this should be a different > pass with the same execute method, but gate containing > flag_check_pointer_bounds? That's not wasteful but required due to local_pure_const. The remaining wasteful fixup_cfg is the one in pass_build_ssa_passes. ISTR that pass_ipa_chkp_versioning/early_produce_thunks makes that one required? Or EH / CFG cleanup stuff makes it necessary to not fail IL checking done by into-SSA. Richard. > Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 10:15 ` Richard Biener @ 2015-03-25 10:24 ` Ilya Enkovich 0 siblings, 0 replies; 24+ messages in thread From: Ilya Enkovich @ 2015-03-25 10:24 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches 2015-03-25 13:15 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: > On Wed, Mar 25, 2015 at 10:50 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Wed, Mar 25, 2015 at 10:38:56AM +0100, Richard Biener wrote: >>> --- gcc/passes.c (revision 221633) >>> +++ gcc/passes.c (working copy) >>> @@ -156,7 +156,8 @@ void >>> pass_manager::execute_early_local_passes () >>> { >>> execute_pass_list (cfun, pass_build_ssa_passes_1->sub); >>> - execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); >>> + if (flag_check_pointer_bounds) >>> + execute_pass_list (cfun, pass_chkp_instrumentation_passes_1->sub); >>> execute_pass_list (cfun, pass_local_optimization_passes_1->sub); >>> } >>> >>> @@ -424,7 +425,8 @@ public: >>> virtual bool gate (function *) >>> { >>> /* Don't bother doing anything if the program has errors. */ >>> - return (!seen_error () && !in_lto_p); >>> + return (flag_check_pointer_bounds >>> + && !seen_error () && !in_lto_p); >>> } >>> >>> }; // class pass_chkp_instrumentation_passes >> >> There is still the wasteful pass_fixup_cfg at the start of: >> PUSH_INSERT_PASSES_WITHIN (pass_local_optimization_passes) >> NEXT_PASS (pass_fixup_cfg); >> which wasn't there before chkp. Perhaps this should be a different >> pass with the same execute method, but gate containing >> flag_check_pointer_bounds? > > That's not wasteful but required due to local_pure_const. The remaining > wasteful fixup_cfg is the one in pass_build_ssa_passes. ISTR > that pass_ipa_chkp_versioning/early_produce_thunks makes that one > required? Or EH / CFG cleanup stuff makes it necessary to not > fail IL checking done by into-SSA. These two chkp passes don't modify function bodies (mat remove it though). I don't expect them to require following fixup_cfg. Ilya > > Richard. > >> Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-24 14:06 ` Jakub Jelinek 2015-03-24 14:40 ` Richard Biener @ 2015-03-25 8:05 ` Ilya Enkovich 2015-03-25 8:16 ` Jakub Jelinek 2015-04-02 16:28 ` Ilya Enkovich 2 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-03-25 8:05 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 2015-03-24 17:06 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: > On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote: >> 2015-03-24 11:33 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: >> > 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. Mixing instrumented and not instrumented TUs is allowed. All instrumentation passes happen before LTO link. The only code generation problem if instrumented code is linked with no -fcheck-pointer-bounds is disabled chkp_finish_file call which generates static constructors. I think I just should set flag_check_pointer_bounds if see any instrumented symbol on LTO read. It would cause chkp_finish_file call when required and would be available as guard for chkp related codes. > >> 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. Flag in cgraph_node should work. I'll have a look. Thanks, Ilya > > Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 8:05 ` Ilya Enkovich @ 2015-03-25 8:16 ` Jakub Jelinek 2015-03-25 8:56 ` Ilya Enkovich 0 siblings, 1 reply; 24+ messages in thread From: Jakub Jelinek @ 2015-03-25 8:16 UTC (permalink / raw) To: Ilya Enkovich; +Cc: gcc-patches On Wed, Mar 25, 2015 at 11:05:17AM +0300, Ilya Enkovich wrote: > > 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. > > Mixing instrumented and not instrumented TUs is allowed. All > instrumentation passes happen before LTO link. The only code > generation problem if instrumented code is linked with no > -fcheck-pointer-bounds is disabled chkp_finish_file call which > generates static constructors. I think I just should set > flag_check_pointer_bounds if see any instrumented symbol on LTO read. > It would cause chkp_finish_file call when required and would be > available as guard for chkp related codes. Thus perhaps oring the flag_check_pointer_bounds option from all the TUs is the desirable behavior for LTO? I think Richard or Honza would know where would be the best spot to do that. Jakub ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-25 8:16 ` Jakub Jelinek @ 2015-03-25 8:56 ` Ilya Enkovich 0 siblings, 0 replies; 24+ messages in thread From: Ilya Enkovich @ 2015-03-25 8:56 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 2015-03-25 11:16 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: > On Wed, Mar 25, 2015 at 11:05:17AM +0300, Ilya Enkovich wrote: >> > 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. >> >> Mixing instrumented and not instrumented TUs is allowed. All >> instrumentation passes happen before LTO link. The only code >> generation problem if instrumented code is linked with no >> -fcheck-pointer-bounds is disabled chkp_finish_file call which >> generates static constructors. I think I just should set >> flag_check_pointer_bounds if see any instrumented symbol on LTO read. >> It would cause chkp_finish_file call when required and would be >> available as guard for chkp related codes. > > Thus perhaps oring the flag_check_pointer_bounds option from all the TUs is > the desirable behavior for LTO? > I think Richard or Honza would know where would be the best spot to do that. > > Jakub Is such oring used for some other flags to have an example? Thanks, Ilya ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-03-24 14:06 ` Jakub Jelinek 2015-03-24 14:40 ` Richard Biener 2015-03-25 8:05 ` Ilya Enkovich @ 2015-04-02 16:28 ` Ilya Enkovich 2015-04-10 1:28 ` Jan Hubicka 2 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-04-02 16:28 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 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 <jakub@redhat.com>: > > > 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 <ilya.enkovich@intel.com> 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 <ilya.enkovich@intel.com> 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-04-02 16:28 ` Ilya Enkovich @ 2015-04-10 1:28 ` Jan Hubicka 2015-04-14 14:35 ` Ilya Enkovich 0 siblings, 1 reply; 24+ messages in thread From: Jan Hubicka @ 2015-04-10 1:28 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Jakub Jelinek, gcc-patches > 2015-04-02 Ilya Enkovich <ilya.enkovich@intel.com> > > 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 <ilya.enkovich@intel.com> > > 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); 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? > 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? Otherwise the patch seems resonable. Honza ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-04-10 1:28 ` Jan Hubicka @ 2015-04-14 14:35 ` Ilya Enkovich 2015-05-05 8:06 ` Ilya Enkovich 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-04-14 14:35 UTC (permalink / raw) To: Jan Hubicka; +Cc: Jakub Jelinek, gcc-patches 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 <ilya.enkovich@intel.com> 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 <ilya.enkovich@intel.com> 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-04-14 14:35 ` Ilya Enkovich @ 2015-05-05 8:06 ` Ilya Enkovich 2015-05-19 9:40 ` Ilya Enkovich 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-05-05 8:06 UTC (permalink / raw) To: Jan Hubicka; +Cc: Jakub Jelinek, gcc-patches Ping 2015-04-14 17:35 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 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 <ilya.enkovich@intel.com> > > 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 <ilya.enkovich@intel.com> > > 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-05-05 8:06 ` Ilya Enkovich @ 2015-05-19 9:40 ` Ilya Enkovich 2015-05-26 13:11 ` Ilya Enkovich 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-05-19 9:40 UTC (permalink / raw) To: Jan Hubicka; +Cc: Jakub Jelinek, gcc-patches Ping 2015-05-05 11:05 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > Ping > > 2015-04-14 17:35 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >> 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 <ilya.enkovich@intel.com> >> >> 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 <ilya.enkovich@intel.com> >> >> 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-05-19 9:40 ` Ilya Enkovich @ 2015-05-26 13:11 ` Ilya Enkovich 2015-05-29 6:49 ` Jan Hubicka 0 siblings, 1 reply; 24+ messages in thread From: Ilya Enkovich @ 2015-05-26 13:11 UTC (permalink / raw) To: Jan Hubicka; +Cc: Jakub Jelinek, gcc-patches Ping 2015-05-19 12:39 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > Ping > > 2015-05-05 11:05 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >> Ping >> >> 2015-04-14 17:35 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>> 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 <ilya.enkovich@intel.com> >>> >>> 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 <ilya.enkovich@intel.com> >>> >>> 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers 2015-05-26 13:11 ` Ilya Enkovich @ 2015-05-29 6:49 ` Jan Hubicka 0 siblings, 0 replies; 24+ messages in thread From: Jan Hubicka @ 2015-05-29 6:49 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Jan Hubicka, Jakub Jelinek, gcc-patches > >>> 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> > >>> > >>> 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 <ilya.enkovich@intel.com> > >>> > >>> 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. OK. Honza ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-05-29 5:16 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-12 11:13 [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers Ilya Enkovich 2015-03-19 8:30 ` Ilya Enkovich 2015-03-24 8:33 ` Jakub Jelinek 2015-03-24 9:22 ` Ilya Enkovich 2015-03-24 14:06 ` Jakub Jelinek 2015-03-24 14:40 ` Richard Biener 2015-03-25 8:50 ` Ilya Enkovich 2015-03-25 9:39 ` Richard Biener 2015-03-25 9:50 ` Jakub Jelinek 2015-03-25 10:06 ` Ilya Enkovich 2015-03-25 10:11 ` Jakub Jelinek 2015-03-25 10:20 ` Richard Biener 2015-03-25 10:15 ` Richard Biener 2015-03-25 10:24 ` Ilya Enkovich 2015-03-25 8:05 ` Ilya Enkovich 2015-03-25 8:16 ` Jakub Jelinek 2015-03-25 8:56 ` Ilya Enkovich 2015-04-02 16:28 ` Ilya Enkovich 2015-04-10 1:28 ` Jan Hubicka 2015-04-14 14:35 ` Ilya Enkovich 2015-05-05 8:06 ` Ilya Enkovich 2015-05-19 9:40 ` Ilya Enkovich 2015-05-26 13:11 ` Ilya Enkovich 2015-05-29 6:49 ` Jan Hubicka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).