Re: [tsan] ThreadSanitizer instrumentation part

[Wei Mi <wmi at google dot com>]

Sorry, I attached an incorrect patch.txt yesterday. This is the correct one.

Thanks,
Wei.

On Fri, Nov 2, 2012 at 6:31 PM, Wei Mi <wmi@google.com> wrote:
> 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