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 <ja...@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 <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
