Re: [tsan] ThreadSanitizer instrumentation part

[Xinliang David Li <davidxl at google dot com>]

> --fno-default-inline @gol
> +-fmove-loop-invariants -fmudflap -fmudflapir -fmudflapth -fno-branch-count-reg @gol
> +-ftsan -ftsan-ignore -fno-default-inline @gol

Change -ftsan to -fthread-sanitizer

>  -fno-defer-pop -fno-function-cse -fno-guess-branch-probability @gol
>  -fno-inline -fno-math-errno -fno-peephole -fno-peephole2 @gol
>  -fno-sched-interblock -fno-sched-spec -fno-signed-zeros @gol
> @@ -5956,6 +5957,11 @@ appending @file{.dce} to the source file
>  Dump each function after adding mudflap instrumentation.  The file name is
>  made by appending @file{.mudflap} to the source file name.
>
> +
>  @item sra
>  @opindex fdump-tree-sra
>  Dump each function after performing scalar replacement of aggregates.  The
> @@ -6798,6 +6804,12 @@ instrumentation (and therefore faster ex
>  some protection against outright memory corrupting writes, but allows
>  erroneously read data to propagate within a program.
>
> +@item -ftsan -ftsan-ignore
> +@opindex ftsan
> +@opindex ftsan-ignore
> +Add ThreadSanitizer instrumentation. Use @option{-ftsan-ignore} to specify
> +an ignore file. Refer to http://go/tsan for details.
> +

-ftsan --> -fthread-sanitizer

> + *
> + * IT is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 3, or (at your option) any later
> + * version. See http://www.gnu.org/licenses/
> + /

You can copy this part of header from asan.c


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


s/functione/function/

> +
> +static tree
> +build_func_decl (tree typ, const char *name)
> +/* Builds the following decl
> +   void __tsan_read/writeX (void *addr);  */
> +
> +static tree
> +get_memory_access_decl (int is_write, unsigned size)

Change is_write to type bool.


> +{
> +  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.
> +
> +  is_write = !!is_write;


No need for this after making is_write bool.


> +  if (size <= 1)
> +    size = 1;
> +  else if (size <= 3)
> +    size = 2;
> +  else if (size <= 7)
> +    size = 4;


Missing function comment:

/* This function returns the function decl for __tsan_init.  */

> +static tree
> +get_init_decl (void)
> +{
> +  tree typ;
> +  return decl;
> +}
> +

> +/* Builds the following gimple sequence:
> +   __tsan_read/writeX (&EXPR);  */
> +
> +static gimple_seq
> +instr_memory_access (tree expr, int is_write)
> +{
> +  tree addr_expr, expr_type, call_expr, fdecl;
> +  gimple_seq gs;
> +  unsigned size;
> +
> +  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.


> +}
> +
> +/* Builds the following gimple sequence:
> +   __tsan_vptr_update (&EXPR, RHS);  */
> +
> +static gimple_seq
> +instr_vptr_update (tree expr, tree rhs)
> +{
> +  tree expr_ptr, call_expr, fdecl;
> +  gimple_seq gs;
> +
> +  expr_ptr = build_addr (unshare_expr (expr), current_function_decl);
> +  fdecl = get_vptr_update_decl ();
> +  call_expr = build_call_expr (fdecl, 2, expr_ptr, rhs);
> +  gs = NULL;
> +  force_gimple_operand (call_expr, &gs, true, 0);
> +  return gs;
> +}


Same as above -- directly use gimple_build_call interface.


> +
> +/* Returns gimple seq that needs to be inserted at function entry.  */
> +
> +static gimple_seq
> +instr_func_entry (void)
> +{
> +  tree retaddr_decl, pc_addr, fdecl, call_expr;
> +  gimple_seq gs;
> +
> +  retaddr_decl = builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS);
> +  pc_addr = build_call_expr (retaddr_decl, 1, integer_zero_node);
> +  fdecl = get_func_entry_decl ();
> +  call_expr = build_call_expr (fdecl, 1, pc_addr);
> +  gs = NULL;
> +  force_gimple_operand (call_expr, &gs, true, 0);
> +  return gs;
> +}


Same as above.


> +
> +/* Returns gimple seq that needs to be inserted before function exit.  */
> +
> +static gimple_seq
> +instr_func_exit (void)
> +{
> +  tree fdecl, call_expr;
> +  gimple_seq gs;
> +
> +  fdecl = get_func_exit_decl ();
> +  call_expr = build_call_expr (fdecl, 0);
> +  gs = NULL;
> +  force_gimple_operand (call_expr, &gs, true, 0);
> +  return gs;
> +}
> +


Same as above.


> +/* Sets location LOC for all gimples in the SEQ.  */
> +
> +static void
> +set_location (gimple_seq seq, location_t loc)
> +{
> +  gimple_seq_node n;
> +
> +  for (n = gimple_seq_first (seq); n != NULL; n = n->gsbase.next)
> +    gimple_set_location (n, loc);
> +}

Is there a need for this function? The location can be passed into the
creator functions.


> +
> +/* Check as to whether EXPR refers to a store to vptr.  */
> +
> +static tree
> +is_vptr_store (gimple stmt, tree expr, int is_write)
> +{
> +  if (is_write == 1
> +      && gimple_assign_single_p (stmt)
> +      && TREE_CODE (expr) == COMPONENT_REF)
> +    {
> +      tree field = TREE_OPERAND (expr, 1);
> +      if (TREE_CODE (field) == FIELD_DECL
> +          && DECL_VIRTUAL_P (field))
> +        return gimple_assign_rhs1 (stmt);
> +    }
> +  return NULL;
> +}
> +
> +/* Checks as to whether EXPR refers to constant var/field/param.
> +   Don't bother to instrument them.  */
> +
> +static int

change int --> bool

> +is_load_of_const (tree expr, int is_write)

Better name: is_load_of_const_p (...)

> +{
> +  if (is_write)
> +    return 0;

returns false;


> +  if (TREE_CODE (expr) == COMPONENT_REF)
> +    expr = TREE_OPERAND (expr, 1);
> +  if (TREE_CODE (expr) == VAR_DECL
> +      || TREE_CODE (expr) == PARM_DECL
> +      || TREE_CODE (expr) == FIELD_DECL)
> +    {
> +      if (TREE_READONLY (expr))
> +        return 1;

returns true;

> +    }
> +  return 0;

returns false;


> +      || TREE_CODE (base) == STRING_CST)
> +    return;
> +
> +  tcode = TREE_CODE (expr);
> +
> +  /* Below are things we do not instrument
> +     (no possibility of races or not implemented yet).  */
> +  if (/* Compiler-emitted artificial variables.  */
> +      (DECL_P (expr) && DECL_ARTIFICIAL (expr))
> +      /* The var does not live in memory -> no possibility of races.  */
> +      || (tcode == VAR_DECL
> +          && TREE_ADDRESSABLE (expr) == 0

 ==> !TREE_ADDRESSABLE (expr)

> +         && TREE_STATIC (expr) == 0)


To detect stack varaible, TREE_STATIC can not be used.  Use
!DECL_EXTERNAL instead.


> +      /* Not implemented.  */
> +      || TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE

> +    return;
> +
> +  func_mops++;
> +  stmt = gsi_stmt (gsi);
> +  loc = gimple_location (stmt);
> +  rhs = is_vptr_store (stmt, expr, is_write);
> +  if (rhs == NULL)
> +    gs = instr_memory_access (expr, is_write);
> +  else
> +    gs = instr_vptr_update (expr, rhs);

Pass the location to the instrumetors.

> +  set_location (gs, loc);

Remove this.

> +  /* 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);
> +  else
> +    gsi_insert_seq_before (&gsi, gs, GSI_SAME_STMT);
> +}
> +

> +}
> +
> +
> +void tsan_finish_file (void)

function name starts a new line.

void
tsan_finish_file

> +{
> +  tree ctor_statements;
> +
> +  ctor_statements = NULL_TREE;
> +  append_to_statement_list (build_call_expr (get_init_decl (), 0),
> +                            &ctor_statements);
> +  cgraph_build_static_cdtor ('I', ctor_statements,
> +                             MAX_RESERVED_INIT_PRIORITY - 1);
> +}
> +
> +/* The pass descriptor.  */


David


On Wed, Oct 31, 2012 at 11:34 AM, Wei Mi <wmi@google.com> wrote:
> Hi,
>
> The patch is about ThreadSanitizer. ThreadSanitizer is a data race
> detector for C/C++ programs. It contains two parts: instrumentation
> and runtime library. This patch is the first part, and runtime will be
> included in the second part. Dmitry(dvyukov@google.com) is the author
> of this part, and I try to migrate it to trunk. Ok for trunk?
>
> gcc/ChangeLog:
> 2012-10-31  Wei Mi  <wmi@gmail.com>
>
>         * Makefile.in (tsan.o): 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
>         -ftsan is on.
>         * tsan.c: New file about tsan
>         * tsan.h: Ditto
>
> Please check the following links for background:
> http://code.google.com/p/data-race-test
> http://gcc.gnu.org/wiki/cauldron2012?action=AttachFile&do=get&target=kcc.pdf
> (the second half is about ThreadSanitizer).
>
> A small testcase race_on_heap.cc is attached to show its
> functionality. Run the small testcase with -ftsan produce the
> following warning:
>
> WARNING: ThreadSanitizer: data race (pid=5978)
>   Write of size 4 at 0x7d0600039040 by thread 3:
>     #0 Thread2(void*) ??:0 (exe+0x0000000052c0)
>
>   Previous write of size 4 at 0x7d0600039040 by thread 2:
>     #0 Thread1(void*) ??:0 (exe+0x00000000527d)
>
>   Location is heap block of size 99 at 0x7d0600039030 allocated by thread 1:
>     #0 malloc /usr/local/google/home/wmi/Work/llvm-main/trunk/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:293
> (exe+0x00000000e9ce)
>     #1 alloc() ??:0 (exe+0x0000000052fc)
>     #2 AllocThread(void*) ??:0 (exe+0x00000000532c)
>
>   Thread 3 (tid=5981, running) created at:
>     #0 pthread_create
> /usr/local/google/home/wmi/Work/llvm-main/trunk/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:645
> (exe+0x00000000bf1d)
>     #1 main ??:0 (exe+0x000000005433)
>
>   Thread 2 (tid=5980, finished) created at:
>     #0 pthread_create
> /usr/local/google/home/wmi/Work/llvm-main/trunk/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:645
> (exe+0x00000000bf1d)
>     #1 main ??:0 (exe+0x000000005400)
>
>   Thread 1 (tid=5979, finished) created at:
>     #0 pthread_create
> /usr/local/google/home/wmi/Work/llvm-main/trunk/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:645
> (exe+0x00000000bf1d)
>     #1 main ??:0 (exe+0x000000005384)
>
> Thanks,
> Wei.