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

[Richard Biener <rguenther@suse.de>]

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)