[asan] Fix some asan ICEs

[asan] Fix some asan ICEs

[Jakub Jelinek]

Hi!

Dodji reported to me an ICE about NOTE_INSN_BASIC_BLOCK in a middle
of a bb.  The following patch (cfgexpand.c hunk) fixes that.
I then run asan cc1/cc1plus with -O2 -fasan on a portion of cc1files
and got other ICEs, which is the reason for the two asan.c changes
- one was that base wasn't unshared, so we could hit tree sharing
verification failures, and the other issue is that cgraph_build_static_cdtor
can call ggc_collect, so holding trees around in automatic variables
across it doesn't work very well.  I'd have to build too many trees
again, so I think easiest is to add a new GTY root.

Ok for asan?

2012-10-18  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (build_check_stmt): Unshare base.

	* asan.c (asan_ctor_statements): New variable.
	(asan_finish_file): Use asan_ctor_statements instead
	of ctor_statements.

	* cfgexpand.c (gimple_expand_cfg): If return_label is
	followed by NOTE_INSN_BASIC_BLOCK, emit var_ret_seq
	after the note instead of before it.

--- gcc/asan.c.jj	2012-10-17 20:43:00.000000000 +0200
+++ gcc/asan.c	2012-10-18 14:20:23.837423900 +0200
@@ -459,6 +459,8 @@ build_check_stmt (tree base,
       set_immediate_dominator (CDI_DOMINATORS, else_bb, cond_bb);
     }
 
+  base = unshare_expr (base);
+
   gsi = gsi_last_bb (cond_bb);
   g = gimple_build_assign_with_ops (TREE_CODE (base),
 				    make_ssa_name (TREE_TYPE (base), NULL),
@@ -748,6 +750,10 @@ asan_add_global (tree decl, tree type, V
   CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, init);
 }
 
+/* Needs to be GTY(()), because cgraph_build_static_cdtor may
+   invoke ggc_collect.  */
+static GTY(()) tree asan_ctor_statements;
+
 /* Module-level instrumentation.
    - Insert __asan_init() into the list of CTORs.
    - TODO: insert redzones around globals.
@@ -756,12 +762,11 @@ asan_add_global (tree decl, tree type, V
 void
 asan_finish_file (void)
 {
-  tree ctor_statements = NULL_TREE;
   struct varpool_node *vnode;
   unsigned HOST_WIDE_INT gcount = 0;
 
   append_to_statement_list (build_call_expr (asan_init_func (), 0),
-			    &ctor_statements);
+			    &asan_ctor_statements);
   FOR_EACH_DEFINED_VARIABLE (vnode)
     if (asan_protect_global (vnode->symbol.decl))
       ++gcount;
@@ -799,7 +805,7 @@ asan_finish_file (void)
       append_to_statement_list (build_call_expr (decl, 2,
 						 build_fold_addr_expr (var),
 						 build_int_cst (uptr, gcount)),
-				&ctor_statements);
+				&asan_ctor_statements);
 
       decl = build_fn_decl ("__asan_unregister_globals", type);
       TREE_NOTHROW (decl) = 1;
@@ -810,7 +816,7 @@ asan_finish_file (void)
       cgraph_build_static_cdtor ('D', dtor_statements,
 				 MAX_RESERVED_INIT_PRIORITY - 1);
     }
-  cgraph_build_static_cdtor ('I', ctor_statements,
+  cgraph_build_static_cdtor ('I', asan_ctor_statements,
 			     MAX_RESERVED_INIT_PRIORITY - 1);
 }
 
--- gcc/cfgexpand.c.jj	2012-10-17 20:41:02.000000000 +0200
+++ gcc/cfgexpand.c	2012-10-18 13:01:06.084479343 +0200
@@ -4615,7 +4615,12 @@ gimple_expand_cfg (void)
   insn_locations_finalize ();
 
   if (var_ret_seq)
-    emit_insn_after (var_ret_seq, return_label);
+    {
+      rtx after = return_label;
+      if (NEXT_INSN (after) && NOTE_INSN_BASIC_BLOCK_P (NEXT_INSN (after)))
+	after = NEXT_INSN (after);
+      emit_insn_after (var_ret_seq, after);
+    }
 
   /* Zap the tree EH table.  */
   set_eh_throw_stmt_table (cfun, NULL);

	Jakub

