This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [tsan] ThreadSanitizer instrumentation part


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]