Re: [tsan] ThreadSanitizer instrumentation part

[Wei Mi <wmi at google dot com>]

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