[asan] Emit GIMPLE directly, small cleanups

[asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

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)
+
 /* 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);
-  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);
 
   /* 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;
     }
-  size_in_bytes = tree_low_cst (TYPE_SIZE (type), 0) / BITS_PER_UNIT;
-  if (size_in_bytes != 1 && size_in_bytes != 2 &&
-      size_in_bytes != 4 && size_in_bytes != 8 && size_in_bytes != 16)
-      return;
-  {
-    /* For now just avoid instrumenting bit field acceses.
+
+  size_in_bytes = int_size_in_bytes (type);
+  if ((size_in_bytes & (size_in_bytes - 1)) != 0
+      || (unsigned HOST_WIDE_INT) size_in_bytes - 1 >= 16)
+    return;
+
+  /* For now just avoid instrumenting bit field acceses.
      Fixing it is doable, but expected to be messy.  */
 
-    HOST_WIDE_INT bitsize, bitpos;
-    tree offset;
-    enum machine_mode mode;
-    int volatilep = 0, unsignedp = 0;
-    get_inner_reference (t, &bitsize, &bitpos, &offset,
-                         &mode, &unsignedp, &volatilep, false);
-    if (bitpos != 0 || bitsize != size_in_bytes * BITS_PER_UNIT)
-        return;
-  }
+  HOST_WIDE_INT bitsize, bitpos;
+  tree offset;
+  enum machine_mode mode;
+  int volatilep = 0, unsignedp = 0;
+  get_inner_reference (t, &bitsize, &bitpos, &offset,
+		       &mode, &unsignedp, &volatilep, false);
+  if (bitpos != 0 || bitsize != size_in_bytes * BITS_PER_UNIT)
+    return;
 
-  base = build_addr (t, current_function_decl);
+  base = build_fold_addr_expr (t);
   build_check_stmt (base, iter, location, is_store, size_in_bytes);
 }
 
@@ -314,7 +362,6 @@ transform_statements (void)
   basic_block bb;
   gimple_stmt_iterator i;
   int saved_last_basic_block = last_basic_block;
-  enum gimple_rhs_class grhs_class;
 
   FOR_EACH_BB (bb)
     {
@@ -322,16 +369,12 @@ transform_statements (void)
       for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
         {
           gimple s = gsi_stmt (i);
-          if (gimple_code (s) != GIMPLE_ASSIGN)
-              continue;
+          if (!gimple_assign_single_p (s))
+	    continue;
           instrument_derefs (&i, gimple_assign_lhs (s),
-                             gimple_location (s), 1);
+                             gimple_location (s), true);
           instrument_derefs (&i, gimple_assign_rhs1 (s),
-                             gimple_location (s), 0);
-          grhs_class = get_gimple_rhs_class (gimple_assign_rhs_code (s));
-          if (grhs_class == GIMPLE_BINARY_RHS)
-            instrument_derefs (&i, gimple_assign_rhs2 (s),
-                               gimple_location (s), 0);
+                             gimple_location (s), false);
         }
     }
 }
@@ -356,10 +399,19 @@ asan_finish_file (void)
 static unsigned int
 asan_instrument (void)
 {
-  struct gimplify_ctx gctx;
-  push_gimplify_context (&gctx);
+  if (shadow_ptr_types[0] == NULL_TREE)
+    {
+      alias_set_type set = new_alias_set ();
+      shadow_ptr_types[0]
+	= build_distinct_type_copy (unsigned_char_type_node);
+      TYPE_ALIAS_SET (shadow_ptr_types[0]) = set;
+      shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]);
+      shadow_ptr_types[1]
+	= build_distinct_type_copy (short_unsigned_type_node);
+      TYPE_ALIAS_SET (shadow_ptr_types[1]) = set;
+      shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]);
+    }
   transform_statements ();
-  pop_gimplify_context (NULL);
   return 0;
 }
 
@@ -385,6 +437,8 @@ struct gimple_opt_pass pass_asan =
   0,                                    /* properties_destroyed  */
   0,                                    /* todo_flags_start  */
   TODO_verify_flow | TODO_verify_stmts
-  | TODO_update_ssa    /* todo_flags_finish  */
+  | TODO_update_ssa			/* todo_flags_finish  */
  }
 };
+
+#include "gt-asan.h"
--- gcc/asan.h.jj	2012-10-11 16:04:28.000000000 +0200
+++ gcc/asan.h	2012-10-11 18:09:07.430449499 +0200
@@ -24,7 +24,7 @@ along with GCC; see the file COPYING3.
 extern void asan_finish_file(void);
 
 /* Shadow memory is found at
-   (address >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset ().  */
+   (address >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset ().  */
 #define ASAN_SHADOW_SHIFT	3
 
 #endif /* TREE_ASAN */

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Diego Novillo]

On 2012-10-11 12:38 , Jakub Jelinek wrote:

> -  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];

Add '? 1 : 0' in the array index expression.

>     /* Build
> -     (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset ().  */
> +     (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset ().  */

Hm, I wonder if this is a documentation problem or we're generating bad 
runtime code.  Wei, you tested the runtime and it was working with the 
GCC generated code, right?

In any case, we can adjust the expression later.

> +  if (shadow_ptr_types[0] == NULL_TREE)
> +    {
> +      alias_set_type set = new_alias_set ();
> +      shadow_ptr_types[0]
> +	= build_distinct_type_copy (unsigned_char_type_node);
> +      TYPE_ALIAS_SET (shadow_ptr_types[0]) = set;
> +      shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]);
> +      shadow_ptr_types[1]
> +	= build_distinct_type_copy (short_unsigned_type_node);
> +      TYPE_ALIAS_SET (shadow_ptr_types[1]) = set;
> +      shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]);
> +    }

Move this to an initialization function, please.


OK with those changes.


Diego.

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Thu, Oct 11, 2012 at 01:14:31PM -0400, Diego Novillo wrote:
> On 2012-10-11 12:38 , Jakub Jelinek wrote:
> 
> >-  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];
> 
> Add '? 1 : 0' in the array index expression.

Ok.

