Re: [PATCH] Speed-up use-after-scope (re-writing to SSA) (version 2)

[Martin Liška <mliska at suse dot cz>]

On 01/19/2017 05:33 PM, Jakub Jelinek wrote:
> On Wed, Jan 18, 2017 at 04:34:48PM +0100, Martin Liška wrote:
>> Hello.
>>
>> During bootstrap, I came to following test-case:
>>
>> struct A
>> {
>>   int regno;
>> };
>> struct
>> {
>>   A base;
>> } typedef *df_ref;
>> int *a;
>> void
>> fn1 (int N)
>> {
>>   for (int i = 0; i < N; i++)
>>     {
>>       df_ref b;
>>       a[(b)->base.regno]++;
>>     }
>> }
> 
> Well, in this case it is UB too, just not actually out of bounds access,
> but use of uninitialized variable.
> Perhaps what we should do, in addition to turning ASAN_MARK (POISON, &b, ...)
> into b = ASAN_POISON (); turn ASAN_MARK (UNPOISON, &b, ...) into
> b = b_YYY(D);

Great, thanks a lot. I'm going to re-trigger asan-bootstrap with your patch.
I'm also adding gcc/testsuite/gcc.dg/asan/use-after-scope-10.c that is a valid
test-case for this issue.

Hopefully it will survive both regression tests and asan-bootstrap.

Thanks,
Martin


> The following seems to do the job:
> --- gcc/tree-ssa.c.jj	2017-01-19 17:20:15.000000000 +0100
> +++ gcc/tree-ssa.c	2017-01-19 17:29:58.015356370 +0100
> @@ -1911,7 +1911,16 @@ execute_update_addresses_taken (void)
>  			    gsi_replace (&gsi, call, GSI_SAME_STMT);
>  			  }
>  			else
> -			  gsi_remove (&gsi, true);
> +			  {
> +			    /* In ASAN_MARK (UNPOISON, &b, ...) the variable
> +			       is uninitialized.  Avoid dependencies on
> +			       previous out of scope value.  */
> +			    tree clobber
> +			      = build_constructor (TREE_TYPE (var), NULL);
> +			    TREE_THIS_VOLATILE (clobber) = 1;
> +			    gimple *g = gimple_build_assign (var, clobber);
> +			    gsi_replace (&gsi, g, GSI_SAME_STMT);
> +			  }
>  			continue;
>  		      }
>  		  }
> 
> 	Jakub
> 

>From fa8a7fa81df7cf775dcf9018911044e5a331570d Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 17 Jan 2017 16:49:29 +0100
Subject: [PATCH] use-after-scope: handle writes to a poisoned variable

gcc/testsuite/ChangeLog:

2017-01-17  Martin Liska  <mliska@suse.cz>

	* gcc.dg/asan/use-after-scope-10.c: New test.
	* g++.dg/asan/use-after-scope-5.C: New test.

gcc/ChangeLog:

2017-01-16  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (asan_expand_poison_ifn): Support stores and use
	appropriate ASAN report function.
	* internal-fn.c (expand_ASAN_POISON_USE): New function.
	* internal-fn.def (ASAN_POISON_USE): Declare.
	* tree-into-ssa.c (maybe_add_asan_poison_write): New function.
	(maybe_register_def): Create ASAN_POISON_USE when sanitizing.
	* tree-ssa-dce.c (eliminate_unnecessary_stmts): Remove
	ASAN_POISON calls w/o LHS.
	* tree-ssa.c (execute_update_addresses_taken): Create clobber
	for ASAN_MARK (UNPOISON, &x, ...) in order to prevent usage of a LHS
	from ASAN_MARK (POISON, &x, ...) coming to a PHI node.
---
 gcc/asan.c                                     | 19 +++++++++++-------
 gcc/internal-fn.c                              |  8 ++++++++
 gcc/internal-fn.def                            |  1 +
 gcc/testsuite/g++.dg/asan/use-after-scope-5.C  | 23 ++++++++++++++++++++++
 gcc/testsuite/gcc.dg/asan/use-after-scope-10.c | 22 +++++++++++++++++++++
 gcc/tree-into-ssa.c                            | 27 +++++++++++++++++++++++++-
 gcc/tree-ssa-dce.c                             | 16 +++++++++++----
 gcc/tree-ssa.c                                 | 11 ++++++++++-
 8 files changed, 114 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/asan/use-after-scope-5.C
 create mode 100644 gcc/testsuite/gcc.dg/asan/use-after-scope-10.c

