Re: [CHKP, PATCH] Fix instrumented indirect calls with propagated pointers

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