> >    /* Build
> >-     (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset ().  */
> >+     (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset ().  */
> 
> Hm, I wonder if this is a documentation problem or we're generating
> bad runtime code.  Wei, you tested the runtime and it was working
> with the GCC generated code, right?

The asan web pages document |, the old tree-asan.c emitted +, I've changed
it to BIT_IOR_EXPR, but that resulted in worse assembly, and I believe at
least for the current x86_64 and i686 address ranges and shadow offset
values it actually doesn't matter.
On x86_64 stack is like 0x7ffff6e00000, shifted down by 3 is still smaller
than 1L << 44 that is ored or added to it.  And the negative half of the
address space is used by the kernel, nothing is mapped into it (besides
vsyscall page) and neither | nor + of 1L << 44 to it would work well.
On i386, | and + works the same for all addresses, as 0xffffffffU >> 3
is still smaller than 1 << 29.
The reason why + generates better code on x86_64/i686 is that one can use
e.g. movzbl (%r1, %r2), %r3 instead of orq %r2, %r1; movzb (%r1), %r3.

> >+  if (shadow_ptr_types[0] == NULL_TREE)
> >+    {
> >+      alias_set_type set = new_alias_set ();
> >+      shadow_ptr_types[0]
> >+	= build_distinct_type_copy (unsigned_char_type_node);
> >+      TYPE_ALIAS_SET (shadow_ptr_types[0]) = set;
> >+      shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]);
> >+      shadow_ptr_types[1]
> >+	= build_distinct_type_copy (short_unsigned_type_node);
> >+      TYPE_ALIAS_SET (shadow_ptr_types[1]) = set;
> >+      shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]);
> >+    }
> 
> Move this to an initialization function, please.

Okay.

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

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;
>      }

Re: [asan] Emit GIMPLE directly, small cleanups

[Wei Mi]

Hi Diego,

>>     /* Build
>> -     (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset ().
>> */
>> +     (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset ().
>> */
>
>
> Hm, I wonder if this is a documentation problem or we're generating bad
> runtime code.  Wei, you tested the runtime and it was working with the GCC
> generated code, right?
>
> In any case, we can adjust the expression later.

I only tested my smallcase and it worked. Because usually the redzone
are not a very small areas (more than 4K), so I think it is possible
the smallcase works even if the shadow addr calculation is incorrect
and has small deviation.

Thanks,
Wei.

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Thu, Oct 11, 2012 at 10:31:58AM -0700, Xinliang David Li wrote:
> > +#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.

Perhaps, but then it would need to go on mainline first, and wait for a
merge from the trunk to asan.  So, IMHO better to put it in now this way
and if mainline gets the macros moved from profile.c to a header, asan
branch can be then adjusted.

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

I don't see other places which create very unlikely edges doing
that (e.g. trans-mem.c, omp-low.c, ...).  Shall it be that
  then_bb->frequency = cond_bb->frequency * PROB_VERY_UNLIKELY
		       / REG_BR_PROB_BASE;
and join_bb->frequency -= then_bb->frequency; ?
join_bb is clearly misnamed, as then_bb is noreturn, so there is no
joining...

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

Why?  I think nameless SSA_NAMEs are just fine for this.

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

On Thu, Oct 11, 2012 at 11:12 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 11, 2012 at 10:31:58AM -0700, Xinliang David Li wrote:
>> > +#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.
>
> Perhaps, but then it would need to go on mainline first, and wait for a
> merge from the trunk to asan.  So, IMHO better to put it in now this way
> and if mainline gets the macros moved from profile.c to a header, asan
> branch can be then adjusted.

reasonable. The existing definitions are in predict.c.
>
>> > @@ -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 ?
>
> I don't see other places which create very unlikely edges doing
> that (e.g. trans-mem.c, omp-low.c, ...).  Shall it be that
>   then_bb->frequency = cond_bb->frequency * PROB_VERY_UNLIKELY
>                        / REG_BR_PROB_BASE;
> and join_bb->frequency -= then_bb->frequency; ?

That looks good to me.

> join_bb is clearly misnamed, as then_bb is noreturn, so there is no
> joining...
>
>> >    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?
>
> Why?  I think nameless SSA_NAMEs are just fine for this.

It makes the GIMPLE dump slightly more readable.

thanks,

David
>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Wei Mi]

Hi,

