[tsan] ThreadSanitizer instrumentation part

On Mon, Nov 05, 2012 at 04:37:41PM -0800, Wei Mi wrote:
> Thanks for the comments. I fix most of them except the setting of
> TODO_.... The new patch.txt is attached.

Please update the patch against trunk, it doesn't apply cleanly because
of the asan commit.  I took the liberty to do at least some formatting
cleanups and other small tweaks against your patch to tsan.c.

> >> +  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.

If it were just for the instrumentations, then you can easily update
the vdef/vuse yourself, e.g. when inserting before a memory write,
you can copy over the gimple_vuse of that write to gimple_vuse of the
instrumentation call, create a new SSA_NAME for the gimple_vdef and
and set the write's gimple_vuse to that new SSA_NAME.  For call
instrumentation it would be a tiny bit harder, but for the instrumentation
of function entry/exit you'd need to find out the current vop at that point.
So perhaps we can live with that, at least for now.

> >> +    | 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?

That is complete misunderstanding of what update_address_taken does.
It removes TREE_ADDRESSABLE from addressables that are no longer
addressable, rather than adding TREE_ADDRESSABLE bits.  For the latter
there is mark_addressable function.
> 
> Wrap builtin_decl_implicit in get_tsan_builtin_decl. If
> builtin_decl_implicit return invalid decl, output error message and
> then exit.

I've moved that over just to the gate, eventually there should be a routine
that will initialize the builtins if they aren't by the FE.

> > 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.

But wrongly, for one adding the same stmt to multiple basic blocks
is going to crash terribly, but also you IMHO want to insert just
on fallthru edge, if the routine throws or longjmps, the result isn't
written.



	Jakub

On Tue, Nov 13, 2012 at 8:40 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Nov 05, 2012 at 04:37:41PM -0800, Wei Mi wrote:
>> Thanks for the comments. I fix most of them except the setting of
>> TODO_.... The new patch.txt is attached.
>
> Please update the patch against trunk, it doesn't apply cleanly because
> of the asan commit.  I took the liberty to do at least some formatting
> cleanups and other small tweaks against your patch to tsan.c.
>
>> >> +  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.
>
> If it were just for the instrumentations, then you can easily update
> the vdef/vuse yourself, e.g. when inserting before a memory write,
> you can copy over the gimple_vuse of that write to gimple_vuse of the
> instrumentation call, create a new SSA_NAME for the gimple_vdef and
> and set the write's gimple_vuse to that new SSA_NAME.  For call
> instrumentation it would be a tiny bit harder, but for the instrumentation
> of function entry/exit you'd need to find out the current vop at that point.
> So perhaps we can live with that, at least for now.
>
>> >> +    | 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?
>
> That is complete misunderstanding of what update_address_taken does.
> It removes TREE_ADDRESSABLE from addressables that are no longer
> addressable, rather than adding TREE_ADDRESSABLE bits.

It will do the latter too. See iv-opts.

>  For the latter
> there is mark_addressable function.

This is certainly cheaper to use.

David



