diff mbox

[RFC] Introduce -fsanitize=use-after-scope (v2)

Message ID 268c6807-f5d3-926d-2571-72817d57fb5f@suse.cz
State New
Headers show

Commit Message

Martin Liška Nov. 2, 2016, 9:51 a.m. UTC
On 11/01/2016 04:12 PM, Jakub Jelinek wrote:
> On Tue, Nov 01, 2016 at 03:53:46PM +0100, Martin Liška wrote:

>> @@ -1504,7 +1505,7 @@ non_rewritable_lvalue_p (tree lhs)

>>  

>>  static void

>>  maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs,

>> -		    bitmap suitable_for_renaming)

>> +		    bitmap suitable_for_renaming, bitmap marked_nonaddressable)

>>  {

>>    /* Global Variables, result decls cannot be changed.  */

>>    if (is_global_var (var)

>> @@ -1522,6 +1523,7 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs,

>>  	  || !bitmap_bit_p (not_reg_needs, DECL_UID (var))))

>>      {

>>        TREE_ADDRESSABLE (var) = 0;

>> +      bitmap_set_bit (marked_nonaddressable, DECL_UID (var));

> 

> Why do you need the marked_nonaddressable bitmap?


Because the later loop (which visits every gimple statement) iterates only
if there's an entry in suitable_for_renaming.

> 

>>        if (is_gimple_reg (var))

>>  	bitmap_set_bit (suitable_for_renaming, DECL_UID (var));

>>        if (dump_file)

>> @@ -1550,20 +1552,43 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs,

>>      }

>>  }

>>  

>> -/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables.  */

>> +/* Return true when STMT is ASAN mark where second argument is an address

>> +   of a local variable.  */

>>  

>> -void

>> -execute_update_addresses_taken (void)

>> +static bool

>> +is_asan_mark_p (gimple *stmt)

>> +{

>> +  if (!gimple_call_internal_p (stmt, IFN_ASAN_MARK))

>> +    return false;

>> +

>> +  tree addr = get_base_address (gimple_call_arg (stmt, 1));

>> +  if (TREE_CODE (addr) == ADDR_EXPR

>> +      && TREE_CODE (TREE_OPERAND (addr, 0)) == VAR_DECL)

> 

> Just check here if dropping TREE_ADDRESSABLE from the VAR (use VAR_P btw)

> would turn it into is_gimple_reg), and don't return true if not.


Well, the predicate is called once before maybe_optimize_var, thus I need to have
it conservative and not consider TREE_ADDRESSABLE flag. Having argument would work
for that?

> 

>> +    return true;

>> +

>> +  return false;

>> +}

>> +

>> +/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables.

>> +   If SANITIZE_ASAN_MARK is set to true, sanitize also ASAN_MARK built-ins.  */

>> +

>> +

>> +static void

>> +execute_update_addresses_taken (bool sanitize_asan_mark = false)

> 

> I wonder if the sanitize_asan_mark wouldn't better be some PROP_* property

> set during the asan pass and kept on until end of compilation of that

> function.  That means even if a var only addressable because of ASAN_MARK is

> discovered after this pass we'd still be able to rewrite it into SSA.


It's doable (please see attached patch) and also nicer. However, one would need to
extend curr_properties to long type as we already use 16 existing values.

Martin

> 

> 	Jakub

>
diff mbox

Patch

From ad5f68a010674118fac7ca8b6953f7b99fd3c2a8 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 1 Nov 2016 11:21:20 +0100
Subject: [PATCH] Use-after-scope: do not mark variables that are no longer
 addressable

gcc/ChangeLog:

2016-11-02  Martin Liska  <mliska@suse.cz>

	* asan.c: Update properties_provided and todo_flags_finish.
	* function.h (struct GTY): Change int to long as there's not
	enough space for a new value.
	* tree-pass.h: Define PROP_asan_check_done.
	* tree-ssa.c (maybe_optimize_var): Add new argument.
	(is_asan_mark_p): New function.
	(execute_update_addresses_taken): Handle ASAN_MARK internal fns.
