Re: [tsan] ThreadSanitizer instrumentation part

[Wei Mi <wmi at google dot com>]

Hi,

Thanks for so many useful comments! I update the file according to the
comments. The major changes include adding sanitizer.def and
generating gimple directly. New patch file is attached.

> On Wed, Oct 31, 2012 at 11:34:10AM -0700, Wei Mi wrote:
>> gcc/ChangeLog:
>> 2012-10-31  Wei Mi  <wmi@gmail.com>
>
> If Dmitry wrote parts of the patch, it would be nice to mention
> him in the ChangeLog too.

> All ChangeLog entries should end with a dot.

Changed.

2012-10-31  Dmitry Vyukov  <dvyukov@google.com>
                     Wei Mi  <wmi@google.com>

        * Makefile.in (tsan.o): New.
        (BUILTINS_DEF): Add sanitizer.def.
        * sanitizer.def: New.
        * passes.c (init_optimization_passes): Add tsan passes.
        * tree-pass.h (register_pass_info): Ditto.
        * cfghooks.h (GCC_CFGHOOKS_H): Avoid including duplicate headers.
        * doc/invoke.texi: Document tsan related options.
        * toplev.c (compile_file): Add tsan pass in driver.
        * gcc.c (LINK_COMMAND_SPEC): Add -lasan in link command if there
        -fthread_sanitizer is on.
        * tsan.c: New file about tsan.
        * tsan.h: Ditto.


