[PATCH] -fsanitize=thread fixes (PR sanitizer/68260)

[PATCH] -fsanitize=thread fixes (PR sanitizer/68260)

[Jakub Jelinek]

Hi!

As the PRs show, -fsanitize=thread hasn't been instrumenting __atomic_clear
and __atomic_test_and_set builtins (i.e. those that operate on bool always),
so we reported a data race on the testcase even when there wasn't one.
And, as the other testcase shows, there were various ICEs with
-fnon-call-exceptions -fsanitize=thread.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2016-09-06  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/68260
	* tsan.c: Include target.h and tree-eh.h.
	(enum tsan_atomic_action): Add bool_clear and bool_test_and_set.
	(BOOL_CLEAR, BOOL_TEST_AND_SET): Define.
	(tsan_atomic_table): Add BUILT_IN_ATOMIC_CLEAR and
	BUILT_IN_ATOMIC_TEST_AND_SET entries.
	(instrument_builtin_call): Remove EH edges of replaced call stmts.
	Handle bool_clear and bool_test_and_set.
	(instrument_memory_accesses): Add RET argument, purge dead eh edges
	at the end of bbs and set *RET in that case to TODO_cleanup_cfg.
	(tsan_pass): Adjust instrument_memory_accesses caller, return ret.

	* c-c++-common/tsan/pr68260.c: New test.
	* g++.dg/tsan/pr68260.C: New test.

--- gcc/tsan.c.jj	2016-07-11 22:18:08.000000000 +0200
+++ gcc/tsan.c	2016-09-06 14:06:21.193642590 +0200
@@ -40,6 +40,8 @@ along with GCC; see the file COPYING3.
 #include "tsan.h"
 #include "asan.h"
 #include "builtins.h"
+#include "target.h"
+#include "tree-eh.h"
 
 /* Number of instrumented memory accesses in the current function.  */
 
