This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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.
Attachment:
patch.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |