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.
> +/* 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.
> +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 ?.
> +
> + return builtin_decl_implicit(fcode);
Space before (. Several times in the code.
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.
> +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.
> + /* 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.
> + /* TODO: handle other cases
> + (FIELD_DECL, MEM_REF, ARRAY_RANGE_REF, TARGET_MEM_REF, ADDR_EXPR). */
The comment is obsolete, MEM_REF is handled.
> + 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.
> + size = int_size_in_bytes(expr_type);
Missing space.
> + g = gimple_build_call(
> + get_memory_access_decl(is_write, size),
> + 1, expr_ptr);
And the formatting here is completely wrong.
> + }
> + 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.
> + stmt = gsi_stmt (gsi);
> + if (is_gimple_call (stmt) &&
> + (gimple_call_fndecl(stmt) !=
Again, missing spaces, && and != belong on next lines.
> + if (gimple_assign_single_p (stmt))
Not gimple_assign_load_p instead?
> +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); ?
> + 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) ?
> +
> + g = gimple_build_call(
> + builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS),
> + 1, integer_zero_node);
Wrong formatting.
> + 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);
> +static unsigned
> +tsan_pass (void)
> +{
> + struct gimplify_ctx gctx;
> +
> + push_gimplify_context (&gctx);
Why?
> + 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.
> + TODO_verify_all | TODO_update_ssa
Ideally you shouldn't need TODO_update_ssa.
> + | TODO_update_address_taken /* todo_flags_finish */
And why this?
> + Copyright (C) 2011 Free Software Foundation, Inc.
We have 2012 now, so 2011, 2012.
Jakub
