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

["Martin Liška" <mliska@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 7525 bytes --]

On 01/16/2017 03:20 PM, Jakub Jelinek wrote:
> On Mon, Jan 09, 2017 at 03:58:04PM +0100, Martin Liška wrote:
>>>> Well, having following sample:
>>>>
>>>> int
>>>> main (int argc, char **argv)
>>>> {
>>>>   int *ptr = 0;
>>>>
>>>>   {
>>>>     int a;
>>>>     ptr = &a;
>>>>     *ptr = 12345;
>>>>   }
>>>>
>>>>   *ptr = 12345;
>>>>   return *ptr;
>>>> }
>>>>
> 
>> I'm still not sure how to do that. Problem is that transformation from:
>>
>>   ASAN_MARK (UNPOISON, &a, 4);
>>   a = 5;
>>   ASAN_MARK (POISON, &a, 4);
>>
>> to 
>>
>>   a_8 = 5;
>>   a_9 = ASAN_POISON ();
>>
>> happens in tree-ssa.c, after SSA is created, in situation where we prove the 'a'
>> does not need to live in memory. Thus said, question is how to identify that we
>> need to transform into SSA in a different way:
>>
>>    a_10 = ASAN_POISON ();
>>    ASAN_POISON (a_10);
> 
> I meant something like this (completely untested, and without the testcase
> added to the testsuite).
> The incremental patch as is relies on the ASAN_POISON_USE call having the
> argument the result of ASAN_POISON, it would ICE if that is not the case
> (especially if -fsanitize-recover=address).  Dunno if some optimization
> might decide to create a PHI in between, say merge two unrelated vars for
> if (something)
>   {
>     x_1 = ASAN_POISON ();
>     ...
>     ASAN_POISON_USE (x_1);
>   }
> else
>   {
>     y_2 = ASAN_POISON ();
>     ...
>     ASAN_POISON_USE (y_2);
>   }
> to turn that into:
> if (something)
>   x_1 = ASAN_POISON ();
> else
>   y_2 = ASAN_POISON ();
> _3 = PHI <x_1, y_2>;
> ...
> ASAN_POISON_USE (_3);
> 
> If it did, we would ICE because ASAN_POISON_USE would survive this way until
> expansion.  A quick fix for the ICE (if it can ever happen) would be easy,
> in sanopt remove ASAN_POISON_USE calls which have argument that is not lhs
> of ASAN_POISON (all other ASAN_POISON_USE calls will be handled by my
> incremental patch).  Of course that would also mean in that case we'd report
> a read rather than write.  But if it can't happen or is very unlikely to
> happen, then it is a non-issue.

Thank you Jakub for working on that.

The patch is fine, I added DCE support and a test-case. Please see attached patch.
asan.exp regression tests look fine and I've been building linux kernel with KASAN
enabled. I'll also do asan-boostrap.

I would like to commit the patch soon, should I squash both patches together, or would it
be preferred to separate basic optimization and support for stores?

Thanks,
Martin

> Something missing from the patch is some change in DCE to remove ASAN_POISON
> calls without lhs earlier.  I think we can't make ASAN_POISON ECF_CONST, we
> don't want it to be merged for different variables.
> 
> --- gcc/internal-fn.def.jj	2017-01-16 13:19:49.000000000 +0100
> +++ gcc/internal-fn.def	2017-01-16 14:25:37.427962196 +0100
> @@ -167,6 +167,7 @@ DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, EC
>  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)
> --- gcc/asan.c.jj	2017-01-16 13:19:49.000000000 +0100
> +++ gcc/asan.c	2017-01-16 14:52:34.022044223 +0100
> @@ -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_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_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_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);
>  	}
>      }
>  
> --- gcc/tree-into-ssa.c.jj	2017-01-01 12:45:35.000000000 +0100
> +++ gcc/tree-into-ssa.c	2017-01-16 14:32:14.853808726 +0100
> @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3.
>  #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_ope
>  }
>  
>  
> +/* 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,
>  	      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);
> --- gcc/internal-fn.c.jj	2017-01-16 13:19:49.000000000 +0100
> +++ gcc/internal-fn.c	2017-01-16 14:26:10.828529039 +0100
> @@ -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
> 
> 
> 	Jakub
> 


[-- Attachment #2: 0001-use-after-scope-handle-writes-to-a-poisoned-variable.patch --]
[-- Type: text/x-patch, Size: 7196 bytes --]

From c30802f6a29390a83208bfdb1090a6378ed42691 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.

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.
---
 gcc/asan.c                                     | 19 +++++++++++-------
 gcc/internal-fn.c                              |  8 ++++++++
 gcc/internal-fn.def                            |  1 +
 gcc/testsuite/gcc.dg/asan/use-after-scope-10.c | 22 +++++++++++++++++++++
 gcc/tree-into-ssa.c                            | 27 +++++++++++++++++++++++++-
 gcc/tree-ssa-dce.c                             |  4 ++++
 6 files changed, 73 insertions(+), 8 deletions(-)
 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/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..7fd2478adec 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -1384,6 +1384,10 @@ eliminate_unnecessary_stmts (void)
 		  case IFN_MUL_OVERFLOW:
 		    maybe_optimize_arith_overflow (&gsi, MULT_EXPR);
 		    break;
+		  case IFN_ASAN_POISON:
+		    if (!gimple_has_lhs (stmt))
+		      remove_dead_stmt (&gsi, bb);
+		    break;
 		  default:
 		    break;
 		  }
-- 
2.11.0