@@ -240,7 +242,8 @@ instrument_expr (gimple_stmt_iterator gs
 enum tsan_atomic_action
 {
   check_last, add_seq_cst, add_acquire, weak_cas, strong_cas,
-  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst
+  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst,
+  bool_clear, bool_test_and_set
 };
 
 /* Table how to map sync/atomic builtins to their corresponding
@@ -274,6 +277,10 @@ static const struct tsan_map_atomic
   TRANSFORM (fcode, tsan_fcode, fetch_op, code)
 #define FETCH_OPS(fcode, tsan_fcode, code) \
   TRANSFORM (fcode, tsan_fcode, fetch_op_seq_cst, code)
+#define BOOL_CLEAR(fcode, tsan_fcode) \
+  TRANSFORM (fcode, tsan_fcode, bool_clear, ERROR_MARK)
+#define BOOL_TEST_AND_SET(fcode, tsan_fcode) \
+  TRANSFORM (fcode, tsan_fcode, bool_test_and_set, ERROR_MARK)
 
   CHECK_LAST (ATOMIC_LOAD_1, TSAN_ATOMIC8_LOAD),
   CHECK_LAST (ATOMIC_LOAD_2, TSAN_ATOMIC16_LOAD),
@@ -463,7 +470,11 @@ static const struct tsan_map_atomic
   LOCK_RELEASE (SYNC_LOCK_RELEASE_2, TSAN_ATOMIC16_STORE),
   LOCK_RELEASE (SYNC_LOCK_RELEASE_4, TSAN_ATOMIC32_STORE),
   LOCK_RELEASE (SYNC_LOCK_RELEASE_8, TSAN_ATOMIC64_STORE),
-  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE)
+  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE),
+
+  BOOL_CLEAR (ATOMIC_CLEAR, TSAN_ATOMIC8_STORE),
+
+  BOOL_TEST_AND_SET (ATOMIC_TEST_AND_SET, TSAN_ATOMIC8_EXCHANGE)
 };
 
 /* Instrument an atomic builtin.  */
@@ -493,6 +504,8 @@ instrument_builtin_call (gimple_stmt_ite
 	    if (!tree_fits_uhwi_p (last_arg)
 		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
 	      return;
+	    if (lookup_stmt_eh_lp (stmt))
+	      remove_stmt_from_eh_lp (stmt);
 	    gimple_call_set_fndecl (stmt, decl);
 	    update_stmt (stmt);
 	    if (tsan_atomic_table[i].action == fetch_op)
@@ -514,6 +527,8 @@ instrument_builtin_call (gimple_stmt_ite
 				       != add_acquire
 				       ? MEMMODEL_SEQ_CST
 				       : MEMMODEL_ACQUIRE);
+	    if (lookup_stmt_eh_lp (stmt))
+	      remove_stmt_from_eh_lp (stmt);
 	    update_gimple_call (gsi, decl, num + 1, args[0], args[1], args[2]);
 	    stmt = gsi_stmt (*gsi);
 	    if (tsan_atomic_table[i].action == fetch_op_seq_cst)
@@ -561,6 +576,8 @@ instrument_builtin_call (gimple_stmt_ite
 	    if (!tree_fits_uhwi_p (args[5])
 		|| memmodel_base (tree_to_uhwi (args[5])) >= MEMMODEL_LAST)
 	      return;
+	    if (lookup_stmt_eh_lp (stmt))
+	      remove_stmt_from_eh_lp (stmt);
 	    update_gimple_call (gsi, decl, 5, args[0], args[1], args[2],
 				args[4], args[5]);
 	    return;
@@ -584,6 +601,8 @@ instrument_builtin_call (gimple_stmt_ite
 	    g = gimple_build_assign (t, args[1]);
 	    gsi_insert_before (gsi, g, GSI_SAME_STMT);
 	    lhs = gimple_call_lhs (stmt);
+	    if (lookup_stmt_eh_lp (stmt))
+	      remove_stmt_from_eh_lp (stmt);
 	    update_gimple_call (gsi, decl, 5, args[0],
 				build_fold_addr_expr (t), args[2],
 				build_int_cst (NULL_TREE,
@@ -610,11 +629,66 @@ instrument_builtin_call (gimple_stmt_ite
 	    gcc_assert (num == 1);
 	    t = TYPE_ARG_TYPES (TREE_TYPE (decl));
 	    t = TREE_VALUE (TREE_CHAIN (t));
+	    if (lookup_stmt_eh_lp (stmt))
+	      remove_stmt_from_eh_lp (stmt);
 	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
 				build_int_cst (t, 0),
 				build_int_cst (NULL_TREE,
 					       MEMMODEL_RELEASE));
 	    return;
+	  case bool_clear:
+	  case bool_test_and_set:
+	    if (BOOL_TYPE_SIZE != 8)
+	      {
+		decl = NULL_TREE;
+		for (j = 1; j < 5; j++)
+		  if (BOOL_TYPE_SIZE == (8 << j))
+		    {
+		      enum built_in_function tsan_fcode
+			= (enum built_in_function)
+			  (tsan_atomic_table[i].tsan_fcode + j);
+		      decl = builtin_decl_implicit (tsan_fcode);
+		      break;
+		    }
+		if (decl == NULL_TREE)
+		  return;
+	      }
+	    last_arg = gimple_call_arg (stmt, num - 1);
+	    if (!tree_fits_uhwi_p (last_arg)
+		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
+	      return;
+	    t = TYPE_ARG_TYPES (TREE_TYPE (decl));
+	    t = TREE_VALUE (TREE_CHAIN (t));
+	    if (lookup_stmt_eh_lp (stmt))
+	      remove_stmt_from_eh_lp (stmt);
+	    if (tsan_atomic_table[i].action == bool_clear)
+	      {
+		update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
+				    build_int_cst (t, 0), last_arg);
+		return;
+	      }
+	    t = build_int_cst (t, targetm.atomic_test_and_set_trueval);
+	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
+				t, last_arg);
+	    stmt = gsi_stmt (*gsi);
+	    lhs = gimple_call_lhs (stmt);
+	    if (lhs == NULL_TREE)
+	      return;
+	    if (targetm.atomic_test_and_set_trueval != 1
+		|| !useless_type_conversion_p (TREE_TYPE (lhs),
+					       TREE_TYPE (t)))
+	      {
+		tree new_lhs = make_ssa_name (TREE_TYPE (t));
+		gimple_call_set_lhs (stmt, new_lhs);
+		if (targetm.atomic_test_and_set_trueval != 1)
+		  g = gimple_build_assign (lhs, NE_EXPR, new_lhs,
+					   build_int_cst (TREE_TYPE (t), 0));
+		else
+		  g = gimple_build_assign (lhs, NOP_EXPR, new_lhs);
+		gsi_insert_after (gsi, g, GSI_NEW_STMT);
+		update_stmt (stmt);
+	      }
+	    return;
 	  default:
 	    continue;
 	  }
@@ -706,7 +780,7 @@ instrument_func_exit (void)
    Return true if func entry/exit should be instrumented.  */
 
 static bool
-instrument_memory_accesses (void)
+instrument_memory_accesses (unsigned int *ret)
 {
   basic_block bb;
   gimple_stmt_iterator gsi;
@@ -715,22 +789,26 @@ instrument_memory_accesses (void)
   auto_vec<gimple *> tsan_func_exits;
 
   FOR_EACH_BB_FN (bb, cfun)
-    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-      {
-	gimple *stmt = gsi_stmt (gsi);
-	if (is_gimple_call (stmt)
-	    && gimple_call_internal_p (stmt)
-	    && gimple_call_internal_fn (stmt) == IFN_TSAN_FUNC_EXIT)
-	  {
-	    if (fentry_exit_instrument)
-	      replace_func_exit (stmt);
-	    else
-	      tsan_func_exits.safe_push (stmt);
-	    func_exit_seen = true;
-	  }
-	else
-	  fentry_exit_instrument |= instrument_gimple (&gsi);
-      }
+    {
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (is_gimple_call (stmt)
+	      && gimple_call_internal_p (stmt)
+	      && gimple_call_internal_fn (stmt) == IFN_TSAN_FUNC_EXIT)
+	    {
+	      if (fentry_exit_instrument)
+		replace_func_exit (stmt);
+	      else
+		tsan_func_exits.safe_push (stmt);
+	      func_exit_seen = true;
+	    }
+	  else
+	    fentry_exit_instrument |= instrument_gimple (&gsi);
+	}
+      if (gimple_purge_dead_eh_edges (bb))
+	*ret = TODO_cleanup_cfg;
+    }
   unsigned int i;
   gimple *stmt;
   FOR_EACH_VEC_ELT (tsan_func_exits, i, stmt)
@@ -776,10 +854,11 @@ instrument_func_entry (void)
 static unsigned
 tsan_pass (void)
 {
+  unsigned int ret = 0;
   initialize_sanitizer_builtins ();
-  if (instrument_memory_accesses ())
+  if (instrument_memory_accesses (&ret))
     instrument_func_entry ();
-  return 0;
+  return ret;
 }
 
 /* Inserts __tsan_init () into the list of CTORs.  */
--- gcc/testsuite/c-c++-common/tsan/pr68260.c.jj	2016-09-06 15:56:27.772209548 +0200
+++ gcc/testsuite/c-c++-common/tsan/pr68260.c	2016-09-06 16:08:23.424203133 +0200
@@ -0,0 +1,28 @@
+/* PR sanitizer/68260 */
+
+#include <pthread.h>
+#include <stdbool.h>
+
+bool lock;
+int counter;
+
+void *
+tf (void *arg)
+{
+  (void) arg;
+  while (__atomic_test_and_set (&lock, __ATOMIC_ACQUIRE))
+    ;
+  ++counter;
+  __atomic_clear (&lock, __ATOMIC_RELEASE);
+  return (void *) 0;
+}
+
+int
+main ()
+{
+  pthread_t thr;
+  pthread_create (&thr, 0, tf, 0);
+  tf ((void *) 0);
+  pthread_join (thr, 0);
+  return 0;
+}
--- gcc/testsuite/g++.dg/tsan/pr68260.C.jj	2016-09-06 15:58:41.118530940 +0200
+++ gcc/testsuite/g++.dg/tsan/pr68260.C	2016-09-06 16:03:11.988121144 +0200
@@ -0,0 +1,19 @@
+/* PR sanitizer/68260 */
+/* { dg-do compile } */
+/* { dg-additional-options "-fnon-call-exceptions" } */
+
+int
+foo (bool *p, int *q)
+{
+  try
+    {
+      while (__atomic_test_and_set (p, __ATOMIC_ACQUIRE))
+	;
+      __atomic_clear (p, __ATOMIC_RELEASE);
+      return __sync_fetch_and_add_4 (q, 4);
+    }
+  catch (...)
+    {
+      return -1;
+    }
+}

	Jakub

Re: [PATCH] -fsanitize=thread fixes (PR sanitizer/68260)

[Richard Biener]

On Tue, 6 Sep 2016, Jakub Jelinek wrote:

> Hi!
> 
> As the PRs show, -fsanitize=thread hasn't been instrumenting __atomic_clear
> and __atomic_test_and_set builtins (i.e. those that operate on bool always),
> so we reported a data race on the testcase even when there wasn't one.
> And, as the other testcase shows, there were various ICEs with
> -fnon-call-exceptions -fsanitize=thread.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
> 
> 2016-09-06  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/68260
> 	* tsan.c: Include target.h and tree-eh.h.
> 	(enum tsan_atomic_action): Add bool_clear and bool_test_and_set.
> 	(BOOL_CLEAR, BOOL_TEST_AND_SET): Define.
> 	(tsan_atomic_table): Add BUILT_IN_ATOMIC_CLEAR and
> 	BUILT_IN_ATOMIC_TEST_AND_SET entries.
> 	(instrument_builtin_call): Remove EH edges of replaced call stmts.
> 	Handle bool_clear and bool_test_and_set.
> 	(instrument_memory_accesses): Add RET argument, purge dead eh edges
> 	at the end of bbs and set *RET in that case to TODO_cleanup_cfg.
> 	(tsan_pass): Adjust instrument_memory_accesses caller, return ret.
> 
> 	* c-c++-common/tsan/pr68260.c: New test.
> 	* g++.dg/tsan/pr68260.C: New test.
> 
> --- gcc/tsan.c.jj	2016-07-11 22:18:08.000000000 +0200
> +++ gcc/tsan.c	2016-09-06 14:06:21.193642590 +0200
> @@ -40,6 +40,8 @@ along with GCC; see the file COPYING3.
>  #include "tsan.h"
>  #include "asan.h"
>  #include "builtins.h"
> +#include "target.h"
> +#include "tree-eh.h"
>  
>  /* Number of instrumented memory accesses in the current function.  */
>  
> @@ -240,7 +242,8 @@ instrument_expr (gimple_stmt_iterator gs
>  enum tsan_atomic_action
>  {
>    check_last, add_seq_cst, add_acquire, weak_cas, strong_cas,
> -  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst
> +  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst,
> +  bool_clear, bool_test_and_set
>  };
>  
>  /* Table how to map sync/atomic builtins to their corresponding
> @@ -274,6 +277,10 @@ static const struct tsan_map_atomic
>    TRANSFORM (fcode, tsan_fcode, fetch_op, code)
>  #define FETCH_OPS(fcode, tsan_fcode, code) \
>    TRANSFORM (fcode, tsan_fcode, fetch_op_seq_cst, code)
> +#define BOOL_CLEAR(fcode, tsan_fcode) \
> +  TRANSFORM (fcode, tsan_fcode, bool_clear, ERROR_MARK)
> +#define BOOL_TEST_AND_SET(fcode, tsan_fcode) \
> +  TRANSFORM (fcode, tsan_fcode, bool_test_and_set, ERROR_MARK)
>  
>    CHECK_LAST (ATOMIC_LOAD_1, TSAN_ATOMIC8_LOAD),
>    CHECK_LAST (ATOMIC_LOAD_2, TSAN_ATOMIC16_LOAD),
> @@ -463,7 +470,11 @@ static const struct tsan_map_atomic
>    LOCK_RELEASE (SYNC_LOCK_RELEASE_2, TSAN_ATOMIC16_STORE),
>    LOCK_RELEASE (SYNC_LOCK_RELEASE_4, TSAN_ATOMIC32_STORE),
>    LOCK_RELEASE (SYNC_LOCK_RELEASE_8, TSAN_ATOMIC64_STORE),
> -  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE)
> +  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE),
> +
> +  BOOL_CLEAR (ATOMIC_CLEAR, TSAN_ATOMIC8_STORE),
> +
> +  BOOL_TEST_AND_SET (ATOMIC_TEST_AND_SET, TSAN_ATOMIC8_EXCHANGE)
>  };
>  
>  /* Instrument an atomic builtin.  */
> @@ -493,6 +504,8 @@ instrument_builtin_call (gimple_stmt_ite
>  	    if (!tree_fits_uhwi_p (last_arg)
>  		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
>  	      return;
> +	    if (lookup_stmt_eh_lp (stmt))
> +	      remove_stmt_from_eh_lp (stmt);

These changes look bogus to me -- how can the tsan instrumentation
function _not_ throw when the original function we replace it can?

It seems you are just avoiding the ICEs for now "wrong-code".  (and
how does this relate to -fnon-call-exceptions as both are calls?)

The instrument_expr case seems to leave the original stmt in-place
(and thus the EH).

Richard.

>  	    gimple_call_set_fndecl (stmt, decl);
>  	    update_stmt (stmt);
>  	    if (tsan_atomic_table[i].action == fetch_op)
> @@ -514,6 +527,8 @@ instrument_builtin_call (gimple_stmt_ite
>  				       != add_acquire
>  				       ? MEMMODEL_SEQ_CST
>  				       : MEMMODEL_ACQUIRE);
> +	    if (lookup_stmt_eh_lp (stmt))
> +	      remove_stmt_from_eh_lp (stmt);
>  	    update_gimple_call (gsi, decl, num + 1, args[0], args[1], args[2]);
>  	    stmt = gsi_stmt (*gsi);
>  	    if (tsan_atomic_table[i].action == fetch_op_seq_cst)
> @@ -561,6 +576,8 @@ instrument_builtin_call (gimple_stmt_ite
>  	    if (!tree_fits_uhwi_p (args[5])
>  		|| memmodel_base (tree_to_uhwi (args[5])) >= MEMMODEL_LAST)
>  	      return;
> +	    if (lookup_stmt_eh_lp (stmt))
> +	      remove_stmt_from_eh_lp (stmt);
>  	    update_gimple_call (gsi, decl, 5, args[0], args[1], args[2],
>  				args[4], args[5]);
>  	    return;
> @@ -584,6 +601,8 @@ instrument_builtin_call (gimple_stmt_ite
>  	    g = gimple_build_assign (t, args[1]);
>  	    gsi_insert_before (gsi, g, GSI_SAME_STMT);
>  	    lhs = gimple_call_lhs (stmt);
> +	    if (lookup_stmt_eh_lp (stmt))
> +	      remove_stmt_from_eh_lp (stmt);
>  	    update_gimple_call (gsi, decl, 5, args[0],
>  				build_fold_addr_expr (t), args[2],
>  				build_int_cst (NULL_TREE,
> @@ -610,11 +629,66 @@ instrument_builtin_call (gimple_stmt_ite
>  	    gcc_assert (num == 1);
>  	    t = TYPE_ARG_TYPES (TREE_TYPE (decl));
>  	    t = TREE_VALUE (TREE_CHAIN (t));
> +	    if (lookup_stmt_eh_lp (stmt))
> +	      remove_stmt_from_eh_lp (stmt);
>  	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
>  				build_int_cst (t, 0),
>  				build_int_cst (NULL_TREE,
>  					       MEMMODEL_RELEASE));
>  	    return;
> +	  case bool_clear:
> +	  case bool_test_and_set:
> +	    if (BOOL_TYPE_SIZE != 8)
> +	      {
> +		decl = NULL_TREE;
> +		for (j = 1; j < 5; j++)
> +		  if (BOOL_TYPE_SIZE == (8 << j))
> +		    {
> +		      enum built_in_function tsan_fcode
> +			= (enum built_in_function)
> +			  (tsan_atomic_table[i].tsan_fcode + j);
> +		      decl = builtin_decl_implicit (tsan_fcode);
> +		      break;
> +		    }
> +		if (decl == NULL_TREE)
> +		  return;
> +	      }
> +	    last_arg = gimple_call_arg (stmt, num - 1);
> +	    if (!tree_fits_uhwi_p (last_arg)
> +		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
> +	      return;
> +	    t = TYPE_ARG_TYPES (TREE_TYPE (decl));
> +	    t = TREE_VALUE (TREE_CHAIN (t));
> +	    if (lookup_stmt_eh_lp (stmt))
> +	      remove_stmt_from_eh_lp (stmt);
> +	    if (tsan_atomic_table[i].action == bool_clear)
> +	      {
> +		update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
> +				    build_int_cst (t, 0), last_arg);
> +		return;
> +	      }
> +	    t = build_int_cst (t, targetm.atomic_test_and_set_trueval);
> +	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
> +				t, last_arg);
> +	    stmt = gsi_stmt (*gsi);
> +	    lhs = gimple_call_lhs (stmt);
> +	    if (lhs == NULL_TREE)
> +	      return;
> +	    if (targetm.atomic_test_and_set_trueval != 1
> +		|| !useless_type_conversion_p (TREE_TYPE (lhs),
> +					       TREE_TYPE (t)))
> +	      {
> +		tree new_lhs = make_ssa_name (TREE_TYPE (t));
> +		gimple_call_set_lhs (stmt, new_lhs);
> +		if (targetm.atomic_test_and_set_trueval != 1)
> +		  g = gimple_build_assign (lhs, NE_EXPR, new_lhs,
> +					   build_int_cst (TREE_TYPE (t), 0));
> +		else
> +		  g = gimple_build_assign (lhs, NOP_EXPR, new_lhs);
> +		gsi_insert_after (gsi, g, GSI_NEW_STMT);
> +		update_stmt (stmt);
> +	      }
> +	    return;
>  	  default:
>  	    continue;
>  	  }
> @@ -706,7 +780,7 @@ instrument_func_exit (void)
>     Return true if func entry/exit should be instrumented.  */
>  
>  static bool
> -instrument_memory_accesses (void)
> +instrument_memory_accesses (unsigned int *ret)
>  {
>    basic_block bb;
>    gimple_stmt_iterator gsi;
> @@ -715,22 +789,26 @@ instrument_memory_accesses (void)
>    auto_vec<gimple *> tsan_func_exits;
>  
>    FOR_EACH_BB_FN (bb, cfun)
> -    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -      {
> -	gimple *stmt = gsi_stmt (gsi);
> -	if (is_gimple_call (stmt)
> -	    && gimple_call_internal_p (stmt)
> -	    && gimple_call_internal_fn (stmt) == IFN_TSAN_FUNC_EXIT)
> -	  {
> -	    if (fentry_exit_instrument)
> -	      replace_func_exit (stmt);
> -	    else
> -	      tsan_func_exits.safe_push (stmt);
> -	    func_exit_seen = true;
> -	  }
> -	else
> -	  fentry_exit_instrument |= instrument_gimple (&gsi);
> -      }
> +    {
> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +	{
> +	  gimple *stmt = gsi_stmt (gsi);
> +	  if (is_gimple_call (stmt)
> +	      && gimple_call_internal_p (stmt)
> +	      && gimple_call_internal_fn (stmt) == IFN_TSAN_FUNC_EXIT)
> +	    {
> +	      if (fentry_exit_instrument)
> +		replace_func_exit (stmt);
> +	      else
> +		tsan_func_exits.safe_push (stmt);
> +	      func_exit_seen = true;
> +	    }
> +	  else
> +	    fentry_exit_instrument |= instrument_gimple (&gsi);
> +	}
> +      if (gimple_purge_dead_eh_edges (bb))
> +	*ret = TODO_cleanup_cfg;
> +    }
>    unsigned int i;
>    gimple *stmt;
>    FOR_EACH_VEC_ELT (tsan_func_exits, i, stmt)
> @@ -776,10 +854,11 @@ instrument_func_entry (void)
>  static unsigned
>  tsan_pass (void)
>  {
> +  unsigned int ret = 0;
>    initialize_sanitizer_builtins ();
> -  if (instrument_memory_accesses ())
> +  if (instrument_memory_accesses (&ret))
>      instrument_func_entry ();
> -  return 0;
> +  return ret;
>  }
>  
>  /* Inserts __tsan_init () into the list of CTORs.  */
> --- gcc/testsuite/c-c++-common/tsan/pr68260.c.jj	2016-09-06 15:56:27.772209548 +0200
> +++ gcc/testsuite/c-c++-common/tsan/pr68260.c	2016-09-06 16:08:23.424203133 +0200
> @@ -0,0 +1,28 @@
> +/* PR sanitizer/68260 */
> +
> +#include <pthread.h>
> +#include <stdbool.h>
> +
> +bool lock;
> +int counter;
> +
> +void *
> +tf (void *arg)
> +{
> +  (void) arg;
> +  while (__atomic_test_and_set (&lock, __ATOMIC_ACQUIRE))
> +    ;
> +  ++counter;
> +  __atomic_clear (&lock, __ATOMIC_RELEASE);
> +  return (void *) 0;
> +}
> +
> +int
> +main ()
> +{
> +  pthread_t thr;
> +  pthread_create (&thr, 0, tf, 0);
> +  tf ((void *) 0);
> +  pthread_join (thr, 0);
> +  return 0;
> +}
> --- gcc/testsuite/g++.dg/tsan/pr68260.C.jj	2016-09-06 15:58:41.118530940 +0200
> +++ gcc/testsuite/g++.dg/tsan/pr68260.C	2016-09-06 16:03:11.988121144 +0200
> @@ -0,0 +1,19 @@
> +/* PR sanitizer/68260 */
> +/* { dg-do compile } */
> +/* { dg-additional-options "-fnon-call-exceptions" } */
> +
> +int
> +foo (bool *p, int *q)
> +{
> +  try
> +    {
> +      while (__atomic_test_and_set (p, __ATOMIC_ACQUIRE))
> +	;
> +      __atomic_clear (p, __ATOMIC_RELEASE);
> +      return __sync_fetch_and_add_4 (q, 4);
> +    }
> +  catch (...)
> +    {
> +      return -1;
> +    }
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] -fsanitize=thread fixes (PR sanitizer/68260)

[Jakub Jelinek]

On Wed, Sep 07, 2016 at 09:07:45AM +0200, Richard Biener wrote:
> > @@ -493,6 +504,8 @@ instrument_builtin_call (gimple_stmt_ite
> >  	    if (!tree_fits_uhwi_p (last_arg)
> >  		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
> >  	      return;
> > +	    if (lookup_stmt_eh_lp (stmt))
> > +	      remove_stmt_from_eh_lp (stmt);
> 
> These changes look bogus to me -- how can the tsan instrumentation
> function _not_ throw when the original function we replace it can?

The __tsan*atomic* functions are right now declared to be nothrow, so the
patch just matches how they are declared.
While the sync-builtins.def use
#define ATTR_NOTHROWCALL_LEAF_LIST (flag_non_call_exceptions ? \
        ATTR_LEAF_LIST : ATTR_NOTHROW_LEAF_LIST)
I guess I could use the same for the tsan atomics, but wonder if it will work
properly when the libtsan is built with exceptions disabled and without
-fnon-call-exceptions.  Perhaps it would, at least if it is built with
-fasynchronous-unwind-tables (which is the case for x86_64 and aarch64 and
tsan isn't supported elsewhere).
> 
> It seems you are just avoiding the ICEs for now "wrong-code".  (and
> how does this relate to -fnon-call-exceptions as both are calls?)
> 
> The instrument_expr case seems to leave the original stmt in-place
> (and thus the EH).

Those are different.  For loads and stores there is just added call that
logs in the load or store, but for atomics the atomics are done inside of
the library and the library tracks everything.

	Jakub

Re: [PATCH] -fsanitize=thread fixes (PR sanitizer/68260)

[Jakub Jelinek]

On Wed, Sep 07, 2016 at 10:40:20PM +0200, Jakub Jelinek wrote:
> On Wed, Sep 07, 2016 at 09:07:45AM +0200, Richard Biener wrote:
> > > @@ -493,6 +504,8 @@ instrument_builtin_call (gimple_stmt_ite
> > >  	    if (!tree_fits_uhwi_p (last_arg)
> > >  		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
> > >  	      return;
> > > +	    if (lookup_stmt_eh_lp (stmt))
> > > +	      remove_stmt_from_eh_lp (stmt);
> > 
> > These changes look bogus to me -- how can the tsan instrumentation
> > function _not_ throw when the original function we replace it can?
> 
> The __tsan*atomic* functions are right now declared to be nothrow, so the
> patch just matches how they are declared.
> While the sync-builtins.def use
> #define ATTR_NOTHROWCALL_LEAF_LIST (flag_non_call_exceptions ? \
>         ATTR_LEAF_LIST : ATTR_NOTHROW_LEAF_LIST)
> I guess I could use the same for the tsan atomics, but wonder if it will work
> properly when the libtsan is built with exceptions disabled and without
> -fnon-call-exceptions.  Perhaps it would, at least if it is built with
> -fasynchronous-unwind-tables (which is the case for x86_64 and aarch64 and
> tsan isn't supported elsewhere).

Actually, looking at the libsanitizer/tsan/ implementation, I have doubts
the atomics handling code would behave reasonably if an exception is thrown
during the atomic memory access, I'm afraid some classes wouldn't be
destructed.

I can split the patch into two, one dealing just with the
__atomic_clear/__atomic_test_and_set instrumentation and another for the
preexisting -fnon-call-exceptions ICEs.  For the latter, one possibility
would be to error out on the -fsanitize=thread -fnon-call-exceptions
combination.

	Jakub

[PATCH] -fsanitize=thread fixes (PR sanitizer/68260, take 2)

[Jakub Jelinek]

On Thu, Sep 08, 2016 at 02:34:18PM +0200, Jakub Jelinek wrote:
> I can split the patch into two, one dealing just with the
> __atomic_clear/__atomic_test_and_set instrumentation and another for the
> preexisting -fnon-call-exceptions ICEs.  For the latter, one possibility
> would be to error out on the -fsanitize=thread -fnon-call-exceptions
> combination.

Here is just the hopefully non-controversial first split part.
Ok for trunk?

2016-09-08  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/68260
	* tsan.c: Include target.h.
	(enum tsan_atomic_action): Add bool_clear and bool_test_and_set.
	(BOOL_CLEAR, BOOL_TEST_AND_SET): Define.
	(tsan_atomic_table): Add BUILT_IN_ATOMIC_CLEAR and
	BUILT_IN_ATOMIC_TEST_AND_SET entries.
	(instrument_builtin_call): Handle bool_clear and bool_test_and_set.

	* c-c++-common/tsan/pr68260.c: New test.

--- gcc/tsan.c.jj	2016-07-22 15:55:31.591287885 +0200
+++ gcc/tsan.c	2016-09-08 14:46:08.645027959 +0200
@@ -40,6 +40,7 @@ along with GCC; see the file COPYING3.
 #include "tsan.h"
 #include "asan.h"
 #include "builtins.h"
+#include "target.h"
 
 /* Number of instrumented memory accesses in the current function.  */
 
@@ -240,7 +241,8 @@ instrument_expr (gimple_stmt_iterator gs
 enum tsan_atomic_action
 {
   check_last, add_seq_cst, add_acquire, weak_cas, strong_cas,
-  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst
+  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst,
+  bool_clear, bool_test_and_set
 };
 
 /* Table how to map sync/atomic builtins to their corresponding
@@ -274,6 +276,10 @@ static const struct tsan_map_atomic
   TRANSFORM (fcode, tsan_fcode, fetch_op, code)
 #define FETCH_OPS(fcode, tsan_fcode, code) \
   TRANSFORM (fcode, tsan_fcode, fetch_op_seq_cst, code)
+#define BOOL_CLEAR(fcode, tsan_fcode) \
+  TRANSFORM (fcode, tsan_fcode, bool_clear, ERROR_MARK)
+#define BOOL_TEST_AND_SET(fcode, tsan_fcode) \
+  TRANSFORM (fcode, tsan_fcode, bool_test_and_set, ERROR_MARK)
 
   CHECK_LAST (ATOMIC_LOAD_1, TSAN_ATOMIC8_LOAD),
   CHECK_LAST (ATOMIC_LOAD_2, TSAN_ATOMIC16_LOAD),
@@ -463,7 +469,11 @@ static const struct tsan_map_atomic
   LOCK_RELEASE (SYNC_LOCK_RELEASE_2, TSAN_ATOMIC16_STORE),
   LOCK_RELEASE (SYNC_LOCK_RELEASE_4, TSAN_ATOMIC32_STORE),
   LOCK_RELEASE (SYNC_LOCK_RELEASE_8, TSAN_ATOMIC64_STORE),
-  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE)
+  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE),
+
+  BOOL_CLEAR (ATOMIC_CLEAR, TSAN_ATOMIC8_STORE),
+
+  BOOL_TEST_AND_SET (ATOMIC_TEST_AND_SET, TSAN_ATOMIC8_EXCHANGE)
 };
 
 /* Instrument an atomic builtin.  */
@@ -615,6 +625,57 @@ instrument_builtin_call (gimple_stmt_ite
 				build_int_cst (NULL_TREE,
 					       MEMMODEL_RELEASE));
 	    return;
+	  case bool_clear:
+	  case bool_test_and_set:
+	    if (BOOL_TYPE_SIZE != 8)
+	      {
+		decl = NULL_TREE;
+		for (j = 1; j < 5; j++)
+		  if (BOOL_TYPE_SIZE == (8 << j))
+		    {
+		      enum built_in_function tsan_fcode
+			= (enum built_in_function)
+			  (tsan_atomic_table[i].tsan_fcode + j);
+		      decl = builtin_decl_implicit (tsan_fcode);
+		      break;
+		    }
+		if (decl == NULL_TREE)
+		  return;
+	      }
+	    last_arg = gimple_call_arg (stmt, num - 1);
+	    if (!tree_fits_uhwi_p (last_arg)
+		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
+	      return;
+	    t = TYPE_ARG_TYPES (TREE_TYPE (decl));
+	    t = TREE_VALUE (TREE_CHAIN (t));
+	    if (tsan_atomic_table[i].action == bool_clear)
+	      {
+		update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
+				    build_int_cst (t, 0), last_arg);
+		return;
+	      }
+	    t = build_int_cst (t, targetm.atomic_test_and_set_trueval);
+	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
+				t, last_arg);
+	    stmt = gsi_stmt (*gsi);
+	    lhs = gimple_call_lhs (stmt);
+	    if (lhs == NULL_TREE)
+	      return;
+	    if (targetm.atomic_test_and_set_trueval != 1
+		|| !useless_type_conversion_p (TREE_TYPE (lhs),
+					       TREE_TYPE (t)))
+	      {
+		tree new_lhs = make_ssa_name (TREE_TYPE (t));
+		gimple_call_set_lhs (stmt, new_lhs);
+		if (targetm.atomic_test_and_set_trueval != 1)
+		  g = gimple_build_assign (lhs, NE_EXPR, new_lhs,
+					   build_int_cst (TREE_TYPE (t), 0));
+		else
+		  g = gimple_build_assign (lhs, NOP_EXPR, new_lhs);
+		gsi_insert_after (gsi, g, GSI_NEW_STMT);
+		update_stmt (stmt);
+	      }
+	    return;
 	  default:
 	    continue;
 	  }
--- gcc/testsuite/c-c++-common/tsan/pr68260.c.jj	2016-09-08 14:45:08.994860025 +0200
+++ gcc/testsuite/c-c++-common/tsan/pr68260.c	2016-09-08 14:45:08.994860025 +0200
@@ -0,0 +1,28 @@
+/* PR sanitizer/68260 */
+
+#include <pthread.h>
+#include <stdbool.h>
+
+bool lock;
+int counter;
+
+void *
+tf (void *arg)
+{
+  (void) arg;
+  while (__atomic_test_and_set (&lock, __ATOMIC_ACQUIRE))
+    ;
+  ++counter;
+  __atomic_clear (&lock, __ATOMIC_RELEASE);
+  return (void *) 0;
+}
+
+int
+main ()
+{
+  pthread_t thr;
+  pthread_create (&thr, 0, tf, 0);
+  tf ((void *) 0);
+  pthread_join (thr, 0);
+  return 0;
+}


	Jakub

Re: [PATCH] -fsanitize=thread fixes (PR sanitizer/68260, take 2)

[Richard Biener]

On Thu, 8 Sep 2016, Jakub Jelinek wrote:

> On Thu, Sep 08, 2016 at 02:34:18PM +0200, Jakub Jelinek wrote:
> > I can split the patch into two, one dealing just with the
> > __atomic_clear/__atomic_test_and_set instrumentation and another for the
> > preexisting -fnon-call-exceptions ICEs.  For the latter, one possibility
> > would be to error out on the -fsanitize=thread -fnon-call-exceptions
> > combination.
> 
> Here is just the hopefully non-controversial first split part.
> Ok for trunk?

Ok.

Thanks,
Richard.

> 2016-09-08  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/68260
> 	* tsan.c: Include target.h.
> 	(enum tsan_atomic_action): Add bool_clear and bool_test_and_set.
> 	(BOOL_CLEAR, BOOL_TEST_AND_SET): Define.
> 	(tsan_atomic_table): Add BUILT_IN_ATOMIC_CLEAR and
> 	BUILT_IN_ATOMIC_TEST_AND_SET entries.
> 	(instrument_builtin_call): Handle bool_clear and bool_test_and_set.
> 
> 	* c-c++-common/tsan/pr68260.c: New test.
> 
> --- gcc/tsan.c.jj	2016-07-22 15:55:31.591287885 +0200
> +++ gcc/tsan.c	2016-09-08 14:46:08.645027959 +0200
> @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3.
>  #include "tsan.h"
>  #include "asan.h"
>  #include "builtins.h"
> +#include "target.h"
>  
>  /* Number of instrumented memory accesses in the current function.  */
>  
> @@ -240,7 +241,8 @@ instrument_expr (gimple_stmt_iterator gs
>  enum tsan_atomic_action
>  {
>    check_last, add_seq_cst, add_acquire, weak_cas, strong_cas,
> -  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst
> +  bool_cas, val_cas, lock_release, fetch_op, fetch_op_seq_cst,
> +  bool_clear, bool_test_and_set
>  };
>  
>  /* Table how to map sync/atomic builtins to their corresponding
> @@ -274,6 +276,10 @@ static const struct tsan_map_atomic
>    TRANSFORM (fcode, tsan_fcode, fetch_op, code)
>  #define FETCH_OPS(fcode, tsan_fcode, code) \
>    TRANSFORM (fcode, tsan_fcode, fetch_op_seq_cst, code)
> +#define BOOL_CLEAR(fcode, tsan_fcode) \
> +  TRANSFORM (fcode, tsan_fcode, bool_clear, ERROR_MARK)
> +#define BOOL_TEST_AND_SET(fcode, tsan_fcode) \
> +  TRANSFORM (fcode, tsan_fcode, bool_test_and_set, ERROR_MARK)
>  
>    CHECK_LAST (ATOMIC_LOAD_1, TSAN_ATOMIC8_LOAD),
>    CHECK_LAST (ATOMIC_LOAD_2, TSAN_ATOMIC16_LOAD),
> @@ -463,7 +469,11 @@ static const struct tsan_map_atomic
>    LOCK_RELEASE (SYNC_LOCK_RELEASE_2, TSAN_ATOMIC16_STORE),
>    LOCK_RELEASE (SYNC_LOCK_RELEASE_4, TSAN_ATOMIC32_STORE),
>    LOCK_RELEASE (SYNC_LOCK_RELEASE_8, TSAN_ATOMIC64_STORE),
> -  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE)
> +  LOCK_RELEASE (SYNC_LOCK_RELEASE_16, TSAN_ATOMIC128_STORE),
> +
> +  BOOL_CLEAR (ATOMIC_CLEAR, TSAN_ATOMIC8_STORE),
> +
> +  BOOL_TEST_AND_SET (ATOMIC_TEST_AND_SET, TSAN_ATOMIC8_EXCHANGE)
>  };
>  
>  /* Instrument an atomic builtin.  */
> @@ -615,6 +625,57 @@ instrument_builtin_call (gimple_stmt_ite
>  				build_int_cst (NULL_TREE,
>  					       MEMMODEL_RELEASE));
>  	    return;
> +	  case bool_clear:
> +	  case bool_test_and_set:
> +	    if (BOOL_TYPE_SIZE != 8)
> +	      {
> +		decl = NULL_TREE;
> +		for (j = 1; j < 5; j++)
> +		  if (BOOL_TYPE_SIZE == (8 << j))
> +		    {
> +		      enum built_in_function tsan_fcode
> +			= (enum built_in_function)
> +			  (tsan_atomic_table[i].tsan_fcode + j);
> +		      decl = builtin_decl_implicit (tsan_fcode);
> +		      break;
> +		    }
> +		if (decl == NULL_TREE)
> +		  return;
> +	      }
> +	    last_arg = gimple_call_arg (stmt, num - 1);
> +	    if (!tree_fits_uhwi_p (last_arg)
> +		|| memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
> +	      return;
> +	    t = TYPE_ARG_TYPES (TREE_TYPE (decl));
> +	    t = TREE_VALUE (TREE_CHAIN (t));
> +	    if (tsan_atomic_table[i].action == bool_clear)
> +	      {
> +		update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
> +				    build_int_cst (t, 0), last_arg);
> +		return;
> +	      }
> +	    t = build_int_cst (t, targetm.atomic_test_and_set_trueval);
> +	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
> +				t, last_arg);
> +	    stmt = gsi_stmt (*gsi);
> +	    lhs = gimple_call_lhs (stmt);
> +	    if (lhs == NULL_TREE)
> +	      return;
> +	    if (targetm.atomic_test_and_set_trueval != 1
> +		|| !useless_type_conversion_p (TREE_TYPE (lhs),
> +					       TREE_TYPE (t)))
> +	      {
> +		tree new_lhs = make_ssa_name (TREE_TYPE (t));
> +		gimple_call_set_lhs (stmt, new_lhs);
> +		if (targetm.atomic_test_and_set_trueval != 1)
> +		  g = gimple_build_assign (lhs, NE_EXPR, new_lhs,
> +					   build_int_cst (TREE_TYPE (t), 0));
> +		else
> +		  g = gimple_build_assign (lhs, NOP_EXPR, new_lhs);
> +		gsi_insert_after (gsi, g, GSI_NEW_STMT);
> +		update_stmt (stmt);
> +	      }
> +	    return;
>  	  default:
>  	    continue;
>  	  }
> --- gcc/testsuite/c-c++-common/tsan/pr68260.c.jj	2016-09-08 14:45:08.994860025 +0200
> +++ gcc/testsuite/c-c++-common/tsan/pr68260.c	2016-09-08 14:45:08.994860025 +0200
> @@ -0,0 +1,28 @@
> +/* PR sanitizer/68260 */
> +
> +#include <pthread.h>
> +#include <stdbool.h>
> +
> +bool lock;
> +int counter;
> +
> +void *
> +tf (void *arg)
> +{
> +  (void) arg;
> +  while (__atomic_test_and_set (&lock, __ATOMIC_ACQUIRE))
> +    ;
> +  ++counter;
> +  __atomic_clear (&lock, __ATOMIC_RELEASE);
> +  return (void *) 0;
> +}
> +
> +int
> +main ()
> +{
> +  pthread_t thr;
> +  pthread_create (&thr, 0, tf, 0);
> +  tf ((void *) 0);
> +  pthread_join (thr, 0);
> +  return 0;
> +}
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)