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 <w...@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 <ja...@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.