[PINGv4][PATCH] ASan on unaligned accesses

[Jakub Jelinek]

On Thu, Mar 26, 2015 at 03:28:53PM +0300, Marat Zakirov wrote:
> 2015-02-25  Marat Zakirov  <m.zakirov@samsung.com>
> 
> 	* asan.c (asan_emit_stack_protection): Support for misalign accesses. 
> 	(asan_expand_check_ifn): Likewise. 
> 	* params.def: New option asan-catch-misaligned.
> 	* params.h: New param ASAN_CATCH_MISALIGNED.
> 	* doc/invoke.texi: New asan param description.

Can you please start by explaining the asan_emit_stack_protection changes?
What is the problem there, what do you want to achieve etc.?
Is it to support ABI violating stack pointer alignment, or something
different?  If so, the compiler knows (or should be aware) what the stack
alignment is.
The asan_expand_check_ifn change looks reasonable.

Also, the changes regress code quality without the parameter,
say on the testcase you've added at -O2 -fsanitize=address (no param used),
on x86_64 the patch causes undesirable
 	movl	$0, 2147450880(%rbp)
-	movq	$0, 2147450892(%rbp)
+	movl	$0, 2147450892(%rbp)
+	movl	$0, 2147450896(%rbp)
change.

> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -1050,7 +1050,6 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
>    rtx_code_label *lab;
>    rtx_insn *insns;
>    char buf[30];
> -  unsigned char shadow_bytes[4];

Do you really need to do that and why?

>    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;
> @@ -1059,6 +1058,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
>    unsigned char cur_shadow_byte = ASAN_STACK_MAGIC_LEFT;
>    tree str_cst, decl, id;
>    int use_after_return_class = -1;
> +  bool misalign = (flag_sanitize & SANITIZE_KERNEL_ADDRESS) || ASAN_CATCH_MISALIGNED;

Too long line.

> +  vec<rtx> shadow_mems;
> +  vec<unsigned char> shadow_bytes;
> +
> +  shadow_mems.create(0);
> +  shadow_bytes.create(0);

2x missing space before (.

> +	  shadow_mems.safe_push(shadow_mem);

Similarly.

> +	  shadow_bytes.safe_push (cur_shadow_byte);
> +	  shadow_bytes.safe_push (cur_shadow_byte);
> +	  shadow_bytes.safe_push (cur_shadow_byte);
> +	  shadow_mems.safe_push(shadow_mem);

Similarly.

>      }
> +  for (unsigned i = 0; misalign && i < shadow_bytes.length () - 1; i++)
> +    if (shadow_bytes[i] == 0 && shadow_bytes[i + 1] > 0)
> +      shadow_bytes[i] = 8 + (shadow_bytes[i + 1] > 7 ? 0 : shadow_bytes[i + 1]);

Too long line.

> -  prev_offset = base_offset;
> -  last_offset = base_offset;
> -  last_size = 0;
> -  for (l = length; l; l -= 2)
> -    {
> -      offset = base_offset + ((offsets[l - 1] - base_offset)
> -			     & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
> -      if (last_offset + last_size != offset)
> -	{
> -	  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;
> -    }
> -  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);
> -    }
> +  for (unsigned i = 0; i < shadow_mems.length (); i++)
> +    asan_clear_shadow (shadow_mems[i], 4);

Bet this change causes the regression I've mentioned above.

> @@ -2546,6 +2555,7 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls)
>    gimple g = gsi_stmt (*iter);
>    location_t loc = gimple_location (g);
>  
> +  bool misalign = (flag_sanitize & SANITIZE_KERNEL_ADDRESS) || ASAN_CATCH_MISALIGNED;

Too long line.

	Jakub