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: [asan] Emit GIMPLE directly, small cleanups


On Thu, Oct 11, 2012 at 9:38 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Building trees, then gimplifying it, is unnecessarily expensive.
> This patch changes build_check_stmt to emit GIMPLE directly, and
> a couple of small cleanups here and there.  Also, I'm using a different
> alias set for the shadow memory accesses, those obviously can't alias any
> other accesses.
>
> Ok for asan?
>
> 2012-10-11  Jakub Jelinek  <jakub@redhat.com>
>
>         * Makefile.in (GTFILES): Add $(srcdir)/asan.c.
>         * asan.c (shadow_ptr_types): New variable.
>         (report_error_func): Change is_store argument to bool, don't append
>         newline to function name.
>         (PROB_VERY_UNLIKELY, PROB_ALWAYS): Define.
>         (build_check_stmt): Change is_store argument to bool.  Emit GIMPLE
>         directly instead of creating trees and gimplifying them.  Mark
>         the error reporting function as very unlikely.
>         (instrument_derefs): Change is_store argument to bool.  Use
>         int_size_in_bytes to compute size_in_bytes, simplify size check.
>         Use build_fold_addr_expr instead of build_addr.
>         (transform_statements): Adjust instrument_derefs caller.
>         Use gimple_assign_single_p as stmt test.  Don't look at MEM refs
>         in rhs2.
>         (asan_instrument): Don't push/pop gimplify context.
>         Initialize shadow_ptr_types if not yet initialized.
>         * asan.h (ASAN_SHADOW_SHIFT): Adjust comment.
>
> --- gcc/Makefile.in.jj  2012-10-11 16:09:02.000000000 +0200
> +++ gcc/Makefile.in     2012-10-11 16:51:59.666672099 +0200
> @@ -3681,6 +3681,7 @@ GTFILES = $(CPP_ID_DATA_H) $(srcdir)/inp
>    $(srcdir)/lto-streamer.h \
>    $(srcdir)/target-globals.h \
>    $(srcdir)/ipa-inline.h \
> +  $(srcdir)/asan.c \
>    @all_gtfiles@
>
>  # Compute the list of GT header files from the corresponding C sources,
> --- gcc/asan.c.jj       2012-10-11 16:33:58.000000000 +0200
> +++ gcc/asan.c  2012-10-11 18:20:35.265675838 +0200
> @@ -79,18 +79,20 @@ along with GCC; see the file COPYING3.
>   to create redzones for stack and global object and poison them.
>  */
>
> +static GTY(()) tree shadow_ptr_types[2];
> +
>  /* Construct a function tree for __asan_report_{load,store}{1,2,4,8,16}.
>     IS_STORE is either 1 (for a store) or 0 (for a load).
>     SIZE_IN_BYTES is one of 1, 2, 4, 8, 16.  */
>
>  static tree
> -report_error_func (int is_store, int size_in_bytes)
> +report_error_func (bool is_store, int size_in_bytes)
>  {
>    tree fn_type;
>    tree def;
>    char name[100];
>
> -  sprintf (name, "__asan_report_%s%d\n",
> +  sprintf (name, "__asan_report_%s%d",
>             is_store ? "store" : "load", size_in_bytes);
>    fn_type = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE);
>    def = build_fn_decl (name, fn_type);
> @@ -118,6 +120,9 @@ asan_init_func (void)
>  }
>
>
> +#define PROB_VERY_UNLIKELY     (REG_BR_PROB_BASE / 2000 - 1)
> +#define PROB_ALWAYS            (REG_BR_PROB_BASE)
> +


Does it belong here ? -- looks like they can be generally useful for others.



>  /* Instrument the memory access instruction BASE.
>     Insert new statements before ITER.
>     LOCATION is source code location.
> @@ -127,21 +132,17 @@ asan_init_func (void)
>  static void
>  build_check_stmt (tree base,
>                    gimple_stmt_iterator *iter,
> -                  location_t location, int is_store, int size_in_bytes)
> +                  location_t location, bool is_store, int size_in_bytes)
>  {
>    gimple_stmt_iterator gsi;
>    basic_block cond_bb, then_bb, join_bb;
>    edge e;
> -  tree cond, t, u;
> -  tree base_addr;
> -  tree shadow_value;
> +  tree t, base_addr, shadow;
>    gimple g;
> -  gimple_seq seq, stmts;
> -  tree shadow_type = size_in_bytes == 16 ?
> -      short_integer_type_node : char_type_node;
> -  tree shadow_ptr_type = build_pointer_type (shadow_type);
> -  tree uintptr_type = lang_hooks.types.type_for_mode (ptr_mode,
> -                                                      /*unsignedp=*/true);
> +  tree shadow_ptr_type = shadow_ptr_types[size_in_bytes == 16];
> +  tree shadow_type = TREE_TYPE (shadow_ptr_type);
> +  tree uintptr_type
> +    = build_nonstandard_integer_type (TYPE_PRECISION (TREE_TYPE (base)), 1);
>
>    /* We first need to split the current basic block, and start altering
>       the CFG.  This allows us to insert the statements we're about to
> @@ -166,14 +167,15 @@ build_check_stmt (tree base,
>
>    /* Create the bb that contains the crash block.  */
>    then_bb = create_empty_bb (cond_bb);