Here is the initial test results of gcc asan patch, and it shows us
some missing features in gcc but existing in llvm.
[1]. gcc regression test for gcc-asan passes.
[2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests
written in google test and 24 failures in 28 for tests written in lit
tests.

gcc missing features:
1. gcc implementation doesn't support stack/global overflow check
1. gcc implementation doesn't support some attributes, such as
__attribute__((no_address_safety_analysis)), which llvm does
2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does
3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy
for heap allocated memory or string, which llvm does
4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm
-asan-initialization-order
5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit
tests contain checks from -O0 to -O3, which makes gcc fail.
6. $HOME/llvm/trunk/projects/compiler-rt/lib/asan/scripts/asan_symbolize.py
could generate valid source locations from virtual addresses for llvm
binary, but fail to do that for gcc binary.  example, llvm result #1
0x402694 in main heap-overflow.cc:23 .vs. gcc result: #1 0x402694 in
main ??:0. Some FileCheck in llvm lit tests expect the valid source
locations.

Thanks,
Wei.


On Thu, Oct 11, 2012 at 10:31 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 11, 2012 at 01:14:31PM -0400, Diego Novillo wrote:
>> On 2012-10-11 12:38 , Jakub Jelinek wrote:
>>
>> >-  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];
>>
>> Add '? 1 : 0' in the array index expression.
>
> Ok.
>
>> >    /* Build
>> >-     (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset ().  */
>> >+     (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset ().  */
>>
>> Hm, I wonder if this is a documentation problem or we're generating
>> bad runtime code.  Wei, you tested the runtime and it was working
>> with the GCC generated code, right?
>
> The asan web pages document |, the old tree-asan.c emitted +, I've changed
> it to BIT_IOR_EXPR, but that resulted in worse assembly, and I believe at
> least for the current x86_64 and i686 address ranges and shadow offset
> values it actually doesn't matter.
> On x86_64 stack is like 0x7ffff6e00000, shifted down by 3 is still smaller
> than 1L << 44 that is ored or added to it.  And the negative half of the
> address space is used by the kernel, nothing is mapped into it (besides
> vsyscall page) and neither | nor + of 1L << 44 to it would work well.
> On i386, | and + works the same for all addresses, as 0xffffffffU >> 3
> is still smaller than 1 << 29.
> The reason why + generates better code on x86_64/i686 is that one can use
> e.g. movzbl (%r1, %r2), %r3 instead of orq %r2, %r1; movzb (%r1), %r3.
>
>> >+  if (shadow_ptr_types[0] == NULL_TREE)
>> >+    {
>> >+      alias_set_type set = new_alias_set ();
>> >+      shadow_ptr_types[0]
>> >+    = build_distinct_type_copy (unsigned_char_type_node);
>> >+      TYPE_ALIAS_SET (shadow_ptr_types[0]) = set;
>> >+      shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]);
>> >+      shadow_ptr_types[1]
>> >+    = build_distinct_type_copy (short_unsigned_type_node);
>> >+      TYPE_ALIAS_SET (shadow_ptr_types[1]) = set;
>> >+      shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]);
>> >+    }
>>
>> Move this to an initialization function, please.
>
> Okay.
>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Thu, Oct 11, 2012 at 04:19:18PM -0700, Wei Mi wrote:
> Here is the initial test results of gcc asan patch, and it shows us
> some missing features in gcc but existing in llvm.
> [1]. gcc regression test for gcc-asan passes.
> [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests
> written in google test and 24 failures in 28 for tests written in lit
> tests.
> 
> gcc missing features:

I think LLVM calls the option -faddress-sanitizer, perhaps we should rename
-fasan to that too for some compatibility.  For the varios knobs LLVM asan
has we could add params to params.def if we want to support them.

> 1. gcc implementation doesn't support stack/global overflow check

Yeah, I think the stack check shouldn't be that hard and can hack it up,
I'll perhaps leave the global vars stuff to Dodji or others if he has time.

> 1. gcc implementation doesn't support some attributes, such as
> __attribute__((no_address_safety_analysis)), which llvm does

Yeah, shouldn't be hard.

> 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does

Testcase (for everything testcases would be useful, and of course the
runtime library too)?  Yeah, the code currently punts on those, the question
is how to handle them.

> 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy
> for heap allocated memory or string, which llvm does

That is easy (we can easily handle instrumenting lots of builtins).
Just a big switch on DECL_FUNCTION_CODE, collecting src/dst addresses and
length for each of them and adding the instrumentation for first and last
bytes in the strings.

> 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm
> -asan-initialization-order

I must say I don't like the -asan-blacklist option at all, IMHO it is much
saner to just ask users to use attributes (or pragmas around whole headers).

> 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit
> tests contain checks from -O0 to -O3, which makes gcc fail.

That is because of the place where the instrumentation is scheduled right
now in the pass queue.  We can easily add a pass_asan_O0 that will run say
close to pass_lower_complex_O0.

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Richard Biener]

On Thu, Oct 11, 2012 at 7:31 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 11, 2012 at 01:14:31PM -0400, Diego Novillo wrote:
>> On 2012-10-11 12:38 , Jakub Jelinek wrote:
>>
>> >-  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];
>>
>> Add '? 1 : 0' in the array index expression.
>
> Ok.
>
>> >    /* Build
>> >-     (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset ().  */
>> >+     (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset ().  */
>>
>> Hm, I wonder if this is a documentation problem or we're generating
>> bad runtime code.  Wei, you tested the runtime and it was working
>> with the GCC generated code, right?
>
> The asan web pages document |, the old tree-asan.c emitted +, I've changed
> it to BIT_IOR_EXPR, but that resulted in worse assembly, and I believe at
> least for the current x86_64 and i686 address ranges and shadow offset
> values it actually doesn't matter.
> On x86_64 stack is like 0x7ffff6e00000, shifted down by 3 is still smaller
> than 1L << 44 that is ored or added to it.  And the negative half of the
> address space is used by the kernel, nothing is mapped into it (besides
> vsyscall page) and neither | nor + of 1L << 44 to it would work well.
> On i386, | and + works the same for all addresses, as 0xffffffffU >> 3
> is still smaller than 1 << 29.
> The reason why + generates better code on x86_64/i686 is that one can use
> e.g. movzbl (%r1, %r2), %r3 instead of orq %r2, %r1; movzb (%r1), %r3.
>
>> >+  if (shadow_ptr_types[0] == NULL_TREE)
>> >+    {
>> >+      alias_set_type set = new_alias_set ();
>> >+      shadow_ptr_types[0]
>> >+    = build_distinct_type_copy (unsigned_char_type_node);
>> >+      TYPE_ALIAS_SET (shadow_ptr_types[0]) = set;
>> >+      shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]);
>> >+      shadow_ptr_types[1]
>> >+    = build_distinct_type_copy (short_unsigned_type_node);
>> >+      TYPE_ALIAS_SET (shadow_ptr_types[1]) = set;
>> >+      shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]);
>> >+    }
>>
>> Move this to an initialization function, please.
>
> Okay.

That doesn't work with LTO btw, so make sure you instrument during LTRANS.

Richard.

>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Diego Novillo]