>>
>> Wrap builtin_decl_implicit in get_tsan_builtin_decl. If
>> builtin_decl_implicit return invalid decl, output error message and
>> then exit.
>
> I've moved that over just to the gate, eventually there should be a routine
> that will initialize the builtins if they aren't by the FE.
>
>> > 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.
>
> But wrongly, for one adding the same stmt to multiple basic blocks
> is going to crash terribly, but also you IMHO want to insert just
> on fallthru edge, if the routine throws or longjmps, the result isn't
> written.
>
> --- gcc/tsan.c.jj       2012-11-13 16:46:21.000000000 +0100
> +++ gcc/tsan.c  2012-11-13 17:22:56.054837754 +0100
> @@ -1,4 +1,4 @@
> -/* GCC instrumentation plugin for ThreadSanitizer.
> +/* GCC instrumentation plugin for ThreadSanitizer.
>     Copyright (C) 2011, 2012 Free Software Foundation, Inc.
>     Contributed by Dmitry Vyukov <dvyukov@google.com>
>
> @@ -44,36 +44,27 @@ along with GCC; see the file COPYING3.
>     void __tsan_read/writeX (void *addr);  */
>
>  static tree
> -get_tsan_builtin_decl (enum built_in_function fcode)
> -{
> -  tree decl = builtin_decl_implicit (fcode);
> -  if (decl == NULL_TREE)
> -    internal_error ("undefined builtin %s", built_in_names[fcode]);
> -  return decl;
> -}
> -
> -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;
> +                    : BUILT_IN_TSAN_READ_1;
>    else if (size <= 3)
> -    fcode = is_write ? BUILT_IN_TSAN_WRITE_2
> -                     : BUILT_IN_TSAN_READ_2;
> +    fcode = is_write ? BUILT_IN_TSAN_WRITE_2
> +                    : BUILT_IN_TSAN_READ_2;
>    else if (size <= 7)
> -    fcode = is_write ? BUILT_IN_TSAN_WRITE_4
> -                     : BUILT_IN_TSAN_READ_4;
> +    fcode = is_write ? BUILT_IN_TSAN_WRITE_4
> +                    : BUILT_IN_TSAN_READ_4;
>    else if (size <= 15)
> -    fcode = is_write ? BUILT_IN_TSAN_WRITE_8
> -                     : BUILT_IN_TSAN_READ_8;
> +    fcode = is_write ? BUILT_IN_TSAN_WRITE_8
> +                    : BUILT_IN_TSAN_READ_8;
>    else
> -    fcode = is_write ? BUILT_IN_TSAN_WRITE_16
> -                     : BUILT_IN_TSAN_READ_16;
> +    fcode = is_write ? BUILT_IN_TSAN_WRITE_16
> +                    : BUILT_IN_TSAN_READ_16;
>
> -  return get_tsan_builtin_decl (fcode);
> +  return builtin_decl_implicit (fcode);
>  }
>
>  /* Check as to whether EXPR refers to a store to vptr.  */
> @@ -81,14 +72,14 @@ get_memory_access_decl (bool is_write, u
>  static tree
>  is_vptr_store (gimple stmt, tree expr, bool is_write)
>  {
> -  if (is_write == true
> +  if (is_write == true
>        && 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);
> +         && DECL_VIRTUAL_P (field))
> +       return gimple_assign_rhs1 (stmt);
>      }
>    return NULL;
>  }
> @@ -96,7 +87,7 @@ is_vptr_store (gimple stmt, tree expr, b
>  /* Checks as to whether EXPR refers to constant var/field/param.
>     Don't bother to instrument them.  */
>
> -static bool
> +static bool
>  is_load_of_const_p (tree expr, bool is_write)
>  {
>    if (is_write)
> @@ -108,7 +99,7 @@ is_load_of_const_p (tree expr, bool is_w
>        || TREE_CODE (expr) == FIELD_DECL)
>      {
>        if (TREE_READONLY (expr))
> -        return true;
> +       return true;
>      }
>    return false;
>  }
> @@ -116,18 +107,14 @@ is_load_of_const_p (tree expr, bool is_w
>  /* Instruments EXPR if needed. If any instrumentation is inserted,
>   * return true. */
>
> -static bool
> +static bool
>  instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write)
>  {
>    enum tree_code tcode;
> -  unsigned fld_off, fld_size;
>    tree base, rhs, expr_type, expr_ptr, builtin_decl;
> -  basic_block bb, succ_bb;
> -  edge_iterator ei;
> -  edge e;
> +  basic_block bb;
>    HOST_WIDE_INT size;
>    gimple stmt, g;
> -  gimple_stmt_iterator start_gsi;
>    location_t loc;
>
>    base = get_base_address (expr);
> @@ -144,8 +131,8 @@ instrument_expr (gimple_stmt_iterator gs
>        (DECL_P (expr) && DECL_ARTIFICIAL (expr))
>        /* The var does not live in memory -> no possibility of races.  */
>        || (tcode == VAR_DECL
> -          && !TREE_ADDRESSABLE (expr)
> -          && TREE_STATIC (expr) == 0)
> +         && !TREE_ADDRESSABLE (expr)
> +         && TREE_STATIC (expr) == 0)
>        /* Not implemented.  */
>        || TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE
>        /* Not implemented.  */
> @@ -156,22 +143,21 @@ instrument_expr (gimple_stmt_iterator gs
>        || is_load_of_const_p (expr, is_write))
>      return false;
>
> -  if (tcode == COMPONENT_REF)
> -    {
> -      tree field = TREE_OPERAND (expr, 1);
> -      if (TREE_CODE (field) == FIELD_DECL)
> -        {
> -          fld_off = TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field));
> -          fld_size = TREE_INT_CST_LOW (DECL_SIZE (field));
> -          if (((fld_off % BITS_PER_UNIT) != 0)
> -              || ((fld_size % BITS_PER_UNIT) != 0))
> -            {
> -              /* As of now it crashes compilation.
> -                 TODO: handle bit-fields as if touching the whole field.  */
> -              return false;
> -            }
> -        }
> -    }
> +  size = int_size_in_bytes (TREE_TYPE (expr));
> +  if (size == -1)
> +    return false;
> +
> +  /* For now just avoid instrumenting bit field acceses.
> +     TODO: handle bit-fields as if touching the whole field.  */
> +  HOST_WIDE_INT bitsize, bitpos;
> +  tree offset;
> +  enum machine_mode mode;
> +  int volatilep = 0, unsignedp = 0;
> +  get_inner_reference (expr, &bitsize, &bitpos, &offset,
> +                      &mode, &unsignedp, &volatilep, false);
> +  if (bitpos % (size * BITS_PER_UNIT)
> +      || bitsize != size * BITS_PER_UNIT)
> +    return false;
>
>    /* TODO: handle other cases
>       (FIELD_DECL, ARRAY_RANGE_REF, TARGET_MEM_REF, ADDR_EXPR).  */
> @@ -186,44 +172,42 @@ instrument_expr (gimple_stmt_iterator gs
>    loc = gimple_location (stmt);
>    rhs = is_vptr_store (stmt, expr, is_write);
>    gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
> -  expr_ptr = build_addr (unshare_expr (expr),
> -                         current_function_decl);
> +  expr_ptr = build_fold_addr_expr (unshare_expr (expr));
>    if (rhs == NULL)
>      {
>        expr_type = TREE_TYPE (expr);
>        while (TREE_CODE (expr_type) == ARRAY_TYPE)
> -        expr_type = TREE_TYPE (expr_type);
> -      size = int_size_in_bytes (expr_type);
> +       expr_type = TREE_TYPE (expr_type);
> +      size = int_size_in_bytes (expr_type);
>        g = gimple_build_call (get_memory_access_decl (is_write, size),
> -                             1, expr_ptr);
> +                            1, expr_ptr);
>      }
>    else
>      {
> -      builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_VPTR_UPDATE);
> +      builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
>        g = gimple_build_call (builtin_decl, 1, expr_ptr);
>      }
> -  gimple_set_location (g, loc);
> +  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)
> +  if (is_gimple_call (stmt) && is_write)
>      {
>        /* If the call can throw, it must be the last stmt in
> -         a basicblock, so the instrumented stmts need to be
> -         inserted in successor bbs. */
> -      if (is_ctrl_altering_stmt (stmt))
> -        {
> -          bb = gsi_bb (gsi);
> -          FOR_EACH_EDGE (e, ei, bb->succs)
> -            {
> -              succ_bb = split_edge (e);
> -              start_gsi = gsi_start_bb (succ_bb);
> -              gsi_insert_after (&start_gsi, g, GSI_NEW_STMT);
> -            }
> -        }
> +        a basic block, so the instrumented stmts need to be
> +        inserted in successor bbs. */
> +      if (is_ctrl_altering_stmt (stmt))
> +       {
> +         edge e;
> +
> +         bb = gsi_bb (gsi);
> +         e = find_fallthru_edge (bb->succs);
> +         if (e)
> +           gsi_insert_seq_on_edge_immediate (e, g);
> +       }
>        else
> -        gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +       gsi_insert_after (&gsi, g, GSI_NEW_STMT);
>      }
>    else
>      gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> @@ -242,22 +226,22 @@ instrument_gimple (gimple_stmt_iterator
>    bool instrumented = false;
>
>    stmt = gsi_stmt (gsi);
> -  if (is_gimple_call (stmt)
> -      && (gimple_call_fndecl (stmt)
> -          != get_tsan_builtin_decl (BUILT_IN_TSAN_INIT)))
> -    return true;
> +  if (is_gimple_call (stmt)
> +      && (gimple_call_fndecl (stmt)
> +         != builtin_decl_implicit (BUILT_IN_TSAN_INIT)))
> +    return true;
>    else if (is_gimple_assign (stmt))
>      {
>        if (gimple_store_p (stmt))
> -        {
> -          lhs = gimple_assign_lhs (stmt);
> -          instrumented = instrument_expr (gsi, lhs, true);
> -        }
> +       {
> +         lhs = gimple_assign_lhs (stmt);
> +         instrumented = instrument_expr (gsi, lhs, true);
> +       }
>        if (gimple_assign_load_p (stmt))
> -        {
> -          rhs = gimple_assign_rhs1 (stmt);
> -          instrumented = instrument_expr (gsi, rhs, false);
> -        }
> +       {
> +         rhs = gimple_assign_rhs1 (stmt);
> +         instrumented = instrument_expr (gsi, rhs, false);
> +       }
>      }
>    return instrumented;
>  }
> @@ -265,7 +249,7 @@ instrument_gimple (gimple_stmt_iterator
>  /* Instruments all interesting memory accesses in the current function.
>   * Return true if func entry/exit should be instrumented. */
>
> -static bool
> +static bool
>  instrument_memory_accesses (void)
>  {
>    basic_block bb;
> @@ -291,15 +275,15 @@ instrument_func_entry (void)
>    succ_bb = single_succ (ENTRY_BLOCK_PTR);
>    gsi = gsi_after_labels (succ_bb);
>
> -  builtin_decl = get_tsan_builtin_decl (BUILT_IN_RETURN_ADDRESS);
> +  builtin_decl = builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS);
>    g = gimple_build_call (builtin_decl, 1, integer_zero_node);
> -  ret_addr = make_ssa_name (ptr_type_node, NULL);
> +  ret_addr = make_ssa_name (ptr_type_node, NULL);
>    gimple_call_set_lhs (g, ret_addr);
>    gimple_set_location (g, cfun->function_start_locus);
>    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
>
> -  builtin_decl =  get_tsan_builtin_decl (BUILT_IN_TSAN_FUNC_ENTRY);
> -  g = gimple_build_call (builtin_decl, 1, ret_addr);
> +  builtin_decl =  builtin_decl_implicit (BUILT_IN_TSAN_FUNC_ENTRY);
> +  g = gimple_build_call (builtin_decl, 1, ret_addr);
>    gimple_set_location (g, cfun->function_start_locus);
>    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
>  }
> @@ -325,7 +309,7 @@ instrument_func_exit (void)
>        stmt = gsi_stmt (gsi);
>        gcc_assert (gimple_code (stmt) == GIMPLE_RETURN);
>        loc = gimple_location (stmt);
> -      builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_FUNC_EXIT);
> +      builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_FUNC_EXIT);
>        g = gimple_build_call (builtin_decl, 0);
>        gimple_set_location (g, loc);
>        gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> @@ -350,70 +334,71 @@ tsan_pass (void)
>  static bool
>  tsan_gate (void)
>  {
> -  return flag_tsan != 0;
> +  return flag_tsan != 0 && builtin_decl_implicit_p (BUILT_IN_TSAN_INIT);
>  }
>
>  /* Inserts __tsan_init () into the list of CTORs.  */
>
> -void
> +void
>  tsan_finish_file (void)
>  {
>    tree ctor_statements;
>    tree init_decl;
>
>    ctor_statements = NULL_TREE;
> -  init_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_INIT);
> +  init_decl = builtin_decl_implicit (BUILT_IN_TSAN_INIT);
>    append_to_statement_list (build_call_expr (init_decl, 0),
> -                            &ctor_statements);
> +                           &ctor_statements);
>    cgraph_build_static_cdtor ('I', ctor_statements,
> -                             MAX_RESERVED_INIT_PRIORITY - 1);
> +                            MAX_RESERVED_INIT_PRIORITY - 1);
>  }
>
>  /* The pass descriptor.  */
>
> -struct gimple_opt_pass pass_tsan =
> +struct gimple_opt_pass pass_tsan =
>  {
>   {
>    GIMPLE_PASS,
> -  "tsan",                               /* name  */
> -  tsan_gate,                            /* gate  */
> -  tsan_pass,                            /* execute  */
> -  NULL,                                 /* sub  */
> -  NULL,                                 /* next  */
> -  0,                                    /* static_pass_number  */
> -  TV_NONE,                              /* tv_id  */
> -  PROP_ssa | PROP_cfg,                  /* properties_required  */
> -  0,                                    /* properties_provided  */
> -  0,                                    /* properties_destroyed  */
> -  0,                                    /* todo_flags_start  */
> +  "tsan",                              /* name  */
> +  OPTGROUP_NONE,                       /* optinfo_flags */
> +  tsan_gate,                           /* gate  */
> +  tsan_pass,                           /* execute  */
> +  NULL,                                        /* sub  */
> +  NULL,                                        /* next  */
> +  0,                                   /* static_pass_number  */
> +  TV_NONE,                             /* tv_id  */
> +  PROP_ssa | PROP_cfg,                 /* properties_required  */
> +  0,                                   /* properties_provided  */
> +  0,                                   /* properties_destroyed  */
> +  0,                                   /* todo_flags_start  */
>    TODO_verify_all | TODO_update_ssa
> -  | TODO_update_address_taken           /* todo_flags_finish  */
> +  | TODO_update_address_taken          /* todo_flags_finish  */
>   }
>  };
>
> -static bool
> +static bool
>  tsan_gate_O0 (void)
> -{
> -  return flag_tsan != 0 && !optimize;
> -}
> +{
> +  return flag_tsan != 0 && !optimize;
> +}
>
> -struct gimple_opt_pass pass_tsan_O0 =
> +struct gimple_opt_pass pass_tsan_O0 =
>  {
>   {
>    GIMPLE_PASS,
> -  "tsan0",                              /* name  */
> -  tsan_gate_O0,                         /* gate  */
> -  tsan_pass,                            /* execute  */
> -  NULL,                                 /* sub  */
> -  NULL,                                 /* next  */
> -  0,                                    /* static_pass_number  */
> -  TV_NONE,                              /* tv_id  */
> -  PROP_ssa | PROP_cfg,                  /* properties_required  */
> -  0,                                    /* properties_provided  */
> -  0,                                    /* properties_destroyed  */
> -  0,                                    /* todo_flags_start  */
> +  "tsan0",                             /* name  */
> +  OPTGROUP_NONE,                       /* optinfo_flags */
> +  tsan_gate_O0,                                /* gate  */
> +  tsan_pass,                           /* execute  */
> +  NULL,                                        /* sub  */
> +  NULL,                                        /* next  */
> +  0,                                   /* static_pass_number  */
> +  TV_NONE,                             /* tv_id  */
> +  PROP_ssa | PROP_cfg,                 /* properties_required  */
> +  0,                                   /* properties_provided  */
> +  0,                                   /* properties_destroyed  */
> +  0,                                   /* todo_flags_start  */
>    TODO_verify_all | TODO_update_ssa
> -  | TODO_update_address_taken           /* todo_flags_finish  */
> +  | TODO_update_address_taken          /* todo_flags_finish  */
>   }
>  };
> -
>
>
>         Jakub