Missing frequency update for then_bb ?


> -  make_edge (cond_bb, then_bb, EDGE_TRUE_VALUE);
> +  e = make_edge (cond_bb, then_bb, EDGE_TRUE_VALUE);
> +  e->probability = PROB_VERY_UNLIKELY;
>    make_single_succ_edge (then_bb, join_bb, EDGE_FALLTHRU);
>
>    /* Mark the pseudo-fallthrough edge from cond_bb to join_bb.  */
>    e = find_edge (cond_bb, join_bb);
>    e->flags = EDGE_FALSE_VALUE;
>    e->count = cond_bb->count;
> -  e->probability = REG_BR_PROB_BASE;
> +  e->probability = PROB_ALWAYS - PROB_VERY_UNLIKELY;
>
>    /* Update dominance info.  Note that bb_join's data was
>       updated by split_block.  */
> @@ -183,75 +185,123 @@ build_check_stmt (tree base,
>        set_immediate_dominator (CDI_DOMINATORS, join_bb, cond_bb);
>      }
>
> -  base_addr = create_tmp_reg (uintptr_type, "__asan_base_addr");
> +  gsi = gsi_last_bb (cond_bb);
> +  g = gimple_build_assign_with_ops (TREE_CODE (base),
> +                                   make_ssa_name (TREE_TYPE (base), NULL),
> +                                   base, NULL_TREE);
> +  gimple_set_location (g, location);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
>
> -  seq = NULL;
> -  t = fold_convert_loc (location, uintptr_type,
> -                        unshare_expr (base));
> -  t = force_gimple_operand (t, &stmts, false, NULL_TREE);
> -  gimple_seq_add_seq (&seq, stmts);
> -  g = gimple_build_assign (base_addr, t);
> +  g = gimple_build_assign_with_ops (NOP_EXPR,
> +                                   make_ssa_name (uintptr_type, NULL),
> +                                   gimple_assign_lhs (g), NULL_TREE);
>    gimple_set_location (g, location);
> -  gimple_seq_add_stmt (&seq, g);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +  base_addr = gimple_assign_lhs (g);
>


Set base_addr name here?


thanks,

David