On 2012-10-11 19:19 , Wei Mi wrote:> Hi,
 >
 > Here is the initial test results of gcc asan patch, and it shows us
 > some missing features in gcc but existing in llvm.
 > [1]. gcc regression test for gcc-asan passes.
 > [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests
 > written in google test and 24 failures in 28 for tests written in lit
 > tests.
 >
 > gcc missing features:
 > 1. gcc implementation doesn't support stack/global overflow check
 > 1. gcc implementation doesn't support some attributes, such as
 > __attribute__((no_address_safety_analysis)), which llvm does
 > 2. gcc doesn't detect out-of-bound bitfield access of heap, which 
llvm does
 > 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy
 > for heap allocated memory or string, which llvm does
 > 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm
 > -asan-initialization-order
 > 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit
 > tests contain checks from -O0 to -O3, which makes gcc fail.
 > 6. 
$HOME/llvm/trunk/projects/compiler-rt/lib/asan/scripts/asan_symbolize.py
 > could generate valid source locations from virtual addresses for llvm
 > binary, but fail to do that for gcc binary.  example, llvm result #1
 > 0x402694 in main heap-overflow.cc:23 .vs. gcc result: #1 0x402694 in
 > main ??:0. Some FileCheck in llvm lit tests expect the valid source
 > locations.

Thanks for the analysis Wei.

We need to decide what's the best way of dealing with the runtime.  My 
inclination is to treat it the same way we treat gmp, mpfr, et al:

1- If libasan/ exists, we enable asan as a feature at configure time.

2- libasan/ is always a pristine copy of the LLVM repository.  We could 
have tarballs in ftp://gcc.gnu.org/pub/gcc/infrastructure/ which are 
brought in by contrib/download_prerequisites.

3- To run ASAN's testsuite, I propose a simple wrapper script that 
executes it using the just-built gcc.  I don't think it's worth the pain 
to convert the testsuite into DejaGNU.

Comments?


Thanks.  Diego.

Re: [asan] Emit GIMPLE directly, small cleanups

[Rainer Orth]

Diego Novillo <dnovillo@google.com> writes:

> 3- To run ASAN's testsuite, I propose a simple wrapper script that executes
> it using the just-built gcc.  I don't think it's worth the pain to convert
> the testsuite into DejaGNU.

If the testsuite is not converted (which can be ugly for maintainers
since it needs separate mechanisms for everything Dejagnu already deals
with, like timeouts, target-specific skipping), at the very least make
sure that the make check output is in the same format produced by
Dejagnu so it's picked up make make mail-report.log, otherwise failures
are almost guaranteed to be overlooked.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [asan] Emit GIMPLE directly, small cleanups

[Diego Novillo]

On 2012-10-12 11:01 , Rainer Orth wrote:
> Diego Novillo <dnovillo@google.com> writes:
>
>> 3- To run ASAN's testsuite, I propose a simple wrapper script that executes
>> it using the just-built gcc.  I don't think it's worth the pain to convert
>> the testsuite into DejaGNU.
>
> If the testsuite is not converted (which can be ugly for maintainers
> since it needs separate mechanisms for everything Dejagnu already deals
> with, like timeouts, target-specific skipping), at the very least make

The thing is that if we convert the testsuite, then taking pristine 
tarballs is out of the question and we end up with a library like 
boehm-gc or zlib.  I would prefer to have something completely separate 
that we can just drop in.

> sure that the make check output is in the same format produced by
> Dejagnu so it's picked up make make mail-report.log, otherwise failures
> are almost guaranteed to be overlooked.

Sure.  That's a good idea.


Diego.

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

On Fri, Oct 12, 2012 at 12:15 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 11, 2012 at 04:19:18PM -0700, Wei Mi wrote:
>> Here is the initial test results of gcc asan patch, and it shows us
>> some missing features in gcc but existing in llvm.
>> [1]. gcc regression test for gcc-asan passes.
>> [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests
>> written in google test and 24 failures in 28 for tests written in lit
>> tests.
>>
>> gcc missing features:
>
> I think LLVM calls the option -faddress-sanitizer, perhaps we should rename
> -fasan to that too for some compatibility.  For the varios knobs LLVM asan
> has we could add params to params.def if we want to support them.
>
>> 1. gcc implementation doesn't support stack/global overflow check
>
> Yeah, I think the stack check shouldn't be that hard and can hack it up,
> I'll perhaps leave the global vars stuff to Dodji or others if he has time.

Since the stack part is relative self contained, might it be better
for Wei (he is new) to tackle it with guidance from you?

Global handling needs people more experienced with varasm stuff.

>
>> 1. gcc implementation doesn't support some attributes, such as
>> __attribute__((no_address_safety_analysis)), which llvm does
>
> Yeah, shouldn't be hard.
>

yes -- that is simple.


>> 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does
>
> Testcase (for everything testcases would be useful, and of course the
> runtime library too)?  Yeah, the code currently punts on those, the question
> is how to handle them.

The code should sythensize the smallest containing
byte/word/doubleword/qword of the bitfield and then compute the shadow
address.

>
>> 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy
>> for heap allocated memory or string, which llvm does
>
> That is easy (we can easily handle instrumenting lots of builtins).
> Just a big switch on DECL_FUNCTION_CODE, collecting src/dst addresses and
> length for each of them and adding the instrumentation for first and last
> bytes in the strings.

Yes.

>
>> 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm
>> -asan-initialization-order
>
> I must say I don't like the -asan-blacklist option at all, IMHO it is much
> saner to just ask users to use attributes (or pragmas around whole headers).

But the option is easy to have too.

>
>> 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit
>> tests contain checks from -O0 to -O3, which makes gcc fail.
>
> That is because of the place where the instrumentation is scheduled right
> now in the pass queue.  We can easily add a pass_asan_O0 that will run say
> close to pass_lower_complex_O0.

Right.

thanks,

David
>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Fri, Oct 12, 2012 at 09:30:33AM -0700, Xinliang David Li wrote:
> > Yeah, I think the stack check shouldn't be that hard and can hack it up,
> > I'll perhaps leave the global vars stuff to Dodji or others if he has time.
> 
> Since the stack part is relative self contained, might it be better
> for Wei (he is new) to tackle it with guidance from you?

Too late, already posted (just is missing actually pushing the cleanup
sequence in the right spots).

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

On Fri, Oct 12, 2012 at 9:32 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Oct 12, 2012 at 09:30:33AM -0700, Xinliang David Li wrote:
>> > Yeah, I think the stack check shouldn't be that hard and can hack it up,
>> > I'll perhaps leave the global vars stuff to Dodji or others if he has time.
>>
>> Since the stack part is relative self contained, might it be better
>> for Wei (he is new) to tackle it with guidance from you?
>
> Too late, already posted (just is missing actually pushing the cleanup
> sequence in the right spots).


that is fast :)

thanks,

David
>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

On Fri, Oct 12, 2012 at 12:26 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 11, 2012 at 10:05:54PM -0700, Xinliang David Li wrote:
>> Was:
>>     Type global;
>> Now:
>>     struct {  // at least 32-byte aligned
>>        Type orig;
>>        char redzone[32 + required_for_alignment];
>>     } global;
>>
>> All uses of global in the module should be replaced with global.orig
>>
>> (Note that the actual implementation does not need to change global
>> type nor references -- just adding the padding space)
>
> Yeah, IMHO it is easier to hook into varasm.c and just force the higher
> alignment and append padding after globals to be instrumented.
> The vars can be exported, we don't want to affect their .size, etc.
>

I think so too. Changing types and IR references will be too heavy
weight for it.



>> 4) Implement stack redzones:
>>
>> Collect all local variables that are still on stack (not on virtual registers).
>> Create a new local variable of size enough to hold all current locals
>> and their redzones.
>> Replace uses of locals with the new variable (Note that this does not
>> need to happen in IR, only add guard space in stack layout)
>> Poison the stack at function entry (one or two stores per local variable).
>> Unpoison the stack at function exits (maybe including all cleanup landing pads?)
>> Was:
>> int foo() {
>>    Type1 a;
>>    Type2 b;
>>    <code that uses 'a' and 'b'; they are not on virtual registers>
>>    return;
>> }
>> Now:
>> int foo() {
>>    struct {  // 32-byte aligned
>>      char redzone0[32];
>>      Type1 a;
>>      char redzone1[32 + required_alignment_a];
>>      Type2 b;
>>      char redzone2[32 + required_alignment_b];
>>    } locals;
>>
>>    int *shadow_base = ((&locals.redzone0)>>3)+kAsanOffset;
>>    shadow_base[0] = 0xffffffff;  // poison redzone0
>>    // poison the rest
>>
>>    <code that uses 'locals.a' and 'locals.b'>
>>
>>    shadow_base[0] = 0;  // unpoison redzone0
>>    // unpoison the rest
>>    return;
>> }
>
> We don't need to do this at the GIMPLE level, instead we can hook into
> cfgexpand.c var layout code.

