On Tue, Nov 13, 2012 at 8:40 AM, Jakub Jelinek <ja...@redhat.com> wrote:
> On Mon, Nov 05, 2012 at 04:37:41PM -0800, Wei Mi wrote:
>> Thanks for the comments. I fix most of them except the setting of
>> TODO_.... The new patch.txt is attached.
>
> Please update the patch against trunk, it doesn't apply cleanly because
> of the asan commit. I took the liberty to do at least some formatting
> cleanups and other small tweaks against your patch to tsan.c.
>
>> >> + TODO_verify_all | TODO_update_ssa
>> >
>> > Ideally you shouldn't need TODO_update_ssa.
>> >
>>
>> I got error when I removed TODO_update_ssa, so I kept it.
>
> If it were just for the instrumentations, then you can easily update
> the vdef/vuse yourself, e.g. when inserting before a memory write,
> you can copy over the gimple_vuse of that write to gimple_vuse of the
> instrumentation call, create a new SSA_NAME for the gimple_vdef and
> and set the write's gimple_vuse to that new SSA_NAME. For call
> instrumentation it would be a tiny bit harder, but for the instrumentation
> of function entry/exit you'd need to find out the current vop at that point.
> So perhaps we can live with that, at least for now.
>
>> >> + | TODO_update_address_taken /* todo_flags_finish */
>> >
>> > And why this?
>> >
>>
>> If we generate tsan_read(&a) for a non-address taken static variable
>> a, we need to change a to be address taken, right?
>
> That is complete misunderstanding of what update_address_taken does.
> It removes TREE_ADDRESSABLE from addressables that are no longer
> addressable, rather than adding TREE_ADDRESSABLE bits.
It will do the latter too. See iv-opts.
> For the latter
> there is mark_addressable function.
This is certainly cheaper to use.
David
>>
>> Wrap builtin_decl_implicit in get_tsan_builtin_decl. If
>> builtin_decl_implicit return invalid decl, output error message and
>> then exit.
>
> I've moved that over just to the gate, eventually there should be a routine
> that will initialize the builtins if they aren't by the FE.
>
>> > Please avoid *'s at the beginning of comment continuation lines.
>> > Use is_ctrl_altering_stmt (stmt) to check whether the call must
>> > be the last stmt in a block or not.
>> > And, don't expect there is a single_succ_edge, there could be
>> > no edge at all (e.g. noreturn call), or there could be multiple
>> > edges.
>> >
>>
>> Fixed. Iterate every successive edge of current bb and insert stmt on
>> each edge.
>
> But wrongly, for one adding the same stmt to multiple basic blocks
> is going to crash terribly, but also you IMHO want to insert just
> on fallthru edge, if the routine throws or longjmps, the result isn't
> written.
>
> --- gcc/tsan.c.jj 2012-11-13 16:46:21.000000000 +0100
> +++ gcc/tsan.c 2012-11-13 17:22:56.054837754 +0100
> @@ -1,4 +1,4 @@
> -/* GCC instrumentation plugin for ThreadSanitizer.
> +/* GCC instrumentation plugin for ThreadSanitizer.
> Copyright (C) 2011, 2012 Free Software Foundation, Inc.
> Contributed by Dmitry Vyukov <dvyu...@google.com>
>
> @@ -44,36 +44,27 @@ along with GCC; see the file COPYING3.
> void __tsan_read/writeX (void *addr); */
>
> static tree
> -get_tsan_builtin_decl (enum built_in_function fcode)
> -{
> - tree decl = builtin_decl_implicit (fcode);
> - if (decl == NULL_TREE)
> - internal_error ("undefined builtin %s", built_in_names[fcode]);
> - return decl;
> -}
> -
> -static tree
> get_memory_access_decl (bool is_write, unsigned size)
> {
> enum built_in_function fcode;
>
> if (size <= 1)
> fcode = is_write ? BUILT_IN_TSAN_WRITE_1
> - : BUILT_IN_TSAN_READ_1;
> + : BUILT_IN_TSAN_READ_1;
> else if (size <= 3)
> - fcode = is_write ? BUILT_IN_TSAN_WRITE_2
> - : BUILT_IN_TSAN_READ_2;
> + fcode = is_write ? BUILT_IN_TSAN_WRITE_2
> + : BUILT_IN_TSAN_READ_2;
> else if (size <= 7)
> - fcode = is_write ? BUILT_IN_TSAN_WRITE_4
> - : BUILT_IN_TSAN_READ_4;
> + fcode = is_write ? BUILT_IN_TSAN_WRITE_4
> + : BUILT_IN_TSAN_READ_4;
> else if (size <= 15)
> - fcode = is_write ? BUILT_IN_TSAN_WRITE_8
> - : BUILT_IN_TSAN_READ_8;
> + fcode = is_write ? BUILT_IN_TSAN_WRITE_8
> + : BUILT_IN_TSAN_READ_8;
> else
> - fcode = is_write ? BUILT_IN_TSAN_WRITE_16
> - : BUILT_IN_TSAN_READ_16;
> + fcode = is_write ? BUILT_IN_TSAN_WRITE_16
> + : BUILT_IN_TSAN_READ_16;
>
> - return get_tsan_builtin_decl (fcode);
> + return builtin_decl_implicit (fcode);
> }
>
> /* Check as to whether EXPR refers to a store to vptr. */
> @@ -81,14 +72,14 @@ get_memory_access_decl (bool is_write, u
> static tree
> is_vptr_store (gimple stmt, tree expr, bool is_write)
> {
> - if (is_write == true
> + if (is_write == true
> && gimple_assign_single_p (stmt)
> && TREE_CODE (expr) == COMPONENT_REF)
> {
> tree field = TREE_OPERAND (expr, 1);
> if (TREE_CODE (field) == FIELD_DECL
> - && DECL_VIRTUAL_P (field))
> - return gimple_assign_rhs1 (stmt);
> + && DECL_VIRTUAL_P (field))
> + return gimple_assign_rhs1 (stmt);
> }
> return NULL;
> }
> @@ -96,7 +87,7 @@ is_vptr_store (gimple stmt, tree expr, b
> /* Checks as to whether EXPR refers to constant var/field/param.
> Don't bother to instrument them. */
>
> -static bool
> +static bool
> is_load_of_const_p (tree expr, bool is_write)
> {
> if (is_write)
> @@ -108,7 +99,7 @@ is_load_of_const_p (tree expr, bool is_w
> || TREE_CODE (expr) == FIELD_DECL)
> {
> if (TREE_READONLY (expr))
> - return true;
> + return true;
> }
> return false;
> }
> @@ -116,18 +107,14 @@ is_load_of_const_p (tree expr, bool is_w
> /* Instruments EXPR if needed. If any instrumentation is inserted,
> * return true. */
>
> -static bool
> +static bool
> instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write)
> {
> enum tree_code tcode;
> - unsigned fld_off, fld_size;
> tree base, rhs, expr_type, expr_ptr, builtin_decl;
> - basic_block bb, succ_bb;
> - edge_iterator ei;
> - edge e;
> + basic_block bb;
> HOST_WIDE_INT size;
> gimple stmt, g;
> - gimple_stmt_iterator start_gsi;
> location_t loc;
>
> base = get_base_address (expr);
> @@ -144,8 +131,8 @@ instrument_expr (gimple_stmt_iterator gs
> (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)
> + && !TREE_ADDRESSABLE (expr)
> + && TREE_STATIC (expr) == 0)
> /* Not implemented. */
> || TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE
> /* Not implemented. */
> @@ -156,22 +143,21 @@ instrument_expr (gimple_stmt_iterator gs
> || is_load_of_const_p (expr, is_write))
> return false;
>
> - if (tcode == COMPONENT_REF)
> - {
> - tree field = TREE_OPERAND (expr, 1);
> - if (TREE_CODE (field) == FIELD_DECL)
> - {
> - fld_off = TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field));
> - fld_size = TREE_INT_CST_LOW (DECL_SIZE (field));
> - if (((fld_off % BITS_PER_UNIT) != 0)
> - || ((fld_size % BITS_PER_UNIT) != 0))
> - {
> - /* As of now it crashes compilation.
> - TODO: handle bit-fields as if touching the whole field. */
> - return false;
> - }
> - }
> - }
> + size = int_size_in_bytes (TREE_TYPE (expr));
> + if (size == -1)
> + return false;
> +
> + /* For now just avoid instrumenting bit field acceses.
> + TODO: handle bit-fields as if touching the whole field. */
> + HOST_WIDE_INT bitsize, bitpos;
> + 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)
> + return false;
>
> /* TODO: handle other cases
> (FIELD_DECL, ARRAY_RANGE_REF, TARGET_MEM_REF, ADDR_EXPR). */
> @@ -186,44 +172,42 @@ instrument_expr (gimple_stmt_iterator gs
> loc = gimple_location (stmt);
> rhs = is_vptr_store (stmt, expr, is_write);
> gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
> - expr_ptr = build_addr (unshare_expr (expr),
> - current_function_decl);
> + expr_ptr = build_fold_addr_expr (unshare_expr (expr));
> if (rhs == NULL)
> {
> expr_type = TREE_TYPE (expr);
> while (TREE_CODE (expr_type) == ARRAY_TYPE)
> - expr_type = TREE_TYPE (expr_type);
> - size = int_size_in_bytes (expr_type);
> + expr_type = TREE_TYPE (expr_type);
> + size = int_size_in_bytes (expr_type);
> g = gimple_build_call (get_memory_access_decl (is_write, size),
> - 1, expr_ptr);
> + 1, expr_ptr);
> }
> else
> {
> - builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_VPTR_UPDATE);
> + builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
> g = gimple_build_call (builtin_decl, 1, expr_ptr);
> }
> - gimple_set_location (g, loc);
> + gimple_set_location (g, loc);
> /* Instrumentation for assignment of a function result
> must be inserted after the call. Instrumentation for
> reads of function arguments must be inserted before the call.
> That's because the call can contain synchronization. */
> - if (is_gimple_call (stmt) && is_write)
> + if (is_gimple_call (stmt) && is_write)
> {
> /* If the call can throw, it must be the last stmt in
> - a basicblock, so the instrumented stmts need to be
> - inserted in successor bbs. */
> - if (is_ctrl_altering_stmt (stmt))
> - {
> - bb = gsi_bb (gsi);
> - FOR_EACH_EDGE (e, ei, bb->succs)
> - {
> - succ_bb = split_edge (e);
> - start_gsi = gsi_start_bb (succ_bb);
> - gsi_insert_after (&start_gsi, g, GSI_NEW_STMT);
> - }
> - }
> + a basic block, so the instrumented stmts need to be
> + inserted in successor bbs. */
> + if (is_ctrl_altering_stmt (stmt))
> + {
> + edge e;
> +
> + bb = gsi_bb (gsi);
> + e = find_fallthru_edge (bb->succs);
> + if (e)
> + gsi_insert_seq_on_edge_immediate (e, g);
> + }
> else
> - gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> + gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> }
> else
> gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> @@ -242,22 +226,22 @@ instrument_gimple (gimple_stmt_iterator
> bool instrumented = false;
>
> stmt = gsi_stmt (gsi);
> - if (is_gimple_call (stmt)
> - && (gimple_call_fndecl (stmt)
> - != get_tsan_builtin_decl (BUILT_IN_TSAN_INIT)))
> - return true;
> + if (is_gimple_call (stmt)
> + && (gimple_call_fndecl (stmt)
> + != builtin_decl_implicit (BUILT_IN_TSAN_INIT)))
> + return true;
> else if (is_gimple_assign (stmt))
> {
> if (gimple_store_p (stmt))
> - {
> - lhs = gimple_assign_lhs (stmt);
> - instrumented = instrument_expr (gsi, lhs, true);
> - }
> + {
> + lhs = gimple_assign_lhs (stmt);
> + instrumented = instrument_expr (gsi, lhs, true);
> + }
> if (gimple_assign_load_p (stmt))
> - {
> - rhs = gimple_assign_rhs1 (stmt);
> - instrumented = instrument_expr (gsi, rhs, false);
> - }
> + {
> + rhs = gimple_assign_rhs1 (stmt);
> + instrumented = instrument_expr (gsi, rhs, false);
> + }
> }
> return instrumented;
> }
> @@ -265,7 +249,7 @@ instrument_gimple (gimple_stmt_iterator
> /* Instruments all interesting memory accesses in the current function.
> * Return true if func entry/exit should be instrumented. */
>
> -static bool
> +static bool
> instrument_memory_accesses (void)
> {
> basic_block bb;
> @@ -291,15 +275,15 @@ instrument_func_entry (void)
> succ_bb = single_succ (ENTRY_BLOCK_PTR);
> gsi = gsi_after_labels (succ_bb);
>
> - builtin_decl = get_tsan_builtin_decl (BUILT_IN_RETURN_ADDRESS);
> + builtin_decl = builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS);
> g = gimple_build_call (builtin_decl, 1, integer_zero_node);
> - ret_addr = make_ssa_name (ptr_type_node, NULL);
> + ret_addr = make_ssa_name (ptr_type_node, NULL);
> gimple_call_set_lhs (g, ret_addr);
> gimple_set_location (g, cfun->function_start_locus);
> gsi_insert_before (&gsi, g, GSI_SAME_STMT);
>
> - builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_FUNC_ENTRY);
> - g = gimple_build_call (builtin_decl, 1, ret_addr);
> + builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_FUNC_ENTRY);
> + g = gimple_build_call (builtin_decl, 1, ret_addr);
> gimple_set_location (g, cfun->function_start_locus);
> gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> }
> @@ -325,7 +309,7 @@ instrument_func_exit (void)
> stmt = gsi_stmt (gsi);
> gcc_assert (gimple_code (stmt) == GIMPLE_RETURN);
> loc = gimple_location (stmt);
> - builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_FUNC_EXIT);
> + builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_FUNC_EXIT);
> g = gimple_build_call (builtin_decl, 0);
> gimple_set_location (g, loc);
> gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> @@ -350,70 +334,71 @@ tsan_pass (void)
> static bool
> tsan_gate (void)
> {
> - return flag_tsan != 0;
> + return flag_tsan != 0 && builtin_decl_implicit_p (BUILT_IN_TSAN_INIT);
> }
>
> /* Inserts __tsan_init () into the list of CTORs. */
>
> -void
> +void
> tsan_finish_file (void)
> {
> tree ctor_statements;
> tree init_decl;
>
> ctor_statements = NULL_TREE;
> - init_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_INIT);
> + init_decl = builtin_decl_implicit (BUILT_IN_TSAN_INIT);
> append_to_statement_list (build_call_expr (init_decl, 0),
> - &ctor_statements);
> + &ctor_statements);
> cgraph_build_static_cdtor ('I', ctor_statements,
> - MAX_RESERVED_INIT_PRIORITY - 1);
> + MAX_RESERVED_INIT_PRIORITY - 1);
> }
>
> /* The pass descriptor. */
>
> -struct gimple_opt_pass pass_tsan =
> +struct gimple_opt_pass pass_tsan =
> {
> {
> GIMPLE_PASS,
> - "tsan", /* name */
> - tsan_gate, /* gate */
> - tsan_pass, /* execute */
> - NULL, /* sub */
> - NULL, /* next */
> - 0, /* static_pass_number */
> - TV_NONE, /* tv_id */
> - PROP_ssa | PROP_cfg, /* properties_required */
> - 0, /* properties_provided */
> - 0, /* properties_destroyed */
> - 0, /* todo_flags_start */
> + "tsan", /* name */
> + OPTGROUP_NONE, /* optinfo_flags */
> + tsan_gate, /* gate */
> + tsan_pass, /* execute */
> + NULL, /* sub */
> + NULL, /* next */
> + 0, /* static_pass_number */
> + TV_NONE, /* tv_id */
> + PROP_ssa | PROP_cfg, /* properties_required */
> + 0, /* properties_provided */
> + 0, /* properties_destroyed */
> + 0, /* todo_flags_start */
> TODO_verify_all | TODO_update_ssa
> - | TODO_update_address_taken /* todo_flags_finish */
> + | TODO_update_address_taken /* todo_flags_finish */
> }
> };
>
> -static bool
> +static bool
> tsan_gate_O0 (void)
> -{
> - return flag_tsan != 0 && !optimize;
> -}
> +{
> + return flag_tsan != 0 && !optimize;
> +}
>
> -struct gimple_opt_pass pass_tsan_O0 =
> +struct gimple_opt_pass pass_tsan_O0 =
> {
> {
> GIMPLE_PASS,
> - "tsan0", /* name */
> - tsan_gate_O0, /* gate */
> - tsan_pass, /* execute */
> - NULL, /* sub */
> - NULL, /* next */
> - 0, /* static_pass_number */
> - TV_NONE, /* tv_id */
> - PROP_ssa | PROP_cfg, /* properties_required */
> - 0, /* properties_provided */
> - 0, /* properties_destroyed */
> - 0, /* todo_flags_start */
> + "tsan0", /* name */
> + OPTGROUP_NONE, /* optinfo_flags */
> + tsan_gate_O0, /* gate */
> + tsan_pass, /* execute */
> + NULL, /* sub */
> + NULL, /* next */
> + 0, /* static_pass_number */
> + TV_NONE, /* tv_id */
> + PROP_ssa | PROP_cfg, /* properties_required */
> + 0, /* properties_provided */
> + 0, /* properties_destroyed */
> + 0, /* todo_flags_start */
> TODO_verify_all | TODO_update_ssa
> - | TODO_update_address_taken /* todo_flags_finish */
> + | TODO_update_address_taken /* todo_flags_finish */
> }
> };
> -
>
>
> Jakub