Re: [asan] Fix some asan ICEs

[Xinliang David Li]

On Thu, Oct 18, 2012 at 5:58 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Dodji reported to me an ICE about NOTE_INSN_BASIC_BLOCK in a middle
> of a bb.  The following patch (cfgexpand.c hunk) fixes that.
> I then run asan cc1/cc1plus with -O2 -fasan on a portion of cc1files
> and got other ICEs, which is the reason for the two asan.c changes
> - one was that base wasn't unshared, so we could hit tree sharing
> verification failures, and the other issue is that cgraph_build_static_cdtor
> can call ggc_collect, so holding trees around in automatic variables
> across it doesn't work very well.  I'd have to build too many trees
> again, so I think easiest is to add a new GTY root.
>
> Ok for asan?
>
> 2012-10-18  Jakub Jelinek  <jakub@redhat.com>
>
>         * asan.c (build_check_stmt): Unshare base.
>
>         * asan.c (asan_ctor_statements): New variable.
>         (asan_finish_file): Use asan_ctor_statements instead
>         of ctor_statements.
>
>         * cfgexpand.c (gimple_expand_cfg): If return_label is
>         followed by NOTE_INSN_BASIC_BLOCK, emit var_ret_seq
>         after the note instead of before it.
>
> --- gcc/asan.c.jj       2012-10-17 20:43:00.000000000 +0200
> +++ gcc/asan.c  2012-10-18 14:20:23.837423900 +0200
> @@ -459,6 +459,8 @@ build_check_stmt (tree base,
>        set_immediate_dominator (CDI_DOMINATORS, else_bb, cond_bb);
>      }
>
> +  base = unshare_expr (base);
> +
>    gsi = gsi_last_bb (cond_bb);
>    g = gimple_build_assign_with_ops (TREE_CODE (base),
>                                     make_ssa_name (TREE_TYPE (base), NULL),
> @@ -748,6 +750,10 @@ asan_add_global (tree decl, tree type, V
>    CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, init);
>  }
>
> +/* Needs to be GTY(()), because cgraph_build_static_cdtor may
> +   invoke ggc_collect.  */
> +static GTY(()) tree asan_ctor_statements;
> +
>  /* Module-level instrumentation.
>     - Insert __asan_init() into the list of CTORs.
>     - TODO: insert redzones around globals.
> @@ -756,12 +762,11 @@ asan_add_global (tree decl, tree type, V
>  void
>  asan_finish_file (void)
>  {
> -  tree ctor_statements = NULL_TREE;
>    struct varpool_node *vnode;
>    unsigned HOST_WIDE_INT gcount = 0;
>
>    append_to_statement_list (build_call_expr (asan_init_func (), 0),
> -                           &ctor_statements);
> +                           &asan_ctor_statements);
>    FOR_EACH_DEFINED_VARIABLE (vnode)
>      if (asan_protect_global (vnode->symbol.decl))
>        ++gcount;
> @@ -799,7 +805,7 @@ asan_finish_file (void)
>        append_to_statement_list (build_call_expr (decl, 2,
>                                                  build_fold_addr_expr (var),
>                                                  build_int_cst (uptr, gcount)),
> -                               &ctor_statements);
> +                               &asan_ctor_statements);
>
>        decl = build_fn_decl ("__asan_unregister_globals", type);
>        TREE_NOTHROW (decl) = 1;
> @@ -810,7 +816,7 @@ asan_finish_file (void)
>        cgraph_build_static_cdtor ('D', dtor_statements,
>                                  MAX_RESERVED_INIT_PRIORITY - 1);
>      }
> -  cgraph_build_static_cdtor ('I', ctor_statements,
> +  cgraph_build_static_cdtor ('I', asan_ctor_statements,
>                              MAX_RESERVED_INIT_PRIORITY - 1);
>  }
>
> --- gcc/cfgexpand.c.jj  2012-10-17 20:41:02.000000000 +0200
> +++ gcc/cfgexpand.c     2012-10-18 13:01:06.084479343 +0200
> @@ -4615,7 +4615,12 @@ gimple_expand_cfg (void)
>    insn_locations_finalize ();
>
>    if (var_ret_seq)
> -    emit_insn_after (var_ret_seq, return_label);
> +    {
> +      rtx after = return_label;
> +      if (NEXT_INSN (after) && NOTE_INSN_BASIC_BLOCK_P (NEXT_INSN (after)))
> +       after = NEXT_INSN (after);


Just a nit: NEXT_INSN (after) can be commoned.

Other than that, looks fine to me.

thanks,

David

> +      emit_insn_after (var_ret_seq, after);
> +    }
>
>    /* Zap the tree EH table.  */
>    set_eh_throw_stmt_table (cfun, NULL);
>
>         Jakub