Yes.

>  BTW, I wonder if for ASAN_SHADDOW_SHIFT 3
> we really need to force 32-byte alignment for the stack vars (as opposed
> to use the highest alignment among the protected vars).  32-byte alignment
> is fairly expensive, and if none of the vars actually need 32-byte
> alignment, I'd hope that 16-byte alignment should be enough.  The redzones
> would still be 32-byte etc.
>

I wonder about the 32 byte alignment requirement too.


thanks,

David
By doing this during expand we can keep say -fstack-protector to do its job
> too.
> LLVM seems to emit 0xf1, 0xf2, 0xf3 etc. bytes into shadow memory for
> various parts of the padding (left, middle, right).
>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Fri, Oct 12, 2012 at 11:51:22AM -0400, Diego Novillo wrote:
> On 2012-10-12 11:01 , Rainer Orth wrote:
> >Diego Novillo <dnovillo@google.com> writes:
> >
> >>3- To run ASAN's testsuite, I propose a simple wrapper script that executes
> >>it using the just-built gcc.  I don't think it's worth the pain to convert
> >>the testsuite into DejaGNU.
> >
> >If the testsuite is not converted (which can be ugly for maintainers
> >since it needs separate mechanisms for everything Dejagnu already deals
> >with, like timeouts, target-specific skipping), at the very least make
> 
> The thing is that if we convert the testsuite, then taking pristine
> tarballs is out of the question and we end up with a library like
> boehm-gc or zlib.  I would prefer to have something completely
> separate that we can just drop in.

I don't see how can their testcase be used if not converted to Dejagnu
though, most of their testcases are full of LLVM testcase markup
(// RUN, // CHECK*, etc.).  So, if we import the library unmodified,
we'll need to setup some directory under gcc/testsuite/ for asan tests
and arrange for *.exp to find the libraries.

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Diego Novillo]