On Tue, Nov 13, 2012 at 09:25:36AM -0800, Xinliang David Li wrote:
> > That is complete misunderstanding of what update_address_taken does.
> > It removes TREE_ADDRESSABLE from addressables that are no longer
> > addressable, rather than adding TREE_ADDRESSABLE bits.
> 
> It will do the latter too. See iv-opts.

Where?  I strongly doubt that.

> >  For the latter
> > there is mark_addressable function.
> 
> This is certainly cheaper to use.

Even cheaper is just do nothing, tree-ssa-operands.c during
update_stmt_operands (which will be called for all newly created stmts,
upon update_stmt etc.) will take care of calling mark_address_taken.

	Jakub

On Tue, Nov 13, 2012 at 9:36 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Nov 13, 2012 at 09:25:36AM -0800, Xinliang David Li wrote:
>> > That is complete misunderstanding of what update_address_taken does.
>> > It removes TREE_ADDRESSABLE from addressables that are no longer
>> > addressable, rather than adding TREE_ADDRESSABLE bits.
>>
>> It will do the latter too. See iv-opts.
>
> Where?  I strongly doubt that.

You are right. iv-opts is probably a good example that the update_stmt
can do the right job.

thanks,

David


>
>> >  For the latter
>> > there is mark_addressable function.
>>
>> This is certainly cheaper to use.
>
> Even cheaper is just do nothing, tree-ssa-operands.c during
> update_stmt_operands (which will be called for all newly created stmts,
> upon update_stmt etc.) will take care of calling mark_address_taken.
>
>         Jakub