This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [tsan] ThreadSanitizer instrumentation part
For TODO_update_ssa, when we insert tsan_write(&a), current function's
ssa_renaming_needed flag will be set in finalize_ssa_defs because we
insert non-ssaname vdef. An assertion in execute_todo will check
whether we have TODO_update_ssa set when current function's
ssa_renaming_needed flag is set. That is why I will get assertion when
I remove TODO_update_ssa flag.
Is it ok to keep TODO_update_ssa and TODO_update_address_taken?
Thanks,
Wei.
On Mon, Nov 5, 2012 at 4:37 PM, Wei Mi <wmi@google.com> wrote:
> Hi Jakub,
>
> Thanks for the comments. I fix most of them except the setting of
> TODO_.... The new patch.txt is attached.
>
> Thanks,
> Wei.
>
>>> + 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.
>
>>> + | 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?
>
> On Sat, Nov 3, 2012 at 11:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Sat, Nov 03, 2012 at 10:05:35AM -0700, Wei Mi wrote:
>>> --- gcc/sanitizer.def (revision 0)
>>> +++ gcc/sanitizer.def (revision 0)
>>> @@ -0,0 +1,31 @@
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE_16, "__tsan_write16",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +
>>> +
>>> +
>>
>> Please remove the trailing whitespace.
>
> Done
>
>>
>>> +/* Builtin used by the implementation of libsanitizer. These
>>> + functions are mapped to the actual implementation of the
>>> + libasan and libtsan library. */
>>> +#undef DEF_SANITIZER_BUILTIN
>>> +#define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
>>> + DEF_BUILTIN (ENUM, "__builtin_" NAME, BUILT_IN_NORMAL, TYPE, TYPE, \
>>> + true, true, true, ATTRS, true, flag_tsan)
>>
>> That should be eventually flag_asan || flag_tsan, as sanitizer.def
>> should be also for asan builtins, or it must be DEF_TSAN_BUILTIN/tsan.def.
>>
>
> Postpone to fix it after asan checkin to trunk.
>
>>> +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;
>>
>> Formatting, : should be below ?.
>
> Fixed.
>
>>> +
>>> + return builtin_decl_implicit(fcode);
>>
>> Space before (. Several times in the code.
>>
>
> Fixed.
>
>> Also, as is the tsan builtins will be defined only for
>> C/C++ family FEs, so either something needs to be done
>> for other FEs, or perhaps the pass should just error out
>> if say the BUILT_IN_TSAN_INIT isn't defined.
>>
>
> Wrap builtin_decl_implicit in get_tsan_builtin_decl. If
> builtin_decl_implicit return invalid decl, output error message and
> then exit.
>
>>> +static tree
>>> +is_vptr_store (gimple stmt, tree expr, int is_write)
>>
>> is_write should be bool,
>>
>>> +{
>>> + if (is_write == 1
>>
>> and this just is_write
>>
>>> +static bool
>>> +is_load_of_const_p (tree expr, int is_write)
>>> +{
>>> + if (is_write)
>>> + return false;
>>
>> Again.
>>
>
> Fixed
>
>>> + /* The var does not live in memory -> no possibility of races. */
>>> + || (tcode == VAR_DECL
>>> + && !TREE_ADDRESSABLE (expr)
>>> + && TREE_STATIC (expr) == 0)
>>
>> Please use && !is_global_var (expr) here instead.
>>
>
> Changed.
>
>>> + /* TODO: handle other cases
>>> + (FIELD_DECL, MEM_REF, ARRAY_RANGE_REF, TARGET_MEM_REF, ADDR_EXPR). */
>>
>> The comment is obsolete, MEM_REF is handled.
>>
>
> Fixed.
>
>>> + if (tcode != ARRAY_REF
>>> + && tcode != VAR_DECL
>>> + && tcode != COMPONENT_REF
>>> + && tcode != INDIRECT_REF
>>> + && tcode != MEM_REF)
>>> + return false;
>>> +
>>> + stmt = gsi_stmt (gsi);
>>> + loc = gimple_location (stmt);
>>> + rhs = is_vptr_store (stmt, expr, is_write);
>>> +#ifdef DEBUG
>>> + if (rhs == NULL)
>>> + gcc_assert (is_gimple_addressable (expr));
>>> +#endif
>>
>> That should be
>> gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
>> if you want to check it in checking versions only.
>>
>
> Fixed.
>
>>> + size = int_size_in_bytes(expr_type);
>>
>> Missing space.
>>
>
> Fixed.
>
>>> + g = gimple_build_call(
>>> + get_memory_access_decl(is_write, size),
>>> + 1, expr_ptr);
>>
>> And the formatting here is completely wrong.
>>
>
> Fixed.
>
>>> + }
>>> + else
>>> + g = gimple_build_call(
>>> + builtin_decl_implicit(BUILT_IN_TSAN_VPTR_UPDATE),
>>> + 1, expr_ptr);
>>> + 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)
>>> + {
>>> + int flags = gimple_call_flags (stmt);
>>> + /* If the call can throw, it must be the last stmt in
>>> + * a basicblock, so the instrumented stmts need to be
>>> + * inserted on a successor edge. */
>>
>> 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.
>
>>> + stmt = gsi_stmt (gsi);
>>> + if (is_gimple_call (stmt) &&
>>> + (gimple_call_fndecl(stmt) !=
>>
>> Again, missing spaces, && and != belong on next lines.
>
> Fixed.
>
>>
>>> + if (gimple_assign_single_p (stmt))
>>
>> Not gimple_assign_load_p instead?
>
> Change to gimple_assign_load_p.
>
>>> +static bool
>>> +instrument_memory_accesses (void)
>>> +{
>>> + basic_block bb;
>>> + gimple_stmt_iterator gsi;
>>> + bool fentry_exit_instrument = false;
>>> +
>>> + FOR_EACH_BB (bb)
>>> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>> + fentry_exit_instrument = instrument_gimple (gsi) || fentry_exit_instrument;
>>
>> Line too long. Just do
>> fentry_exit_instrument |= instrument_gimple (gsi); ?
>>
>
> Fixed.
>
>>> + return fentry_exit_instrument;
>>> +}
>>> +
>>> +/* Instruments function entry. */
>>> +
>>> +static void
>>> +instrument_func_entry (void)
>>> +{
>>> + basic_block entry_bb;
>>> + edge entry_edge;
>>> + gimple_stmt_iterator gsi;
>>> + tree ret_addr;
>>> + gimple g;
>>> +
>>> + entry_bb = ENTRY_BLOCK_PTR;
>>> + entry_edge = single_succ_edge (entry_bb);
>>> + entry_bb = split_edge (entry_edge);
>>> + gsi = gsi_start_bb (entry_bb);
>>
>> Why? Just add the stmts to gsi_after_labels of
>> single_succ (ENTRY_BLOCK_PTR) ?
>>
>
> Fixed.
>
>>> +
>>> + g = gimple_build_call(
>>> + builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS),
>>> + 1, integer_zero_node);
>>
>> Wrong formatting.
>>
>
> Fixed.
>
>>> + ret_addr = create_tmp_var (ptr_type_node, "ret_addr");
>>
>> You don't need to create a decl for that, just
>> ret_addr = make_ssa_name (ptr_type_node, NULL);
>>
>
> Fixed.
>
>>> +static unsigned
>>> +tsan_pass (void)
>>> +{
>>> + struct gimplify_ctx gctx;
>>> +
>>> + push_gimplify_context (&gctx);
>>
>> Why?
>>
>
> Removed.
>
>>> + GIMPLE_PASS,
>>> + "tsan0", /* name */
>>> + tsan_gate_O0, /* gate */
>>> + tsan_pass, /* execute */
>>> + NULL, /* sub */
>>
>> The above is clearly badly formatted, /* execute */ comment
>> is not aligned with others. Please just use tabs instead
>> of spaces.
>>
>
> Fixed.
>
>>> + Copyright (C) 2011 Free Software Foundation, Inc.
>>
>> We have 2012 now, so 2011, 2012.
>>
>> Jakub
>
> Fixed.