On 2012-10-12 12:40 , Jakub Jelinek wrote:
> On Fri, Oct 12, 2012 at 11:51:22AM -0400, Diego Novillo wrote:
>> On 2012-10-12 11:01 , Rainer Orth wrote:
>>> Diego Novillo <dnovillo@google.com> writes:
>>>
>>>> 3- To run ASAN's testsuite, I propose a simple wrapper script that executes
>>>> it using the just-built gcc.  I don't think it's worth the pain to convert
>>>> the testsuite into DejaGNU.
>>>
>>> If the testsuite is not converted (which can be ugly for maintainers
>>> since it needs separate mechanisms for everything Dejagnu already deals
>>> with, like timeouts, target-specific skipping), at the very least make
>>
>> The thing is that if we convert the testsuite, then taking pristine
>> tarballs is out of the question and we end up with a library like
>> boehm-gc or zlib.  I would prefer to have something completely
>> separate that we can just drop in.
>
> I don't see how can their testcase be used if not converted to Dejagnu
> though, most of their testcases are full of LLVM testcase markup
> (// RUN, // CHECK*, etc.).  So, if we import the library unmodified,
> we'll need to setup some directory under gcc/testsuite/ for asan tests
> and arrange for *.exp to find the libraries.

Hm, perhaps that would be the way then.  Have the testsuite in 
gcc/testsuite/asan and only run it if ASAN was configured.  We could 
adapt the basic tests from libasan and over time add more into 
gcc/testsuite/asan.


Would that work?


Diego.

Re: [asan] Emit GIMPLE directly, small cleanups

[Ian Lance Taylor]

On Fri, Oct 12, 2012 at 9:40 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>
> I don't see how can their testcase be used if not converted to Dejagnu
> though, most of their testcases are full of LLVM testcase markup
> (// RUN, // CHECK*, etc.).  So, if we import the library unmodified,
> we'll need to setup some directory under gcc/testsuite/ for asan tests
> and arrange for *.exp to find the libraries.

Yes, that's more or less what the Go testsuite is like.  I just have a
complicated gcc/testsuite/go.test/go-test.exp that does the right
thing to handle the Go test cases as DejaGNU test cases.

Ian

Re: [asan] Emit GIMPLE directly, small cleanups

[Rainer Orth]

Ian Lance Taylor <iant@google.com> writes:

> On Fri, Oct 12, 2012 at 9:40 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> I don't see how can their testcase be used if not converted to Dejagnu
>> though, most of their testcases are full of LLVM testcase markup
>> (// RUN, // CHECK*, etc.).  So, if we import the library unmodified,
>> we'll need to setup some directory under gcc/testsuite/ for asan tests
>> and arrange for *.exp to find the libraries.
>
> Yes, that's more or less what the Go testsuite is like.  I just have a
> complicated gcc/testsuite/go.test/go-test.exp that does the right
> thing to handle the Go test cases as DejaGNU test cases.

I'd prefer if LLVM would accept the (sometimes more expressive and, to
GCC maintainers, well-known) Dejagnu annotations in addition to their
own ones, so the .exp files could live in the gcc tree only, while the
testcases themselves would be imported from upstream as is.

The same holds for gcc/testsuite/go.*, btw.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [asan] Emit GIMPLE directly, small cleanups

[Ian Lance Taylor]

On Mon, Oct 15, 2012 at 7:31 AM, Rainer Orth
<ro@cebitec.uni-bielefeld.de> wrote:
> Ian Lance Taylor <iant@google.com> writes:
>
>> On Fri, Oct 12, 2012 at 9:40 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> I don't see how can their testcase be used if not converted to Dejagnu
>>> though, most of their testcases are full of LLVM testcase markup
>>> (// RUN, // CHECK*, etc.).  So, if we import the library unmodified,
>>> we'll need to setup some directory under gcc/testsuite/ for asan tests
>>> and arrange for *.exp to find the libraries.
>>
>> Yes, that's more or less what the Go testsuite is like.  I just have a
>> complicated gcc/testsuite/go.test/go-test.exp that does the right
>> thing to handle the Go test cases as DejaGNU test cases.
>
> I'd prefer if LLVM would accept the (sometimes more expressive and, to
> GCC maintainers, well-known) Dejagnu annotations in addition to their
> own ones, so the .exp files could live in the gcc tree only, while the
> testcases themselves would be imported from upstream as is.
>
> The same holds for gcc/testsuite/go.*, btw.

In my opinion, supporting the full range of GCC testsuite annotations
means imposing a lot of mechanism that the Go testsuite does not
require.  It would complicate the Go testsuite for no benefit.
Anybody who can understand the GCC testsuite annotations can
understand the much simpler Go testsuite annotations.

Ian

Re: [asan] Emit GIMPLE directly, small cleanups

[Diego Novillo]

On Mon, Oct 15, 2012 at 11:55 AM, Ian Lance Taylor <iant@google.com> wrote:

> In my opinion, supporting the full range of GCC testsuite annotations
> means imposing a lot of mechanism that the Go testsuite does not
> require.  It would complicate the Go testsuite for no benefit.
> Anybody who can understand the GCC testsuite annotations can
> understand the much simpler Go testsuite annotations.

Agreed.  The fact that we have to suffer DejaGNU does not gives the
right to make other projects miserable.


Diego.

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

Another error checking feature is to poison stack vars on entry and
exit of the lexical scope to catch uninit variable reference and out
of scope references:

S* sp;
  {
    S s;
    sp = &s;
  }
  .. *sp ...

This is relatively easy to do in gcc thanks to the clobber statement.
In Clang/LLVM, it is in the wishlist:
http://code.google.com/p/address-sanitizer/issues/detail?id=83

Might be good consider this feature.

David

On Fri, Oct 12, 2012 at 9:30 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Oct 12, 2012 at 12:15 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Thu, Oct 11, 2012 at 04:19:18PM -0700, Wei Mi wrote:
>>> Here is the initial test results of gcc asan patch, and it shows us
>>> some missing features in gcc but existing in llvm.
>>> [1]. gcc regression test for gcc-asan passes.
>>> [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests
>>> written in google test and 24 failures in 28 for tests written in lit
>>> tests.
>>>
>>> gcc missing features:
>>
>> I think LLVM calls the option -faddress-sanitizer, perhaps we should rename
>> -fasan to that too for some compatibility.  For the varios knobs LLVM asan
>> has we could add params to params.def if we want to support them.
>>
>>> 1. gcc implementation doesn't support stack/global overflow check
>>
>> Yeah, I think the stack check shouldn't be that hard and can hack it up,
>> I'll perhaps leave the global vars stuff to Dodji or others if he has time.
>
> Since the stack part is relative self contained, might it be better
> for Wei (he is new) to tackle it with guidance from you?
>
> Global handling needs people more experienced with varasm stuff.
>
>>
>>> 1. gcc implementation doesn't support some attributes, such as
>>> __attribute__((no_address_safety_analysis)), which llvm does
>>
>> Yeah, shouldn't be hard.
>>
>
> yes -- that is simple.
>
>
>>> 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does
>>
>> Testcase (for everything testcases would be useful, and of course the
>> runtime library too)?  Yeah, the code currently punts on those, the question
>> is how to handle them.
>
> The code should sythensize the smallest containing
> byte/word/doubleword/qword of the bitfield and then compute the shadow
> address.
>
>>
>>> 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy
>>> for heap allocated memory or string, which llvm does
>>
>> That is easy (we can easily handle instrumenting lots of builtins).
>> Just a big switch on DECL_FUNCTION_CODE, collecting src/dst addresses and
>> length for each of them and adding the instrumentation for first and last
>> bytes in the strings.
>
> Yes.
>
>>
>>> 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm
>>> -asan-initialization-order
>>
>> I must say I don't like the -asan-blacklist option at all, IMHO it is much
>> saner to just ask users to use attributes (or pragmas around whole headers).
>
> But the option is easy to have too.
>
>>
>>> 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit
>>> tests contain checks from -O0 to -O3, which makes gcc fail.
>>
>> That is because of the place where the instrumentation is scheduled right
>> now in the pass queue.  We can easily add a pass_asan_O0 that will run say
>> close to pass_lower_complex_O0.
>
> Right.
>
> thanks,
>
> David
>>
>>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Rainer Orth]

Diego Novillo <dnovillo@google.com> writes:

> On Mon, Oct 15, 2012 at 11:55 AM, Ian Lance Taylor <iant@google.com> wrote:
>
>> In my opinion, supporting the full range of GCC testsuite annotations
>> means imposing a lot of mechanism that the Go testsuite does not
>> require.  It would complicate the Go testsuite for no benefit.
>> Anybody who can understand the GCC testsuite annotations can
>> understand the much simpler Go testsuite annotations.
>
> Agreed.  The fact that we have to suffer DejaGNU does not gives the
> right to make other projects miserable.

But importing different upstream testsuites with different annotation
systems allows them to make GCC maintainer's lives miserable?  If this
trend continues, maintainers need to cope with several different
annoation systems with different capabilites instead of a single one,
some of them lacking necessary features which are already present in
DejaGnu (which leads to handling stuff that's just a simple annotation
in DejaGnu in the testsuite drivers instead).  I'm not asking upstreams
to deal with DejaGnu themselves, just to accept that the dg annotations
live in their repos.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [asan] Emit GIMPLE directly, small cleanups

[Diego Novillo]

On 2012-10-16 07:05 , Rainer Orth wrote:
> Diego Novillo <dnovillo@google.com> writes:
>
>> On Mon, Oct 15, 2012 at 11:55 AM, Ian Lance Taylor <iant@google.com> wrote:
>>
>>> In my opinion, supporting the full range of GCC testsuite annotations
>>> means imposing a lot of mechanism that the Go testsuite does not
>>> require.  It would complicate the Go testsuite for no benefit.
>>> Anybody who can understand the GCC testsuite annotations can
>>> understand the much simpler Go testsuite annotations.
>>
>> Agreed.  The fact that we have to suffer DejaGNU does not gives the
>> right to make other projects miserable.
>
> But importing different upstream testsuites with different annotation
> systems allows them to make GCC maintainer's lives miserable?

Yes, absolutely.  We have to adapt to upstream's conventions.  If that 
means putting translation layers, the onus is on us.

Alternately, we could import libasan the same way we import things like 
zlib or boehm-gc and then re-write the testsuite.  That makes it harder 
to import newer versions, however.

Finally, we could simply duplicate libasan's testsuite inside 
gcc/testsuite/asan and add our own tests as well.  This may be the more 
practical choice.


Diego.

Re: [asan] Emit GIMPLE directly, small cleanups

[Ian Lance Taylor]

On Tue, Oct 16, 2012 at 4:05 AM, Rainer Orth
<ro@cebitec.uni-bielefeld.de> wrote:
>
> But importing different upstream testsuites with different annotation
> systems allows them to make GCC maintainer's lives miserable?  If this
> trend continues, maintainers need to cope with several different
> annoation systems with different capabilites instead of a single one,
> some of them lacking necessary features which are already present in
> DejaGnu (which leads to handling stuff that's just a simple annotation
> in DejaGnu in the testsuite drivers instead).

Yes, that is true.  But when you say "if this trend continues" you are
making a slippery slope argument that I don't think applies.  To date,
the trend consists of a single example.  We are discussing adding a
second example, and we may decide against it.  There are no current
prospects of a third example.

> I'm not asking upstreams
> to deal with DejaGnu themselves, just to accept that the dg annotations
> live in their repos.

Where it would be untested and unmaintained.  Do you think we would be
happy adding additional annotations to our testsuite for the benefit
of some other project?

Ian

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Mon, Oct 15, 2012 at 10:48:13PM -0700, Xinliang David Li wrote:
> Another error checking feature is to poison stack vars on entry and
> exit of the lexical scope to catch uninit variable reference and out
> of scope references:
> 
> S* sp;
>   {
>     S s;
>     sp = &s;
>   }
>   .. *sp ...
> 
> This is relatively easy to do in gcc thanks to the clobber statement.
> In Clang/LLVM, it is in the wishlist:
> http://code.google.com/p/address-sanitizer/issues/detail?id=83

That is not easy at all unfortunately, CLOBBER isn't sufficient for that.
You have the points where the variable looses value, but there aren't
similar markup statement where it gets into scope again.
See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54770#c3

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

On Mon, Oct 15, 2012 at 11:12 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 15, 2012 at 10:48:13PM -0700, Xinliang David Li wrote:
>> Another error checking feature is to poison stack vars on entry and
>> exit of the lexical scope to catch uninit variable reference and out
>> of scope references:
>>
>> S* sp;
>>   {
>>     S s;
>>     sp = &s;
>>   }
>>   .. *sp ...
>>
>> This is relatively easy to do in gcc thanks to the clobber statement.
>> In Clang/LLVM, it is in the wishlist:
>> http://code.google.com/p/address-sanitizer/issues/detail?id=83
>
> That is not easy at all unfortunately, CLOBBER isn't sufficient for that.
> You have the points where the variable looses value, but there aren't
> similar markup statement where it gets into scope again.
> See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54770#c3

I don't get it.  Clobber marks the end of lifetime of a variable so it
is safe to emit code to really clobber its value -- otherwise how
would clobber based slot sharing work?

David

>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Tue, Oct 16, 2012 at 09:06:22AM -0700, Xinliang David Li wrote:
> I don't get it.  Clobber marks the end of lifetime of a variable so it
> is safe to emit code to really clobber its value -- otherwise how
> would clobber based slot sharing work?

If you at CLOBBER protect the var in the shadow mem as unaccessible, the
thing is where you make it accessible again.
Consider:

int i;
for (i = 0; i < 2; i++)
  {
    int tmp;
    bar ();
    baz (&tmp);
    bar ();
  }

where the first bar () can't possibly access tmp, but the second bar () can.
Now we unroll it and have:
  bar ();
  baz (&tmp);
  bar ();
  tmp ={v} {CLOBBER};
  bar ();
  baz (&tmp);
  bar ();
  tmp ={v} {CLOBBER};
So, if you want to *mem_to_shadow (&tmp) = 0xff; at {CLOBBER} time, where
and how you would insert *mem_to_shadow (&tmp) = 0; again?  In the above
code it would need to go before the second baz (&tmp);, the question is
how you'd find that out...

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

The way to do this is not to use the shadow memory, but to clobber
directly the variable itself with specified byte pattern -- though
error diagnostics won't be as nice.

To use shadow memory, the scope start marker is also needed.

David

On Tue, Oct 16, 2012 at 9:12 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 16, 2012 at 09:06:22AM -0700, Xinliang David Li wrote:
>> I don't get it.  Clobber marks the end of lifetime of a variable so it
>> is safe to emit code to really clobber its value -- otherwise how
>> would clobber based slot sharing work?
>
> If you at CLOBBER protect the var in the shadow mem as unaccessible, the
> thing is where you make it accessible again.
> Consider:
>
> int i;
> for (i = 0; i < 2; i++)
>   {
>     int tmp;
>     bar ();
>     baz (&tmp);
>     bar ();
>   }
>
> where the first bar () can't possibly access tmp, but the second bar () can.
> Now we unroll it and have:
>   bar ();
>   baz (&tmp);
>   bar ();
>   tmp ={v} {CLOBBER};
>   bar ();
>   baz (&tmp);
>   bar ();
>   tmp ={v} {CLOBBER};
> So, if you want to *mem_to_shadow (&tmp) = 0xff; at {CLOBBER} time, where
> and how you would insert *mem_to_shadow (&tmp) = 0; again?  In the above
> code it would need to go before the second baz (&tmp);, the question is
> how you'd find that out...
>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Jakub Jelinek]

On Tue, Oct 16, 2012 at 09:48:28AM -0700, Xinliang David Li wrote:
> The way to do this is not to use the shadow memory, but to clobber
> directly the variable itself with specified byte pattern -- though
> error diagnostics won't be as nice.

Clobbering the memory directly is definitely easy, but doesn't have anything
to do how shadow memory poisioning is done for stack vars.
Guess should be done depending on some special option, not unconditionally
implied by -fasan though.

	Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Xinliang David Li]