diff --git a/gcc/asan.c b/gcc/asan.c
index fe117a6951a..486ebfdb6af 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -3094,6 +3094,8 @@ create_asan_shadow_var (tree var_decl,
     return *slot;
 }
 
+/* Expand ASAN_POISON ifn.  */
+
 bool
 asan_expand_poison_ifn (gimple_stmt_iterator *iter,
 			bool *need_commit_edge_insert,
@@ -3107,8 +3109,8 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter,
       return true;
     }
 
-  tree shadow_var  = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var),
-					     shadow_vars_mapping);
+  tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var),
+					    shadow_vars_mapping);
 
   bool recover_p;
   if (flag_sanitize & SANITIZE_USER_ADDRESS)
@@ -3122,16 +3124,16 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter,
 						 ASAN_MARK_POISON),
 				  build_fold_addr_expr (shadow_var), size);
 
-  use_operand_p use_p;
+  gimple *use;
   imm_use_iterator imm_iter;
-  FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var)
+  FOR_EACH_IMM_USE_STMT (use, imm_iter, poisoned_var)
     {
-      gimple *use = USE_STMT (use_p);
       if (is_gimple_debug (use))
 	continue;
 
       int nargs;
-      tree fun = report_error_func (false, recover_p, tree_to_uhwi (size),
+      bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE);
+      tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size),
 				    &nargs);
 
       gcall *call = gimple_build_call (fun, 1,
@@ -3160,7 +3162,10 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter,
       else
 	{
 	  gimple_stmt_iterator gsi = gsi_for_stmt (use);
-	  gsi_insert_before (&gsi, call, GSI_NEW_STMT);
+	  if (store_p)
+	    gsi_replace (&gsi, call, true);
+	  else
+	    gsi_insert_before (&gsi, call, GSI_NEW_STMT);
 	}
     }
 
diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
index 71be382ab8b..a4a2995f58b 100644
--- a/gcc/internal-fn.c
+++ b/gcc/internal-fn.c
@@ -388,6 +388,14 @@ expand_ASAN_POISON (internal_fn, gcall *)
   gcc_unreachable ();
 }
 
+/* This should get expanded in the sanopt pass.  */
+
+static void
+expand_ASAN_POISON_USE (internal_fn, gcall *)
+{
+  gcc_unreachable ();
+}
+
 /* This should get expanded in the tsan pass.  */
 
 static void
diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
index 7b28b6722ff..fd25a952299 100644
--- a/gcc/internal-fn.def
+++ b/gcc/internal-fn.def
@@ -168,6 +168,7 @@ DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".R...")
 DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..")
 DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
+DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
diff --git a/gcc/testsuite/g++.dg/asan/use-after-scope-5.C b/gcc/testsuite/g++.dg/asan/use-after-scope-5.C
new file mode 100644
index 00000000000..7e28fc35e6d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/use-after-scope-5.C
@@ -0,0 +1,23 @@
+// { dg-do run }
+
+int *
+__attribute__((optimize(("-O0"))))
+fn1 (int *a)
+{
+  return a;
+}
+
+void
+fn2 ()
+{
+  for (int i = 0; i < 10; i++)
+    {
+      int *a;
+      (a) = fn1 (a);
+    }
+}
+
+int main()
+{
+  fn2();
+}
diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c
new file mode 100644
index 00000000000..24de8cec1ff
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c
@@ -0,0 +1,22 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+// { dg-additional-options "-O2 -fdump-tree-asan1" }
+
+int
+main (int argc, char **argv)
+{
+  int *ptr = 0;
+
+  {
+    int a;
+    ptr = &a;
+    *ptr = 12345;
+  }
+
+  *ptr = 12345;
+  return *ptr;
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-use-after-scope on address.*(\n|\r\n|\r)" }
+// { dg-output "WRITE of size .*" }
+// { dg-output ".*'a' <== Memory access at offset \[0-9\]* is inside this variable.*" }
diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
index c7df237d57f..22261c15dc2 100644
--- a/gcc/tree-into-ssa.c
+++ b/gcc/tree-into-ssa.c
@@ -38,6 +38,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa.h"
 #include "domwalk.h"
 #include "statistics.h"
+#include "asan.h"
 
 #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
 
@@ -1807,6 +1808,26 @@ maybe_replace_use_in_debug_stmt (use_operand_p use_p)
 }
 
 
