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

[Ilya Enkovich <enkovich dot gnu at gmail dot 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 */