On Tue, Oct 16, 2012 at 10:40 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 16, 2012 at 09:48:28AM -0700, Xinliang David Li wrote:
>> The way to do this is not to use the shadow memory, but to clobber
>> directly the variable itself with specified byte pattern -- though
>> error diagnostics won't be as nice.
>
> Clobbering the memory directly is definitely easy, but doesn't have anything
> to do how shadow memory poisioning is done for stack vars.
> Guess should be done depending on some special option, not unconditionally
> implied by -fasan though.

yes -- agree.  Longer term this functionality should be using the
shadow memory scheme though.

David

>
>         Jakub

Re: [asan] Emit GIMPLE directly, small cleanups

[Eric Botcazou]

> Yes, that is true.  But when you say "if this trend continues" you are
> making a slippery slope argument that I don't think applies.  To date,
> the trend consists of a single example.  We are discussing adding a
> second example, and we may decide against it.  There are no current
> prospects of a third example.

You can hardly counter a slippery slope argument by saying "we're adding one, 
but, for the time being, we won't add more" since that's precisely what is 
denounced: you will say it for every new addition and this will never end.

IMHO we need to decide what to do in this case once for all.

-- 
Eric Botcazou

Re: [asan] Emit GIMPLE directly, small cleanups

[Ian Lance Taylor]