>>  struct cfg_hooks
>> @@ -219,3 +222,4 @@ extern void gimple_register_cfg_hooks (v
>>  extern struct cfg_hooks get_cfg_hooks (void);
>>  extern void set_cfg_hooks (struct cfg_hooks);
>>
>> +#endif  /* GCC_CFGHOOKS_H */
>
> Why this?  Simply don't include that header in tsan.c, it is already
> included by basic-block.h.

Remove cfghooks.h from tsan.c. Remove the #ifdef GCC_CFGHOOKS_H from cfghooks.h

> Can't google just assign the code to FSF, and use a standard boilerplate
> as everything else in gcc/ ?

Copy from asan header and make some change.

>> +static tree
>> +get_vptr_update_decl (void)
>> +{
>> +  tree typ;
>> +  static tree decl;
>> +
>> +  if (decl != NULL)
>> +    return decl;
>> +  typ = build_function_type_list (void_type_node,
>> +                                  ptr_type_node, ptr_type_node, NULL_TREE);
>> +  decl = build_func_decl (typ, "__tsan_vptr_update");
>> +  return decl;
>> +}
> ...
>
> Instead of this (but same applies to asan), I think we should just consider
> putting it into builtins.def (or have sanitizer.def like there is sync.def
> or omp-builtins.def).  The problem might be non-C/C++ family frontends
> though.

Create sanitizer.def and use builtin_decl_implicit to create builtin decls.

>> +  while (TREE_CODE (expr_type) == ARRAY_TYPE)
>> +    expr_type = TREE_TYPE (expr_type);
>> +  size = (TREE_INT_CST_LOW (TYPE_SIZE (expr_type))) / BITS_PER_UNIT;
>
> int_size_in_bytes.

Changed.

> preferrably without building everything as trees, then gimplifying it.

Generate gimple directly. Remove funcs: instr_memory_access,
instr_vptr_update, instr_func_entry, instr_func_exit

> For func_calls and func_mops, I believe why you need two variables instead
> of just one, and why the function can't just return a bool whether
> entry/exit needs to be instrumented or not.

instrument_memory_accesses return a bool indicating whether or not
entry/exit needs to be instrumented. func_calls and func_mops removed.

>> +set_location (gimple_seq seq, location_t loc)
>> +{
>> +  gimple_seq_node n;
>> +
>> +  for (n = gimple_seq_first (seq); n != NULL; n = n->gsbase.next)
>
> This really should use a stmt iterator.

set_location removed. set gimple location using gimple_set_location
everytime a new gimple statement is inserted.

>> +  FOR_EACH_BB (bb)
>> +    {
>> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> +        {
>> +          instrument_gimple (gsi);
>> +        }
>> +    }
>
> Extraneous two pairs of {}s.

Fixed.

>> +struct gimple_opt_pass pass_tsan = {{
>
> Please watch formatting of other gimple_opt_pass structures.
> {{ isn't used anywhere.

Fixed.

> Is that the option that LLVM uses (I'm talking about -faddress-sanitizer
> in LLVM vs. -fasan right now in GCC, isn't that similar?).

Fixed.

> +static tree
> +get_init_decl (void)
> +{
> +  tree typ;
> +  static tree decl;
> +
> +  if (decl != NULL)
> +    return decl;
> +  typ = build_function_type_list (void_type_node, NULL_TREE);
> +  decl = build_func_decl (typ, "__tsan_init");
> +  return decl;
> +}
>
> The above can crash the compiler btw, as that static tree decl
> (in many other functions) is not GTY(()) marked (must be file scope for
> that), thus ggc_collect might free it.  Also, please use type
> instead of typ for variable names.

Func get_init_decl removed after generating gimple directly.

>> +  /* 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)
>> +    gsi_insert_seq_after (&gsi, gs, GSI_NEW_STMT);
>
> Inserting stmts after a call may or may not work.  E.g. if the call
> can throw, it must be the last stmt in a basic block, so then the
> stmts need to be inserted on a successor edge.  Similarly noreturn
> call must be last (but in that case it shouldn't have lhs).

Add processing for call which can throw.
  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. */
      if (!(flags & ECF_NOTHROW))
        {
          bb = gsi_bb(gsi);
          succ_edge = single_succ_edge (bb);
          succ_bb = split_edge (succ_edge);
          gsi = gsi_start_bb (succ_bb);
        }
      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
    }
  else
    gsi_insert_before (&gsi, g, GSI_SAME_STMT);

>> +  gcode = gimple_code (stmt);
>> +  if (gcode == GIMPLE_CALL)
>
> is_gimple_call (stmt)

Fixed.

>> +  else if (gcode == GIMPLE_ASSIGN)
>
> is_gimple_assign (stmt)

Fixed.

>> +    {
>> +      /* Handle assignment lhs as store.  */
>> +      lhs = gimple_assign_lhs (stmt);
>> +      instrument_expr (gsi, lhs, 1);
>
> To find what a store or load is, you can just use the new
> gimple_store_p (stmt) and gimple_assign_load_p (stmt)
> predicates, or at least just do gimple_assign_single_p (stmt)
> to guard instrument_expr calls on both lhs and rhs1.
> No need to scan all operands, only single rhs assignments
> can be loads.

Fixed:
  else if (is_gimple_assign (stmt))
    {
      if (gimple_store_p (stmt))
        {
          lhs = gimple_assign_lhs (stmt);
          instrumented = instrument_expr (gsi, lhs, true);
        }
      if (gimple_assign_single_p (stmt))
        {
          rhs = gimple_assign_rhs1 (stmt);
          instrumented = instrument_expr (gsi, rhs, false);
        }
    }

>> +static int func_calls;
>> +
>> +/* Returns a definition of a runtime functione with type TYP and name NAME.  */
>
>
> s/functione/function/

Fixed.

>> +static tree
>> +get_memory_access_decl (int is_write, unsigned size)
>
> Change is_write to type bool.

Fixed.

>> +  tree typ, *decl;
>> +  char fname [64];
>> +  static tree cache [2][17];
>
> There is no need to make this function local. define a macro for value 17.

The cache is removed after we use builtin_decl_implicit to create builtin decls.

>> +  is_write = !!is_write;
>
>
> No need for this after making is_write bool.

Fixed.

> Missing function comment:
>
> /* This function returns the function decl for __tsan_init.  */
>
>> +static tree
>> +get_init_decl (void)
>> +{
>> +  tree typ;
>> +  return decl;
>> +}
>> +

The func is removed.

>> +  gcc_assert (is_gimple_addressable (expr));
>> +  addr_expr = build_addr (unshare_expr (expr), current_function_decl);
>> +  expr_type = TREE_TYPE (expr);
>> +  while (TREE_CODE (expr_type) == ARRAY_TYPE)
>> +    expr_type = TREE_TYPE (expr_type);
>> +  size = (TREE_INT_CST_LOW (TYPE_SIZE (expr_type))) / BITS_PER_UNIT;
>> +  fdecl = get_memory_access_decl (is_write, size);
>> +  call_expr = build_call_expr (fdecl, 1, addr_expr);
>> +  gs = NULL;
>> +  force_gimple_operand (call_expr, &gs, true, 0);
>> +  return gs;
>
>
> Use gimple creator API: gimple_build_call --> see examples in tree-profile.c.
>
> Return the gimple_stmt_iterator for the newly created call stmt.

Fixed.

>> +        return 1;
>
> returns true;
>
>> +    }
>> +  return 0;
>
> returns false;

Fixed.

>> +is_load_of_const (tree expr, int is_write)
>
> Better name: is_load_of_const_p (...)

Fixed.

>> +void tsan_finish_file (void)
>
> function name starts a new line.

Fixed.

Thanks,
Wei.

Attachment: patch.txt
Description: Text document