[PATCH] Remove dead code in asan.c

[PATCH] Remove dead code in asan.c

[Martin Liška]

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

Hi.

Following crap code was added by me when I added use-after-scope.
Actually decl always points to LASANPC, so asan_handled_variables->contains (decl)
is always false.

Well, originally the idea was to not clear content (place in shadow memory in between
red zoner) of auto variables, but as we emit 0xf5 in order to have working use-after-return,
it probably does not worth for doing an optimization?

Survives asan.exp tests.

Ready to be installed after proper regression tests?

Martin

gcc/ChangeLog:

2017-06-30  Martin Liska  <mliska@suse.cz>

	* asan.c (asan_emit_stack_protection): Do not use
	asan_handled_variables.
	(asan_expand_mark_ifn): Likewise.
---
 gcc/asan.c | 62 +-------------------------------------------------------------
 1 file changed, 1 insertion(+), 61 deletions(-)



[-- Attachment #2: 0001-Remove-dead-code-in-asan.c.patch --]
[-- Type: text/x-patch, Size: 3281 bytes --]

diff --git a/gcc/asan.c b/gcc/asan.c
index 2de16402c51..b415e28bd2f 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -246,11 +246,6 @@ static unsigned HOST_WIDE_INT asan_shadow_offset_value;
 static bool asan_shadow_offset_computed;
 static vec<char *> sanitized_sections;
 
-/* Set of variable declarations that are going to be guarded by
-   use-after-scope sanitizer.  */
-
-static hash_set<tree> *asan_handled_variables = NULL;
-
 hash_set <tree> *asan_used_labels = NULL;
 
 /* Sets shadow offset to value in string VAL.  */
@@ -1297,63 +1292,11 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
   if (STRICT_ALIGNMENT)
     set_mem_align (shadow_mem, (GET_MODE_ALIGNMENT (SImode)));
 
-  /* Unpoison shadow memory of a stack at the very end of a function.
-     As we're poisoning stack variables at the end of their scope,
-     shadow memory must be properly unpoisoned here.  The easiest approach
-     would be to collect all variables that should not be unpoisoned and
-     we unpoison shadow memory of the whole stack except ranges
-     occupied by these variables.  */
   last_offset = base_offset;
-  HOST_WIDE_INT current_offset = last_offset;
   if (length)
-    {
-      HOST_WIDE_INT var_end_offset = 0;
-      HOST_WIDE_INT stack_start = offsets[length - 1];
-      gcc_assert (last_offset == stack_start);
-
-      for (int l = length - 2; l > 0; l -= 2)
-	{
-	  HOST_WIDE_INT var_offset = offsets[l];
-	  current_offset = var_offset;
-	  var_end_offset = offsets[l - 1];
-	  HOST_WIDE_INT rounded_size = ROUND_UP (var_end_offset - var_offset,
-					     BITS_PER_UNIT);
-
-	  /* Should we unpoison the variable?  */
-	  if (asan_handled_variables != NULL
-	      && asan_handled_variables->contains (decl))
-	    {
-	      if (dump_file && (dump_flags & TDF_DETAILS))
-		{
-		  const char *n = (DECL_NAME (decl)
-				   ? IDENTIFIER_POINTER (DECL_NAME (decl))
-				   : "<unknown>");
-		  fprintf (dump_file, "Unpoisoning shadow stack for variable: "
-			   "%s (%" PRId64 "B)\n", n,
-			   var_end_offset - var_offset);
-		}
-
-	      unsigned HOST_WIDE_INT s
-		= shadow_mem_size (current_offset - last_offset);
-	      asan_clear_shadow (shadow_mem, s);
-	      HOST_WIDE_INT shift
-		= shadow_mem_size (current_offset - last_offset + rounded_size);
-	      shadow_mem = adjust_address (shadow_mem, VOIDmode, shift);
-	      last_offset = var_offset + rounded_size;
-	      current_offset = last_offset;
-	    }
-
-	}
-
-      /* Handle last redzone.  */
-      current_offset = offsets[0];
-      asan_clear_shadow (shadow_mem,
-			 shadow_mem_size (current_offset - last_offset));
-    }
+    asan_clear_shadow (shadow_mem, shadow_mem_size (offsets[0] - last_offset));
 
   /* Clean-up set with instrumented stack variables.  */
-  delete asan_handled_variables;
-  asan_handled_variables = NULL;
   delete asan_used_labels;
   asan_used_labels = NULL;
 
@@ -2802,9 +2745,6 @@ asan_expand_mark_ifn (gimple_stmt_iterator *iter)
     decl = TREE_OPERAND (decl, 0);
 
   gcc_checking_assert (TREE_CODE (decl) == VAR_DECL);
-  if (asan_handled_variables == NULL)
-    asan_handled_variables = new hash_set<tree> (16);
-  asan_handled_variables->add (decl);
   tree len = gimple_call_arg (g, 2);
 
   gcc_assert (tree_fits_shwi_p (len));

Re: [PATCH] Remove dead code in asan.c

[Jakub Jelinek]

On Fri, Jun 30, 2017 at 12:00:36PM +0200, Martin Liška wrote:
> Hi.
> 
> Following crap code was added by me when I added use-after-scope.
> Actually decl always points to LASANPC, so asan_handled_variables->contains (decl)
> is always false.
> 
> Well, originally the idea was to not clear content (place in shadow memory in between
> red zoner) of auto variables, but as we emit 0xf5 in order to have working use-after-return,
> it probably does not worth for doing an optimization?

use-after-return is only runtime conditional, defaults to off.
And your patch doesn't bring the code to anything close to what we had
before the -fsanitize-use-after-scope changes, just look what it did before
- only cleared the shadow spots that weren't known to be 0, clearing the
whole shadow might be too expensive.  Consider many KB large local
variables.

You can find the right decl in decls[l / 2] or decls[l / 2 - 1] or so.

	Jakub

Re: [PATCH] Remove dead code in asan.c

[Martin Liška]

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

On 06/30/2017 12:15 PM, Jakub Jelinek wrote:
> On Fri, Jun 30, 2017 at 12:00:36PM +0200, Martin Liška wrote:
>> Hi.
>>
>> Following crap code was added by me when I added use-after-scope.
>> Actually decl always points to LASANPC, so asan_handled_variables->contains (decl)
>> is always false.
>>
>> Well, originally the idea was to not clear content (place in shadow memory in between
>> red zoner) of auto variables, but as we emit 0xf5 in order to have working use-after-return,
>> it probably does not worth for doing an optimization?
> 
> use-after-return is only runtime conditional, defaults to off.
> And your patch doesn't bring the code to anything close to what we had
> before the -fsanitize-use-after-scope changes, just look what it did before
> - only cleared the shadow spots that weren't known to be 0, clearing the
> whole shadow might be too expensive.  Consider many KB large local
> variables.
> 
> You can find the right decl in decls[l / 2] or decls[l / 2 - 1] or so.
> 
> 	Jakub
> 

Huh, sorry, I overlooked that shadow stack used by use-after-return has it's own
code that does shadow memory modification.

Please take a look at v2 of the patch where I reverted the hunk to pre use-after-scope
and then I added condition to unpoison variables used in use-after-scope.

I'm going to test it.

Martin

[-- Attachment #2: 0001-Make-stack-epilogue-more-efficient.patch --]
[-- Type: text/x-patch, Size: 5091 bytes --]

From 2e249424e28a0d66ffb1c9cf8f54c1160043a2cc Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 30 Jun 2017 15:08:55 +0200
Subject: [PATCH] Make stack epilogue more efficient

gcc/ChangeLog:

2017-06-30  Martin Liska  <mliska@suse.cz>

	* asan.c (asan_emit_stack_protection): Unpoison just red zones
	and shadow memory of auto variables which are subject of
	use-after-scope sanitization.
	(asan_expand_mark_ifn): Add do set only when is_poison.
---
 gcc/asan.c | 79 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/gcc/asan.c b/gcc/asan.c
index 2de16402c51..5cce2be9962 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1062,7 +1062,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
   HOST_WIDE_INT base_offset = offsets[length - 1];
   HOST_WIDE_INT base_align_bias = 0, offset, prev_offset;
   HOST_WIDE_INT asan_frame_size = offsets[0] - base_offset;
-  HOST_WIDE_INT last_offset;
+  HOST_WIDE_INT last_offset, last_size;
   int l;
   unsigned char cur_shadow_byte = ASAN_STACK_MAGIC_LEFT;
   tree str_cst, decl, id;
@@ -1297,58 +1297,55 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
   if (STRICT_ALIGNMENT)
     set_mem_align (shadow_mem, (GET_MODE_ALIGNMENT (SImode)));
 
-  /* Unpoison shadow memory of a stack at the very end of a function.
-     As we're poisoning stack variables at the end of their scope,
-     shadow memory must be properly unpoisoned here.  The easiest approach
-     would be to collect all variables that should not be unpoisoned and
-     we unpoison shadow memory of the whole stack except ranges
-     occupied by these variables.  */
+  prev_offset = base_offset;
   last_offset = base_offset;
-  HOST_WIDE_INT current_offset = last_offset;
-  if (length)
+  last_size = 0;
+  for (l = length; l; l -= 2)
     {
-      HOST_WIDE_INT var_end_offset = 0;
-      HOST_WIDE_INT stack_start = offsets[length - 1];
-      gcc_assert (last_offset == stack_start);
-
-      for (int l = length - 2; l > 0; l -= 2)
+      offset = base_offset + ((offsets[l - 1] - base_offset)
+			     & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
+      if (last_offset + last_size != offset)
 	{
-	  HOST_WIDE_INT var_offset = offsets[l];
-	  current_offset = var_offset;
-	  var_end_offset = offsets[l - 1];
-	  HOST_WIDE_INT rounded_size = ROUND_UP (var_end_offset - var_offset,
-					     BITS_PER_UNIT);
+	  shadow_mem = adjust_address (shadow_mem, VOIDmode,
+				       (last_offset - prev_offset)
+				       >> ASAN_SHADOW_SHIFT);
+	  prev_offset = last_offset;
+	  asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT);
+	  last_offset = offset;
+	  last_size = 0;
+	}
+      last_size += base_offset + ((offsets[l - 2] - base_offset)
+				  & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1))
+		   - offset;
 
-	  /* Should we unpoison the variable?  */
+      /* Unpoison shadow memory that corresponds to a variable that was
+	 is subject of use-after-return sanitization.  */
+      if (l > 2)
+	{
+	  decl = decls[l / 2 - 2];
 	  if (asan_handled_variables != NULL
 	      && asan_handled_variables->contains (decl))
 	    {
+	      HOST_WIDE_INT size = offsets[l - 3] - offsets[l - 2];
 	      if (dump_file && (dump_flags & TDF_DETAILS))
 		{
 		  const char *n = (DECL_NAME (decl)
 				   ? IDENTIFIER_POINTER (DECL_NAME (decl))
 				   : "<unknown>");
 		  fprintf (dump_file, "Unpoisoning shadow stack for variable: "
-			   "%s (%" PRId64 "B)\n", n,
-			   var_end_offset - var_offset);
+			   "%s (%" PRId64 " B)\n", n, size);
 		}
 
-	      unsigned HOST_WIDE_INT s
-		= shadow_mem_size (current_offset - last_offset);
-	      asan_clear_shadow (shadow_mem, s);
-	      HOST_WIDE_INT shift
-		= shadow_mem_size (current_offset - last_offset + rounded_size);
-	      shadow_mem = adjust_address (shadow_mem, VOIDmode, shift);
-	      last_offset = var_offset + rounded_size;
-	      current_offset = last_offset;
+		last_size += size & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1);
 	    }
-
 	}