+/* If DEF has x_5 = ASAN_POISON () as its current def, add
+   ASAN_POISON_USE (x_5) stmt before GSI to denote the stmt writes into
+   a poisoned (out of scope) variable.  */
+
+static void
+maybe_add_asan_poison_write (tree def, gimple_stmt_iterator *gsi)
+{
+  tree cdef = get_current_def (def);
+  if (cdef != NULL
+      && TREE_CODE (cdef) == SSA_NAME
+      && gimple_call_internal_p (SSA_NAME_DEF_STMT (cdef), IFN_ASAN_POISON))
+    {
+      gcall *call
+	= gimple_build_call_internal (IFN_ASAN_POISON_USE, 1, cdef);
+      gimple_set_location (call, gimple_location (gsi_stmt (*gsi)));
+      gsi_insert_before (gsi, call, GSI_SAME_STMT);
+    }
+}
+
+
 /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES
    or OLD_SSA_NAMES, or if it is a symbol marked for renaming,
    register it as the current definition for the names replaced by
@@ -1837,7 +1858,11 @@ maybe_register_def (def_operand_p def_p, gimple *stmt,
 	      def = get_or_create_ssa_default_def (cfun, sym);
 	    }
 	  else
-	    def = make_ssa_name (def, stmt);
+	    {
+	      if (asan_sanitize_use_after_scope ())
+		maybe_add_asan_poison_write (def, &gsi);
+	      def = make_ssa_name (def, stmt);
+	    }
 	  SET_DEF (def_p, def);
 
 	  tree tracked_var = target_for_debug_bind (sym);
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index 4e51e699d49..5ebe57b0983 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -1367,10 +1367,18 @@ eliminate_unnecessary_stmts (void)
 		  update_stmt (stmt);
 		  release_ssa_name (name);
 
-		  /* GOMP_SIMD_LANE without lhs is not needed.  */
-		  if (gimple_call_internal_p (stmt)
-		      && gimple_call_internal_fn (stmt) == IFN_GOMP_SIMD_LANE)
-		    remove_dead_stmt (&gsi, bb);
+		  /* GOMP_SIMD_LANE or ASAN_POISON without lhs is not
+		     needed.  */
+		  if (gimple_call_internal_p (stmt))
+		    switch (gimple_call_internal_fn (stmt))
+		      {
+		      case IFN_GOMP_SIMD_LANE:
+		      case IFN_ASAN_POISON:
+			remove_dead_stmt (&gsi, bb);
+			break;
+		      default:
+			break;
+		      }
 		}
 	      else if (gimple_call_internal_p (stmt))
 		switch (gimple_call_internal_fn (stmt))
diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c
index cc920950bab..5bd9004e715 100644
--- a/gcc/tree-ssa.c
+++ b/gcc/tree-ssa.c
@@ -1886,7 +1886,16 @@ execute_update_addresses_taken (void)
 			    gsi_replace (&gsi, call, GSI_SAME_STMT);
 			  }
 			else
-			  gsi_remove (&gsi, true);
+			  {
+			    /* In ASAN_MARK (UNPOISON, &b, ...) the variable
+			       is uninitialized.  Avoid dependencies on
+			       previous out of scope value.  */
+			    tree clobber
+			      = build_constructor (TREE_TYPE (var), NULL);
+			    TREE_THIS_VOLATILE (clobber) = 1;
+			    gimple *g = gimple_build_assign (var, clobber);
+			    gsi_replace (&gsi, g, GSI_SAME_STMT);
+			  }
 			continue;
 		      }
 		  }
-- 
2.11.0