On Tue, Oct 16, 2012 at 2:33 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yes, that is true.  But when you say "if this trend continues" you are
>> making a slippery slope argument that I don't think applies.  To date,
>> the trend consists of a single example.  We are discussing adding a
>> second example, and we may decide against it.  There are no current
>> prospects of a third example.
>
> You can hardly counter a slippery slope argument by saying "we're adding one,
> but, for the time being, we won't add more" since that's precisely what is
> denounced: you will say it for every new addition and this will never end.

The slippery slope argument is that this will become easier and easier
over time, and that the earlier examples will serve to pave the way
for the later examples.  I don't see any reason to assume that is
true.  There is no slope here.

> IMHO we need to decide what to do in this case once for all.

That is fine with me as long as we acknowledge that the upstream
sources don't care about GCC and will think it is absurd that they
should modify their code to carry untested and unmaintained
GCC-specific annotations.  It would be one thing if the GCC-specific
annotations were clearly better, but in fact I would say that they are
clearly worse.

Ian

Re: [asan] Emit GIMPLE directly, small cleanups

[Diego Novillo]

On 2012-10-16 18:50 , Ian Lance Taylor wrote:

> That is fine with me as long as we acknowledge that the upstream
> sources don't care about GCC and will think it is absurd that they
> should modify their code to carry untested and unmaintained
> GCC-specific annotations.  It would be one thing if the GCC-specific
> annotations were clearly better, but in fact I would say that they are
> clearly worse.

Precisely.

Eric, Rainer, what do you think of the other two options I outlined in 
my earlier message?

1- Copy the upstream testsuite into gcc/testsuite/asan.  This gives us 
the flexibility of adding new tests as the GCC implementation matures.

2- Deal with libasan as we deal with zlib/boehm-gc.

I prefer option #1, personally.


Diego.

Re: [asan] Emit GIMPLE directly, small cleanups

[Eric Botcazou]

> Eric, Rainer, what do you think of the other two options I outlined in
> my earlier message?
> 
> 1- Copy the upstream testsuite into gcc/testsuite/asan.  This gives us
> the flexibility of adding new tests as the GCC implementation matures.
> 
> 2- Deal with libasan as we deal with zlib/boehm-gc.
> 
> I prefer option #1, personally.

I feel a bit umconfortable saying this given the gcc/testsuite/ada/acats 
precedent, but I don't think we should import foreing testsuites into 
gcc/testsuite, unless they are adapted to our harness.  I don't think we
want to run again into the issues we had with acats (Rainer puts a lot of 
efforts to clean up the mess here).

-- 
Eric Botcazou