diff mbox

[CHKP] Fix instrumented indirect calls with propagated pointers

Message ID 20150402162754.GE6244@msticlxl57.ims.intel.com
State New
Headers show

Commit Message

Ilya Enkovich April 2, 2015, 4:27 p.m. UTC 
On 24 Mar 15:06, Jakub Jelinek wrote:
> On Tue, Mar 24, 2015 at 12:22:27PM +0300, Ilya Enkovich wrote:
> > 2015-03-24 11:33 GMT+03:00 Jakub Jelinek <jakub@redhat.com>:
> > > On Thu, Mar 19, 2015 at 11:29:44AM +0300, Ilya Enkovich 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 (callee)
> > >> +    skip_bounds = chkp_redirect_edge (e);
> > >
> > > I just want to say that I'm not really excited about all this compile time
> > > cost that is added everywhere unconditionally for chkp.
> > > I think much better would be to guard most of it with proper option check
> > > first and only do the more expensive part if the option has been used.
> > 
> > Agree, overhead for not instrumented code should be minimized.
> > Unfortunately there is no option check I can use to guard chkp codes
> > due to LTO. Currently it is allowed to pass -fcheck-pointer-bounds for
> > IL generation and don't pass it for final code generation. I suppose I
> > may set this (or some new) flag if see instrumented node when read
> > cgraph and then use it to guard chkp related codes. Would it be
> > acceptable?
> 
> The question is what you want to do in the LTO case for the different cases,
> in particular a TU compiled with -fcheck-pointer-bounds and LTO link without
> that, or TU compiled without -fcheck-pointer-bounds and LTO link with it.
> It could be handled as LTO incompatible option, where lto1 would error out
> if you try to mix -fcheck-pointer-bounds with -fno-check-pointer-bounds
> code, or e.g. similar to var-tracking, you could consider adjusting the IL
> upon LTO reading if if some TU has been built with -fcheck-pointer-bounds
> and the LTO link is -fno-check-pointer-bounds.  Dunno what will happen
> with -fno-check-pointer-bounds TUs LTO linked with -fcheck-pointer-bounds.
> Or another possibility is to or in -fcheck-pointer-bounds from all TUs.
> 
> > Maybe replace attribute usage with a new flag in tree_decl_with_vis structure?
> 
> Depends, might be better to stick it into cgraph_node instead, depends on
> whether you are querying it already early in the FEs or just during GIMPLE
> when the cgraph node should be created too.
> 
> 	Jakub

Hi,

Here is an updated version of the patch.  I added merge for -fcheck-pointer-bounds option in LTO and used it as a guard for new code.  Testing is in progress.  Does this version look OK?

Thanks,
Ilya
--
gcc/

2015-04-02  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-02  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

Jan Hubicka April 10, 2015, 1:27 a.m. UTC | #1 
> 2015-04-02  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-02  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..28b5996 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -1277,14 +1277,25 @@ cgraph_edge::redirect_call_stmt_to_callee (void)
>  {
>    cgraph_edge *e = this;
>  
> -  tree decl = gimple_call_fndecl (e->call_stmt);
> -  tree lhs = gimple_call_lhs (e->call_stmt);
> +  tree decl;
> +  tree lhs;
>    gcall *new_stmt;
>    gimple_stmt_iterator gsi;
> +  bool skip_bounds = false;
>  #ifdef ENABLE_CHECKING
>    cgraph_node *node;
>  #endif
>  
> +  /* 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?

> 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?

Otherwise the patch seems resonable.
Honza
diff mbox

Patch

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 85531c8..28b5996 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1277,14 +1277,25 @@  cgraph_edge::redirect_call_stmt_to_callee (void)
 {
   cgraph_edge *e = this;
 
-  tree decl = gimple_call_fndecl (e->call_stmt);
-  tree lhs = gimple_call_lhs (e->call_stmt);
+  tree decl;
+  tree lhs;
   gcall *new_stmt;
   gimple_stmt_iterator gsi;
+  bool skip_bounds = false;
 #ifdef ENABLE_CHECKING
   cgraph_node *node;
 #endif
 
+  /* 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);
+
+  decl = gimple_call_fndecl (e->call_stmt);
+  lhs = gimple_call_lhs (e->call_stmt);
+
   if (e->speculative)
     {
       cgraph_edge *e2;
@@ -1390,7 +1401,8 @@  cgraph_edge::redirect_call_stmt_to_callee (void)
     }
 
   if (e->indirect_unknown_callee
-      || decl == e->callee->decl)
+      || (decl == e->callee->decl
+	  && !skip_bounds))
     return e->call_stmt;
 
 #ifdef ENABLE_CHECKING
@@ -1415,13 +1427,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 03f75b3..d6ed1f5 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -500,6 +500,71 @@  chkp_expand_bounds_reset_for_mem (tree mem, tree ptr)
   expand_normal (bndstx);
 }
 
+/* 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 86f3618..40e2489 100644
--- a/gcc/tree-chkp.h
+++ b/gcc/tree-chkp.h
@@ -54,5 +54,7 @@  extern void chkp_copy_bounds_for_assign (gimple assign,
 extern bool chkp_gimple_call_builtin_p (gimple call,
 					enum built_in_function code);
 extern void chkp_expand_bounds_reset_for_mem (tree mem, tree ptr);
+extern gcall *chkp_copy_call_skip_bounds (gcall *call);
+extern bool chkp_redirect_edge (cgraph_edge *e);
 
 #endif /* GCC_TREE_CHKP_H */