On March 18, 2015 11:36:09 PM GMT+01:00, Jakub Jelinek <ja...@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 <ja...@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