-
-      /* Handle last redzone.  */
-      current_offset = offsets[0];
-      asan_clear_shadow (shadow_mem,
-			 shadow_mem_size (current_offset - last_offset));
+    }
+  if (last_size)
+    {
+      shadow_mem = adjust_address (shadow_mem, VOIDmode,
+				   (last_offset - prev_offset)
+				   >> ASAN_SHADOW_SHIFT);
+      asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT);
     }
 
   /* Clean-up set with instrumented stack variables.  */
@@ -2802,9 +2799,13 @@ asan_expand_mark_ifn (gimple_stmt_iterator *iter)
     decl = TREE_OPERAND (decl, 0);
 
   gcc_checking_assert (TREE_CODE (decl) == VAR_DECL);
-  if (asan_handled_variables == NULL)
-    asan_handled_variables = new hash_set<tree> (16);
-  asan_handled_variables->add (decl);
+
+  if (is_poison)
+    {
+      if (asan_handled_variables == NULL)
+	asan_handled_variables = new hash_set<tree> (16);
+      asan_handled_variables->add (decl);
+    }
   tree len = gimple_call_arg (g, 2);
 
   gcc_assert (tree_fits_shwi_p (len));
-- 
2.13.1

Re: [PATCH] Remove dead code in asan.c

[Jakub Jelinek]

> +      /* Unpoison shadow memory that corresponds to a variable that was
> +	 is subject of use-after-return sanitization.  */

was is ?
And shouldn't it be subject to instead of subject of?

Otherwise LGTM.

	Jakub