---
 gcc/asan.c      |  5 +++--
 gcc/function.h  |  2 +-
 gcc/tree-pass.h |  1 +
 gcc/tree-ssa.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++------------
 4 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/gcc/asan.c b/gcc/asan.c
index 95495d2..94ee877 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -59,6 +59,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "params.h"
 #include "builtins.h"
 #include "fnmatch.h"
+#include "tree-ssa.h"
 
 /* AddressSanitizer finds out-of-bounds and use-after-free bugs
    with <2x slowdown on average.
@@ -2993,10 +2994,10 @@  const pass_data pass_data_asan =
   OPTGROUP_NONE, /* optinfo_flags */
   TV_NONE, /* tv_id */
   ( PROP_ssa | PROP_cfg | PROP_gimple_leh ), /* properties_required */
-  0, /* properties_provided */
+  PROP_asan_check_done, /* properties_provided */
   0, /* properties_destroyed */
   0, /* todo_flags_start */
-  TODO_update_ssa, /* todo_flags_finish */
+  TODO_update_ssa | TODO_update_address_taken, /* todo_flags_finish */
 };
 
 class pass_asan : public gimple_opt_pass
diff --git a/gcc/function.h b/gcc/function.h
index e854c7f..5600488 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -289,7 +289,7 @@  struct GTY(()) function {
   location_t function_end_locus;
 
   /* Properties used by the pass manager.  */
-  unsigned int curr_properties;
+  unsigned long curr_properties;
   unsigned int last_verified;
 
   /* Non-null if the function does something that would prevent it from
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index da9ba13..ea43593 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -222,6 +222,7 @@  protected:
 						   of math functions; the
 						   current choices have
 						   been optimized.  */
+#define PROP_asan_check_done	(1 << 16)	/* ASAN check is emitted */
 
 #define PROP_trees \
   (PROP_gimple_any | PROP_gimple_lcf | PROP_gimple_leh | PROP_gimple_lomp)
diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c
index 135952b..1492f47 100644
--- a/gcc/tree-ssa.c
+++ b/gcc/tree-ssa.c
@@ -41,6 +41,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "cfgexpand.h"
 #include "tree-cfg.h"
 #include "tree-dfa.h"
+#include "asan.h"
 
 /* Pointer map of variable mappings, keyed by edge.  */
 static hash_map<edge, auto_vec<edge_var_map> > *edge_var_maps;
@@ -1504,7 +1505,7 @@  non_rewritable_lvalue_p (tree lhs)
 
 static void
 maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs,
-		    bitmap suitable_for_renaming)
+		    bitmap suitable_for_renaming, bitmap marked_nonaddressable)
 {
   /* Global Variables, result decls cannot be changed.  */
   if (is_global_var (var)
@@ -1522,6 +1523,7 @@  maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs,
 	  || !bitmap_bit_p (not_reg_needs, DECL_UID (var))))
     {
       TREE_ADDRESSABLE (var) = 0;
+      bitmap_set_bit (marked_nonaddressable, DECL_UID (var));
       if (is_gimple_reg (var))
 	bitmap_set_bit (suitable_for_renaming, DECL_UID (var));
       if (dump_file)
@@ -1550,6 +1552,22 @@  maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs,
     }
 }
 
