diff mbox

[CHKP] Fix instrumented indirect calls with propagated pointers

Message ID 20150414143506.GB50171@msticlxl57.ims.intel.com
State New
Headers show

Commit Message

Ilya Enkovich April 14, 2015, 2:35 p.m. UTC
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.

Comments

Ilya Enkovich May 5, 2015, 8:05 a.m. UTC | #1
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 */
Ilya Enkovich May 19, 2015, 9:39 a.m. UTC | #2
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 */
Ilya Enkovich May 26, 2015, 12:19 p.m. UTC | #3
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 */
Jan Hubicka May 29, 2015, 5:16 a.m. UTC | #4
> >>> 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
diff mbox

Patch

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 */