Re: [asan] Fix some asan ICEs

[Dodji Seketeli]

> Ok for trunk?
>
> 2012-11-27  Jakub Jelinek  <jakub@redhat.com>
>
> 	* asan.c (instrument_mem_region_access): Don't instrument
> 	if base doesn't have pointer type or len integral type.
> 	Add cast if len doesn't have size_t compatible type.
> 	(instrument_builtin_call): Don't instrument BUILT_IN_ATOMIC_LOAD,
> 	BUILT_IN_ATOMIC_TEST_AND_SET, BUILT_IN_ATOMIC_CLEAR,
> 	BUILT_IN_ATOMIC_EXCHANGE, BUILT_IN_ATOMIC_COMPARE_EXCHANGE
> 	and BUILT_IN_ATOMIC_STORE.

This is OK.  Thanks, and sorry for the delay.

-- 
		Dodji

[asan] Fix some asan ICEs

[Jakub Jelinek]

Hi!

This fixes a couple of asan ICEs found while running make check
with RUNTESTFLAGS='unix/-fsanitize=address'.
The last two hunks fix ICEs while instrumenting atomics with non-standard
sizes, those are always turned into library calls, and the first argument
is the length, not a pointer.
The other issues fixed are if one uses non-prototyped builtins with wrong
types of arguments, it can result in verify_gimple ICEs (e.g. if last
argument to non-prototyped memcmp is int), or worse (say if instead of
pointer it is a double).  The patch just punts for bogus builtins,
and when len is integral, but not of the right type, it casts it to the
right type (for constant just builds the right offset as constant,
otherwise adds extra stmts as needed).

Ok for trunk?

2012-11-27  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (instrument_mem_region_access): Don't instrument
	if base doesn't have pointer type or len integral type.
	Add cast if len doesn't have size_t compatible type.
	(instrument_builtin_call): Don't instrument BUILT_IN_ATOMIC_LOAD,
	BUILT_IN_ATOMIC_TEST_AND_SET, BUILT_IN_ATOMIC_CLEAR,
	BUILT_IN_ATOMIC_EXCHANGE, BUILT_IN_ATOMIC_COMPARE_EXCHANGE
	and BUILT_IN_ATOMIC_STORE.

