On Thu, 29 Nov 2012, Jakub Jelinek wrote:
> Hi!
>
> As discussed earlier on mailing list and on IRC with Richard,
> pt_solution_includes can be used for testing of whether var
> might escape current function (for -O0 unfortunately returns
> true even for !may_be_aliased automatic vars, so I'm testing
> for that too), and there are IMHO tons of completely unnecessary tests
> and restrictions what we want to instrument and what not.
> instrument_expr is guarded with gimple_store_p or
> gimple_assign_load_p, so it shouldn't see SSA_NAMEs, and many other
> ops, basically just DECL_Ps, handled_components, MEM_REF or TARGET_MEM_REF
> where for handled_components the base address is one of DECL_P, MEM_REF
> or TARGET_MEM_REF.
>
> I'm actually surprised that insturment_expr handles also is_gimple_call
> in it, yet nothing calls it for calls, I bet for calls we also want to call
> it for gimple_store_p calls.
>
> Dmitry, could you please test this on something where the previous tsan
> code has been tested on (if anything)? All I've done was eyeball a few
> short testcases.
>
> TSAN will badly need similar optimization pass to what ASAN needs (after
> deferring expansion of the shadow memory checks), e.g. var++ right now
> results in __tsan_read4 (&var); followed soon by __tsan_write4 (&var);.
> With no intervening calls (we could ignore many string/memory builtins
> I guess) and no intervening atomics it should be fine to just use
> __tsan_write4 (&var); for that, right? Similarly for multiple accesses
> to the same var, where first use dominates the other accesses and there
> are no intervening calls.
>
> As for bitfields, I think we want to use DECL_BIT_FIELD_REPRESENTATIVE.
>From a middle-end POV this looks ok.
Thanks,
Richard.
> 2012-11-29 Jakub Jelinek <ja...@redhat.com>
>
> * tsan.c (is_load_of_const_p): Removed.
> (instrument_expr): Use result of get_inner_reference
> instead of get_base_address, avoid some unnecessary tests,
> use !pt_solution_includes and !may_be_aliased tests to
> check whether base might escape current function.
>
> --- gcc/tsan.c.jj 2012-11-23 15:27:38.000000000 +0100
> +++ gcc/tsan.c 2012-11-29 12:41:26.816857288 +0100
> @@ -84,65 +84,18 @@ is_vptr_store (gimple stmt, tree expr, b
> return NULL;
> }
>
> -/* Checks as to whether EXPR refers to constant var/field/param.
> - Don't bother to instrument them. */
> -
> -static bool
> -is_load_of_const_p (tree expr, bool is_write)
> -{
> - if (is_write)
> - return false;
> - if (TREE_CODE (expr) == COMPONENT_REF)
> - expr = TREE_OPERAND (expr, 1);
> - if (TREE_CODE (expr) == VAR_DECL
> - || TREE_CODE (expr) == PARM_DECL
> - || TREE_CODE (expr) == FIELD_DECL)
> - {
> - if (TREE_READONLY (expr))
> - return true;
> - }
> - return false;
> -}
> -
> /* Instruments EXPR if needed. If any instrumentation is inserted,
> return true. */
>
> static bool
> instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write)
> {
> - enum tree_code tcode;
> tree base, rhs, expr_type, expr_ptr, builtin_decl;
> basic_block bb;
> HOST_WIDE_INT size;
> gimple stmt, g;
> location_t loc;
>
> - base = get_base_address (expr);
> - if (base == NULL_TREE
> - || TREE_CODE (base) == SSA_NAME
> - || TREE_CODE (base) == STRING_CST)
> - return false;
> -
> - tcode = TREE_CODE (expr);
> -
> - /* Below are things we do not instrument
> - (no possibility of races or not implemented yet). */
> - if (/* Compiler-emitted artificial variables. */
> - (DECL_P (expr) && DECL_ARTIFICIAL (expr))
> - /* The var does not live in memory -> no possibility of races. */
> - || (tcode == VAR_DECL
> - && !TREE_ADDRESSABLE (expr)
> - && TREE_STATIC (expr) == 0)
> - /* Not implemented. */
> - || TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE
> - /* Not implemented. */
> - || tcode == CONSTRUCTOR
> - /* Not implemented. */
> - || tcode == PARM_DECL
> - /* Load of a const variable/parameter/field. */
> - || is_load_of_const_p (expr, is_write))
> - return false;
> -
> size = int_size_in_bytes (TREE_TYPE (expr));
> if (size == -1)
> return false;
> @@ -153,18 +106,29 @@ instrument_expr (gimple_stmt_iterator gs
> tree offset;
> enum machine_mode mode;
> int volatilep = 0, unsignedp = 0;
> - get_inner_reference (expr, &bitsize, &bitpos, &offset,
> - &mode, &unsignedp, &volatilep, false);
> - if (bitpos % (size * BITS_PER_UNIT)
> - || bitsize != size * BITS_PER_UNIT)
> + base = get_inner_reference (expr, &bitsize, &bitpos, &offset,
> + &mode, &unsignedp, &volatilep, false);
> +
> + /* No need to instrument accesses to decls that don't escape,
> + they can't escape to other threads then. */
> + if (DECL_P (base))
> + {
> + struct pt_solution pt;
> + memset (&pt, 0, sizeof (pt));
> + pt.escaped = 1;
> + pt.ipa_escaped = flag_ipa_pta != 0;
> + pt.nonlocal = 1;
> + if (!pt_solution_includes (&pt, base))
> + return false;
> + if (!is_global_var (base) && !may_be_aliased (base))
> + return false;
> + }
> +
> + if (TREE_READONLY (base))
> return false;
>
> - /* TODO: handle other case: ARRAY_RANGE_REF. */
> - if (tcode != ARRAY_REF
> - && tcode != VAR_DECL
> - && tcode != COMPONENT_REF
> - && tcode != INDIRECT_REF
> - && tcode != MEM_REF)
> + if (bitpos % (size * BITS_PER_UNIT)
> + || bitsize != size * BITS_PER_UNIT)
> return false;
>
> stmt = gsi_stmt (gsi);
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend
