Ping
2015-05-19 12:39 GMT+03:00 Ilya Enkovich <enkovich....@gmail.com>:
> 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 */