--- gcc/asan.c.jj	2012-11-23 15:21:49.000000000 +0100
+++ gcc/asan.c	2012-11-27 17:37:10.948281831 +0100
@@ -849,7 +849,9 @@ instrument_mem_region_access (tree base,
 			      gimple_stmt_iterator *iter,
 			      location_t location, bool is_store)
 {
-  if (integer_zerop (len))
+  if (!POINTER_TYPE_P (TREE_TYPE (base))
+      || !INTEGRAL_TYPE_P (TREE_TYPE (len))
+      || integer_zerop (len))
     return;
 
   gimple_stmt_iterator gsi = *iter;
@@ -902,20 +904,41 @@ instrument_mem_region_access (tree base,
 
   /* offset = len - 1;  */
   len = unshare_expr (len);
-  gimple offset =
-    gimple_build_assign_with_ops (TREE_CODE (len),
-				  make_ssa_name (TREE_TYPE (len), NULL),
-				  len, NULL);
-  gimple_set_location (offset, location);
-  gsi_insert_before (&gsi, offset, GSI_NEW_STMT);
-
-  offset =
-    gimple_build_assign_with_ops (MINUS_EXPR,
-				  make_ssa_name (size_type_node, NULL),
-				  gimple_assign_lhs (offset),
-				  build_int_cst (size_type_node, 1));
-  gimple_set_location (offset, location);
-  gsi_insert_after (&gsi, offset, GSI_NEW_STMT);
+  tree offset;
+  gimple_seq seq = NULL;
+  if (TREE_CODE (len) == INTEGER_CST)
+    offset = fold_build2 (MINUS_EXPR, size_type_node,
+			  fold_convert (size_type_node, len),
+			  build_int_cst (size_type_node, 1));
+  else
+    {
+      gimple g;
+      tree t;
+
+      if (TREE_CODE (len) != SSA_NAME)
+	{
+	  t = make_ssa_name (TREE_TYPE (len), NULL);
+	  g = gimple_build_assign_with_ops (TREE_CODE (len), t, len, NULL);
+	  gimple_set_location (g, location);
+	  gimple_seq_add_stmt_without_update (&seq, g);
+	  len = t;
+	}
+      if (!useless_type_conversion_p (size_type_node, TREE_TYPE (len)))
+	{
+	  t = make_ssa_name (size_type_node, NULL);
+	  g = gimple_build_assign_with_ops (NOP_EXPR, t, len, NULL);
+	  gimple_set_location (g, location);
+	  gimple_seq_add_stmt_without_update (&seq, g);
+	  len = t;
+	}
+
+      t = make_ssa_name (size_type_node, NULL);
+      g = gimple_build_assign_with_ops (MINUS_EXPR, t, len,
+					build_int_cst (size_type_node, 1));
+      gimple_set_location (g, location);
+      gimple_seq_add_stmt_without_update (&seq, g);
+      offset = gimple_assign_lhs (g);
+    }
 
   /* _1 = base;  */
   base = unshare_expr (base);
@@ -924,14 +947,16 @@ instrument_mem_region_access (tree base,
 				  make_ssa_name (TREE_TYPE (base), NULL),
 				  base, NULL);
   gimple_set_location (region_end, location);
-  gsi_insert_after (&gsi, region_end, GSI_NEW_STMT);
+  gimple_seq_add_stmt_without_update (&seq, region_end);
+  gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
+  gsi_prev (&gsi);
 
   /* _2 = _1 + offset;  */
   region_end =
     gimple_build_assign_with_ops (POINTER_PLUS_EXPR,
 				  make_ssa_name (TREE_TYPE (base), NULL),
 				  gimple_assign_lhs (region_end),
-				  gimple_assign_lhs (offset));
+				  offset);
   gimple_set_location (region_end, location);
   gsi_insert_after (&gsi, region_end, GSI_NEW_STMT);
 
@@ -1089,7 +1114,6 @@ instrument_builtin_call (gimple_stmt_ite
        These are handled differently from the classical memory memory
        access builtins above.  */
 
-    case BUILT_IN_ATOMIC_LOAD:
     case BUILT_IN_ATOMIC_LOAD_1:
     case BUILT_IN_ATOMIC_LOAD_2:
     case BUILT_IN_ATOMIC_LOAD_4:
@@ -1192,23 +1216,18 @@ instrument_builtin_call (gimple_stmt_ite
     case BUILT_IN_SYNC_LOCK_RELEASE_8:
     case BUILT_IN_SYNC_LOCK_RELEASE_16:
 
-    case BUILT_IN_ATOMIC_TEST_AND_SET:
-    case BUILT_IN_ATOMIC_CLEAR:
-    case BUILT_IN_ATOMIC_EXCHANGE:
     case BUILT_IN_ATOMIC_EXCHANGE_1:
     case BUILT_IN_ATOMIC_EXCHANGE_2:
     case BUILT_IN_ATOMIC_EXCHANGE_4:
     case BUILT_IN_ATOMIC_EXCHANGE_8:
     case BUILT_IN_ATOMIC_EXCHANGE_16:
 
-    case BUILT_IN_ATOMIC_COMPARE_EXCHANGE:
     case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_1:
     case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_2:
     case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_4:
     case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_8:
     case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_16:
 
-    case BUILT_IN_ATOMIC_STORE:
     case BUILT_IN_ATOMIC_STORE_1:
     case BUILT_IN_ATOMIC_STORE_2:
     case BUILT_IN_ATOMIC_STORE_4:

	Jakub