Teach ipa-split about TSAN_FUNC_EXIT (PR sanitizer/65400)

Hi!

TSAN_FUNC_EXIT internal function is special, because we drop it during
inlining, so for fnsplit we need to be careful about it, otherwise we can
end up with unbalanced pairs of tsan entry/exit marker functions.

This patch gives up unless it is one of the two most common cases with
-fsanitize=thread - either TSAN_FUNC_EXIT in the return bb, which is handled
as accepting it as return bb despite the TSAN_FUNC_EXIT call - duplicating
the TSAN_FUNC_EXIT is exactly the right thing to do in that case, we want it
in the *.part.* function and also in the caller, if the former is later
inlined into the latter again, the TSAN_FUNC_EXIT of the former is dropped.
And another case that we handle is if there is TSAN_FUNC_EXIT on the
predecessors of return_bb - we can handle that case easily too by
duplicating TSAN_FUNC_EXIT after the *.part.* call.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2015-03-18  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/65400
	* ipa-split.c (find_return_bb): Allow TSAN_FUNC_EXIT internal
	call in the return bb.
	(find_split_points): Add RETURN_BB argument, don't call
	find_return_bb.
	(split_function): Likewise.  Add ADD_TSAN_FUNC_EXIT argument,
	if true append TSAN_FUNC_EXIT internal call after the call to
	the split off function.
	(execute_split_functions): Call find_return_bb here.
	Don't optimize if TSAN_FUNC_EXIT is found in unexpected places.
	Adjust find_split_points and split_function calls.

	* c-c++-common/tsan/pr65400-1.c: New test.
	* c-c++-common/tsan/pr65400-2.c: New test.


	Jakub

On March 18, 2015 11:36:09 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>TSAN_FUNC_EXIT internal function is special, because we drop it during
>inlining, so for fnsplit we need to be careful about it, otherwise we
>can
>end up with unbalanced pairs of tsan entry/exit marker functions.
>
>This patch gives up unless it is one of the two most common cases with
>-fsanitize=thread - either TSAN_FUNC_EXIT in the return bb, which is
>handled
>as accepting it as return bb despite the TSAN_FUNC_EXIT call -
>duplicating
>the TSAN_FUNC_EXIT is exactly the right thing to do in that case, we
>want it
>in the *.part.* function and also in the caller, if the former is later
>inlined into the latter again, the TSAN_FUNC_EXIT of the former is
>dropped.
>And another case that we handle is if there is TSAN_FUNC_EXIT on the
>predecessors of return_bb - we can handle that case easily too by
>duplicating TSAN_FUNC_EXIT after the *.part.* call.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

