Ping
2015-05-05 11:05 GMT+03:00 Ilya Enkovich <enkovich....@gmail.com>:
> Ping
>
> 2015-04-14 17:35 GMT+03:00 Ilya Enkovich <enkovich....@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.enkov...@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.enkov...@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 */