>    /* Build
> -     (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset ().  */
> +     (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset ().  */
>
> -  t = build2 (RSHIFT_EXPR, uintptr_type, base_addr,
> -             build_int_cst (uintptr_type, ASAN_SHADOW_SHIFT));
> -  t = build2 (PLUS_EXPR, uintptr_type, t,
> -             build_int_cst (uintptr_type, targetm.asan_shadow_offset ()));
> -  t = build1 (INDIRECT_REF, shadow_type,
> -              build1 (VIEW_CONVERT_EXPR, shadow_ptr_type, t));
> -  t = force_gimple_operand (t, &stmts, false, NULL_TREE);
> -  gimple_seq_add_seq (&seq, stmts);
> -  shadow_value = create_tmp_reg (shadow_type, "__asan_shadow");
> -  g = gimple_build_assign (shadow_value, t);
> +  t = build_int_cst (uintptr_type, ASAN_SHADOW_SHIFT);
> +  g = gimple_build_assign_with_ops (RSHIFT_EXPR,
> +                                   make_ssa_name (uintptr_type, NULL),
> +                                   base_addr, t);
>    gimple_set_location (g, location);
> -  gimple_seq_add_stmt (&seq, g);
> -  t = build2 (NE_EXPR, boolean_type_node, shadow_value,
> -              build_int_cst (shadow_type, 0));
> -  if (size_in_bytes < 8)
> -    {
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
>
> -      /* Slow path for 1-, 2- and 4- byte accesses.
> -         Build ((base_addr & 7) + (size_in_bytes - 1)) >= shadow_value.  */
> -
> -      u = build2 (BIT_AND_EXPR, uintptr_type,
> -                  base_addr,
> -                  build_int_cst (uintptr_type, 7));
> -      u = build1 (CONVERT_EXPR, shadow_type, u);
> -      u = build2 (PLUS_EXPR, shadow_type, u,
> -                  build_int_cst (shadow_type, size_in_bytes - 1));
> -      u = build2 (GE_EXPR, uintptr_type, u, shadow_value);
> -    }
> -  else
> -      u = build_int_cst (boolean_type_node, 1);
> -  t = build2 (TRUTH_AND_EXPR, boolean_type_node, t, u);
> -  t = force_gimple_operand (t, &stmts, false, NULL_TREE);
> -  gimple_seq_add_seq (&seq, stmts);
> -  cond = create_tmp_reg (boolean_type_node, "__asan_crash_cond");
> -  g = gimple_build_assign  (cond, t);
> +  t = build_int_cst (uintptr_type, targetm.asan_shadow_offset ());
> +  g = gimple_build_assign_with_ops (PLUS_EXPR,
> +                                   make_ssa_name (uintptr_type, NULL),
> +                                   gimple_assign_lhs (g), t);
>    gimple_set_location (g, location);
> -  gimple_seq_add_stmt (&seq, g);
> -  g = gimple_build_cond (NE_EXPR, cond, boolean_false_node, NULL_TREE,
> -                         NULL_TREE);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +
> +  g = gimple_build_assign_with_ops (NOP_EXPR,
> +                                   make_ssa_name (shadow_ptr_type, NULL),
> +                                   gimple_assign_lhs (g), NULL_TREE);
>    gimple_set_location (g, location);
> -  gimple_seq_add_stmt (&seq, g);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
>
> -  /* Generate call to the run-time library (e.g. __asan_report_load8).  */
> +  t = build2 (MEM_REF, shadow_type, gimple_assign_lhs (g),
> +             build_int_cst (shadow_ptr_type, 0));
> +  g = gimple_build_assign_with_ops (MEM_REF,
> +                                   make_ssa_name (shadow_type, NULL),
> +                                   t, NULL_TREE);
> +  gimple_set_location (g, location);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +  shadow = gimple_assign_lhs (g);
>
> -  gsi = gsi_last_bb (cond_bb);
> -  gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING);
> -  seq = NULL;
> -  g = gimple_build_call (report_error_func (is_store, size_in_bytes),
> -                         1, base_addr);
> -  gimple_seq_add_stmt (&seq, g);
> +  if (size_in_bytes < 8)
> +    {
> +      /* Slow path for 1, 2 and 4 byte accesses.
> +        Test (shadow != 0)
> +             & ((base_addr & 7) + (size_in_bytes - 1)) >= shadow).  */
> +      g = gimple_build_assign_with_ops (NE_EXPR,
> +                                       make_ssa_name (boolean_type_node,
> +                                                      NULL),
> +                                       shadow,
> +                                       build_int_cst (shadow_type, 0));
> +      gimple_set_location (g, location);
> +      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +      t = gimple_assign_lhs (g);
> +
> +      g = gimple_build_assign_with_ops (BIT_AND_EXPR,
> +                                       make_ssa_name (uintptr_type,
> +                                                      NULL),
> +                                       base_addr,
> +                                       build_int_cst (uintptr_type, 7));
> +      gimple_set_location (g, location);
> +      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +
> +      g = gimple_build_assign_with_ops (NOP_EXPR,
> +                                       make_ssa_name (shadow_type,
> +                                                      NULL),
> +                                       gimple_assign_lhs (g), NULL_TREE);
> +      gimple_set_location (g, location);
> +      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +
> +      if (size_in_bytes > 1)
> +       {
> +         g = gimple_build_assign_with_ops (PLUS_EXPR,
> +                                           make_ssa_name (shadow_type,
> +                                                          NULL),
> +                                           gimple_assign_lhs (g),
> +                                           build_int_cst (shadow_type,
> +                                                          size_in_bytes - 1));
> +         gimple_set_location (g, location);
> +         gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +       }
> +
> +      g = gimple_build_assign_with_ops (GE_EXPR,
> +                                       make_ssa_name (boolean_type_node,
> +                                                      NULL),
> +                                       gimple_assign_lhs (g),
> +                                       shadow);
> +      gimple_set_location (g, location);
> +      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +
> +      g = gimple_build_assign_with_ops (BIT_AND_EXPR,
> +                                       make_ssa_name (boolean_type_node,
> +                                                      NULL),
> +                                       t, gimple_assign_lhs (g));
> +      gimple_set_location (g, location);
> +      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +      t = gimple_assign_lhs (g);
> +    }
> +  else
> +    t = shadow;
>
> -  /* Insert the check code in the THEN block.  */
> +  g = gimple_build_cond (NE_EXPR, t, build_int_cst (TREE_TYPE (t), 0),
> +                        NULL_TREE, NULL_TREE);
> +  gimple_set_location (g, location);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
>
> +  /* Generate call to the run-time library (e.g. __asan_report_load8).  */
>    gsi = gsi_start_bb (then_bb);
> -  gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING);
> +  g = gimple_build_call (report_error_func (is_store, size_in_bytes),
> +                        1, base_addr);
> +  gimple_set_location (g, location);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
>
>    *iter = gsi_start_bb (join_bb);
>  }
> @@ -262,14 +312,12 @@ build_check_stmt (tree base,
>
>  static void
>  instrument_derefs (gimple_stmt_iterator *iter, tree t,
> -                  location_t location, int is_store)
> +                  location_t location, bool is_store)
>  {
>    tree type, base;
> -  int size_in_bytes;
> +  HOST_WIDE_INT size_in_bytes;
>
>    type = TREE_TYPE (t);
> -  if (type == error_mark_node)
> -    return;
>    switch (TREE_CODE (t))
>      {
>      case ARRAY_REF:
> @@ -280,25 +328,25 @@ instrument_derefs (gimple_stmt_iterator
>      default:
>        return;
>      }


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