>2015-03-18  Jakub Jelinek  <jakub@redhat.com>
>
>	PR sanitizer/65400
>	* ipa-split.c (find_return_bb): Allow TSAN_FUNC_EXIT internal
>	call in the return bb.
>	(find_split_points): Add RETURN_BB argument, don't call
>	find_return_bb.
>	(split_function): Likewise.  Add ADD_TSAN_FUNC_EXIT argument,
>	if true append TSAN_FUNC_EXIT internal call after the call to
>	the split off function.
>	(execute_split_functions): Call find_return_bb here.
>	Don't optimize if TSAN_FUNC_EXIT is found in unexpected places.
>	Adjust find_split_points and split_function calls.
>
>	* c-c++-common/tsan/pr65400-1.c: New test.
>	* c-c++-common/tsan/pr65400-2.c: New test.
>
>--- gcc/ipa-split.c.jj	2015-03-17 18:10:11.000000000 +0100
>+++ gcc/ipa-split.c	2015-03-18 17:12:45.017773858 +0100
>@@ -769,7 +769,8 @@ consider_split (struct split_point *curr
>    of the form:
>    <retval> = tmp_var;
>    return <retval>
>-   but return_bb can not be more complex than this.
>+   but return_bb can not be more complex than this (except for
>+   -fsanitize=thread we allow TSAN_FUNC_EXIT () internal call in
>there).
>    If nothing is found, return the exit block.
> 
> When there are multiple RETURN statement, chose one with return value,
>@@ -814,6 +815,13 @@ find_return_bb (void)
> 	  found_return = true;
> 	  retval = gimple_return_retval (return_stmt);
> 	}
>+      /* For -fsanitize=thread, allow also TSAN_FUNC_EXIT () in the
>return
>+	 bb.  */
>+      else if ((flag_sanitize & SANITIZE_THREAD)
>+	       && is_gimple_call (stmt)
>+	       && gimple_call_internal_p (stmt)
>+	       && gimple_call_internal_fn (stmt) == IFN_TSAN_FUNC_EXIT)
>+	;
>       else
> 	break;
>     }
>@@ -1074,12 +1082,11 @@ typedef struct
>    the component used by consider_split.  */
> 
> static void
>-find_split_points (int overall_time, int overall_size)
>+find_split_points (basic_block return_bb, int overall_time, int
>overall_size)
> {
>   stack_entry first;
>   vec<stack_entry> stack = vNULL;
>   basic_block bb;
>-  basic_block return_bb = find_return_bb ();
>   struct split_point current;
> 
>   current.header_time = overall_time;
>@@ -1236,19 +1243,20 @@ insert_bndret_call_after (tree retbnd, t
>   gimple_call_set_lhs (bndret, retbnd);
>   gsi_insert_after (gsi, bndret, GSI_CONTINUE_LINKING);
> }
>+
> /* Split function at SPLIT_POINT.  */
> 
> static void
>-split_function (struct split_point *split_point)
>+split_function (basic_block return_bb, struct split_point
>*split_point,
>+		bool add_tsan_func_exit)
> {
>   vec<tree> args_to_pass = vNULL;
>   bitmap args_to_skip;
>   tree parm;
>   int num = 0;
>cgraph_node *node, *cur_node = cgraph_node::get
>(current_function_decl);
>-  basic_block return_bb = find_return_bb ();
>   basic_block call_bb;
>-  gcall *call;
>+  gcall *call, *tsan_func_exit_call = NULL;
>   edge e;
>   edge_iterator ei;
>   tree retval = NULL, real_retval = NULL, retbnd = NULL;
>@@ -1534,11 +1542,18 @@ split_function (struct split_point *spli
> 	  || DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))))
>     gimple_call_set_return_slot_opt (call, true);
> 
>+  if (add_tsan_func_exit)
>+    tsan_func_exit_call = gimple_build_call_internal
>(IFN_TSAN_FUNC_EXIT, 0);
>+
>   /* Update return value.  This is bit tricky.  When we do not return,
>      do nothing.  When we return we might need to update return_bb
>      or produce a new return statement.  */
>   if (!split_part_return_p)
>-    gsi_insert_after (&gsi, call, GSI_NEW_STMT);
>+    {
>+      gsi_insert_after (&gsi, call, GSI_NEW_STMT);
>+      if (tsan_func_exit_call)
>+	gsi_insert_after (&gsi, tsan_func_exit_call, GSI_NEW_STMT);
>+    }
>   else
>     {
>       e = make_edge (call_bb, return_bb,
>@@ -1642,6 +1657,8 @@ split_function (struct split_point *spli
> 	    }
> 	  else
> 	    gsi_insert_after (&gsi, call, GSI_NEW_STMT);
>+	  if (tsan_func_exit_call)
>+	    gsi_insert_after (&gsi, tsan_func_exit_call, GSI_NEW_STMT);
> 	}
> /* We don't use return block (there is either no return in function or
> 	 multiple of them).  So create new basic block with return statement.
>@@ -1684,6 +1701,8 @@ split_function (struct split_point *spli
> 	  /* Build bndret call to obtain returned bounds.  */
> 	  if (retbnd)
> 	    insert_bndret_call_after (retbnd, retval, &gsi);
>+	  if (tsan_func_exit_call)
>+	    gsi_insert_after (&gsi, tsan_func_exit_call, GSI_NEW_STMT);
> 	  ret = gimple_build_return (retval);
> 	  gsi_insert_after (&gsi, ret, GSI_NEW_STMT);
> 	}
>@@ -1792,6 +1811,8 @@ execute_split_functions (void)
>/* Compute local info about basic blocks and determine function
>size/time.  */
>   bb_info_vec.safe_grow_cleared (last_basic_block_for_fn (cfun) + 1);
>   memset (&best_split_point, 0, sizeof (best_split_point));
>+  basic_block return_bb = find_return_bb ();
>+  int tsan_exit_found = -1;
>   FOR_EACH_BB_FN (bb, cfun)
>     {
>       int time = 0;
>@@ -1818,16 +1839,37 @@ execute_split_functions (void)
> 		       freq, this_size, this_time);
> 	      print_gimple_stmt (dump_file, stmt, 0, 0);
> 	    }
>+
>+	  if ((flag_sanitize & SANITIZE_THREAD)
>+	      && is_gimple_call (stmt)
>+	      && gimple_call_internal_p (stmt)
>+	      && gimple_call_internal_fn (stmt) == IFN_TSAN_FUNC_EXIT)
>+	    {
>+	      /* We handle TSAN_FUNC_EXIT for splitting either in the
>+		 return_bb, or in its immediate predecessors.  */
>+	      if ((bb != return_bb && !find_edge (bb, return_bb))
>+		  || (tsan_exit_found != -1
>+		      && tsan_exit_found != (bb != return_bb)))
>+		{
>+		  if (dump_file)
>+		    fprintf (dump_file, "Not splitting: TSAN_FUNC_EXIT"
>+			     " in unexpected basic block.\n");
>+		  BITMAP_FREE (forbidden_dominators);
>+		  bb_info_vec.release ();
>+		  return 0;
>+		}
>+	      tsan_exit_found = bb != return_bb;
>+	    }
> 	}
>       overall_time += time;
>       overall_size += size;
>       bb_info_vec[bb->index].time = time;
>       bb_info_vec[bb->index].size = size;
>     }
>-  find_split_points (overall_time, overall_size);
>+  find_split_points (return_bb, overall_time, overall_size);
>   if (best_split_point.split_bbs)
>     {
>-      split_function (&best_split_point);
>+      split_function (return_bb, &best_split_point, tsan_exit_found ==
>1);
>       BITMAP_FREE (best_split_point.ssa_names_to_pass);
>       BITMAP_FREE (best_split_point.split_bbs);
>       todo = TODO_update_ssa | TODO_cleanup_cfg;
>--- gcc/testsuite/c-c++-common/tsan/pr65400-1.c.jj	2015-03-18
>18:33:27.478940014 +0100
>+++ gcc/testsuite/c-c++-common/tsan/pr65400-1.c	2015-03-18
>18:43:49.235836436 +0100
>@@ -0,0 +1,85 @@
>+/* PR sanitizer/65400 */
>+/* { dg-shouldfail "tsan" } */
>+/* { dg-additional-options "-fno-omit-frame-pointer -ldl" } */
>+/* { dg-additional-sources pr65400-2.c } */
>+
>+#include <pthread.h>
>+#include "tsan_barrier.h"
>+
>+static pthread_barrier_t barrier;
>+int v;
>+int q;
>+int o;
>+extern void baz4 (int *);
>+
>+__attribute__((noinline, noclone)) int
>+bar (int x)
>+{
>+  q += x;
>+  return x;
>+}
>+
>+void
>+foo (int *x)
>+{
>+  if (__builtin_expect (x == 0, 1))
>+    return;
>+  bar (bar (bar (bar (*x))));
>+}
>+
>+__attribute__((noinline, noclone)) void
>+baz1 (int *x)
>+{
>+  foo (x);
>+}
>+
>+__attribute__((noinline, noclone)) void
>+baz2 (int **x)
>+{
>+  foo (*x);
>+}
>+
>+__attribute__((noinline, noclone)) void
>+baz3 (void)
>+{
>+  barrier_wait (&barrier);
>+  v++;
>+}
>+
>+__attribute__((noinline, noclone)) void
>+baz5 (void)
>+{
>+  int i;
>+  o = 1;
>+  baz1 (&o);
>+  int *p = &o;
>+  baz2 (&p);
>+  for (i = 0; i < 128; i++)
>+    baz4 (&o);
>+  if (q != 130 * 4)
>+    __builtin_abort ();
>+  baz3 ();
>+}
>+
>+__attribute__((noinline, noclone)) void *
>+tf (void *arg)
>+{
>+  (void) arg;
>+  baz5 ();
>+  return NULL;
>+}
>+
>+int
>+main ()
>+{
>+  pthread_t th;
>+  barrier_init (&barrier, 2);
>+  if (pthread_create (&th, NULL, tf, NULL))
>+    return 0;
>+  v++;
>+  barrier_wait (&barrier);
>+  pthread_join (th, NULL);
>+  return 0;
>+}
>+
>+/* { dg-output "WARNING: ThreadSanitizer: data race.*#2 _?tf" } */
>--- gcc/testsuite/c-c++-common/tsan/pr65400-2.c.jj	2015-03-18
>18:40:07.855433878 +0100
>+++ gcc/testsuite/c-c++-common/tsan/pr65400-2.c	2015-03-18
>18:40:03.862498763 +0100
>@@ -0,0 +1,10 @@
>+/* PR sanitizer/65400 */
>+/* { dg-do compile } */
>+
>+extern void foo (int *);
>+
>+void
>+baz4 (int *p)
>+{
>+  foo (p);
>+}
>
>	Jakub