+/* Return true when STMT is ASAN mark where second argument is an address
+   of a local variable.  */
+
+static bool
+is_asan_mark_p (gimple *stmt)
+{
+  if (!gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+    return false;
+
+  tree addr = get_base_address (gimple_call_arg (stmt, 1));
+  if (TREE_CODE (addr) == ADDR_EXPR && VAR_P (TREE_OPERAND (addr, 0)))
+    return true;
+
+  return false;
+}
+
 /* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables.  */
 
 void
@@ -1559,9 +1577,12 @@  execute_update_addresses_taken (void)
   bitmap addresses_taken = BITMAP_ALLOC (NULL);
   bitmap not_reg_needs = BITMAP_ALLOC (NULL);
   bitmap suitable_for_renaming = BITMAP_ALLOC (NULL);
+  bitmap marked_nonaddressable = BITMAP_ALLOC (NULL);
   tree var;
   unsigned i;
 
+  bool sanitize_asan_mark = cfun->curr_properties & PROP_asan_check_done;
+
   timevar_push (TV_ADDRESS_TAKEN);
 
   /* Collect into ADDRESSES_TAKEN all variables whose address is taken within
@@ -1575,17 +1596,23 @@  execute_update_addresses_taken (void)
 	  enum gimple_code code = gimple_code (stmt);
 	  tree decl;
 
-	  if (code == GIMPLE_CALL
-	      && optimize_atomic_compare_exchange_p (stmt))
+	  if (code == GIMPLE_CALL)
 	    {
-	      /* For __atomic_compare_exchange_N if the second argument
-		 is &var, don't mark var addressable;
-		 if it becomes non-addressable, we'll rewrite it into
-		 ATOMIC_COMPARE_EXCHANGE call.  */
-	      tree arg = gimple_call_arg (stmt, 1);
-	      gimple_call_set_arg (stmt, 1, null_pointer_node);
-	      gimple_ior_addresses_taken (addresses_taken, stmt);
-	      gimple_call_set_arg (stmt, 1, arg);
+	      if (optimize_atomic_compare_exchange_p (stmt))
+		{
+		  /* For __atomic_compare_exchange_N if the second argument
+		     is &var, don't mark var addressable;
+		     if it becomes non-addressable, we'll rewrite it into
+		     ATOMIC_COMPARE_EXCHANGE call.  */
+		  tree arg = gimple_call_arg (stmt, 1);
+		  gimple_call_set_arg (stmt, 1, null_pointer_node);
+		  gimple_ior_addresses_taken (addresses_taken, stmt);
+		  gimple_call_set_arg (stmt, 1, arg);
+		}
+	      else if (sanitize_asan_mark && is_asan_mark_p (stmt))
+		;
+	      else
+		gimple_ior_addresses_taken (addresses_taken, stmt);
 	    }
 	  else
 	    /* Note all addresses taken by the stmt.  */
@@ -1675,15 +1702,17 @@  execute_update_addresses_taken (void)
      for -g vs. -g0.  */
   for (var = DECL_ARGUMENTS (cfun->decl); var; var = DECL_CHAIN (var))
     maybe_optimize_var (var, addresses_taken, not_reg_needs,
-			suitable_for_renaming);
+			suitable_for_renaming, marked_nonaddressable);
 
   FOR_EACH_VEC_SAFE_ELT (cfun->local_decls, i, var)
     maybe_optimize_var (var, addresses_taken, not_reg_needs,
-			suitable_for_renaming);
+			suitable_for_renaming, marked_nonaddressable);
 
   /* Operand caches need to be recomputed for operands referencing the updated
      variables and operands need to be rewritten to expose bare symbols.  */
-  if (!bitmap_empty_p (suitable_for_renaming))
+  if (!bitmap_empty_p (suitable_for_renaming)
+      || (asan_sanitize_use_after_scope ()
+	  && !bitmap_empty_p (marked_nonaddressable)))
     {
       FOR_EACH_BB_FN (bb, cfun)
 	for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi);)
@@ -1841,6 +1870,17 @@  execute_update_addresses_taken (void)
 			continue;
 		      }
 		  }
+		else if (sanitize_asan_mark && is_asan_mark_p (stmt))
+		  {
+		    tree var = TREE_OPERAND (gimple_call_arg (stmt, 1), 0);
+		    if (bitmap_bit_p (suitable_for_renaming, DECL_UID (var))
+			|| bitmap_bit_p (marked_nonaddressable, DECL_UID (var)))
+		      {
+			unlink_stmt_vdef (stmt);
+			gsi_remove (&gsi, true);
+			continue;
+		      }
+		  }
 		for (i = 0; i < gimple_call_num_args (stmt); ++i)
 		  {
 		    tree *argp = gimple_call_arg_ptr (stmt, i);
@@ -1896,6 +1936,7 @@  execute_update_addresses_taken (void)
   BITMAP_FREE (not_reg_needs);
   BITMAP_FREE (addresses_taken);
   BITMAP_FREE (suitable_for_renaming);
+  BITMAP_FREE (marked_nonaddressable);
   timevar_pop (TV_ADDRESS_TAKEN);
 }
 
-- 
2.10.1