Re: [PATCH][GCC][AArch64] Cleanup the AArch64 testsuite when stack-clash is on [Patch (6/6)]

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Tue, Jul 24, 2018 at 05:28:03AM -0500, Tamar Christina wrote:
> Hi All,
> 
> This patch cleans up the testsuite when a run is done with stack clash
> protection turned on.
> 
> Concretely this switches off -fstack-clash-protection for a couple of tests:
> 
> * sve: We don't yet support stack-clash-protection and sve, so for now turn these off.
> * assembler scan: some tests are quite fragile in that they check for exact
>        assembly output, e.g. check for exact amount of sub etc.  These won't
>        match now.
> * vla: Some of the ubsan tests negative array indices. Because the arrays weren't
>        used before the incorrect $sp wouldn't have been used. The correct value is
>        restored on ret.  Now however we probe the $sp which causes a segfault.
> * params: When testing the parameters we have to skip these on AArch64 because of our
>           custom constraints on them.  We already test them separately so this isn't a
>           loss.
> 
> Note that the testsuite is not entire clean due to gdb failure caused by alloca with
> stack clash. On AArch64 we output an incorrect .loc directive, but this is already the
> case with the current implementation in GCC and is a bug unrelated to this patch series.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu and no issues.
> Both targets were tested with stack clash on and off by default.
> 
> Ok for trunk?

For each of the generic tests you skip because of mismatched bounds, I think
we should ensure we have an equivalent test checking that behaviour in
gcc.target/aarch64/ . If we have that, it might be good to cross-reference
them with a comment above your skip lines.

> * vla: Some of the ubsan tests negative array indices. Because the arrays weren't
>        used before the incorrect $sp wouldn't have been used. The correct value is
>        restored on ret.  Now however we probe the $sp which causes a segfault.

This is interesting behaviour; is it a desirable side effect of your changes?

Otherwise, this patch is OK.

Thanks,
James


> gcc/testsuite/
> 2018-07-24  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/86486
> 	* gcc.dg/pr82788.c: Skip for AArch64.
> 	* gcc.dg/guality/vla-1.c: Turn off stack-clash.
> 	* gcc.target/aarch64/subsp.c: Likewise.
> 	* gcc.target/aarch64/sve/mask_struct_load_3.c: Likewise.
> 	* gcc.target/aarch64/sve/mask_struct_store_3.c: Likewise.
> 	* gcc.target/aarch64/sve/mask_struct_store_4.c: Likewise.
> 	* gcc.dg/params/blocksort-part.c: Skip stack-clash checks
> 	on AArch64.
> 	* gcc.dg/stack-check-10.c: Add AArch64 specific checks.
> 	* gcc.dg/stack-check-5.c: Add AArch64 specific checks.
> 	* gcc.dg/stack-check-6a.c: Skip on AArch64, we don't support this.
> 	* testsuite/lib/target-supports.exp
> 	(check_effective_target_frame_pointer_for_non_leaf): AArch64 does not
> 	require frame pointer for non-leaf functions.
> 
> > -----Original Message-----
> > From: Tamar Christina <tamar.christina@arm.com>
> > Sent: Wednesday, July 11, 2018 12:23
> > To: gcc-patches@gcc.gnu.org
> > Cc: nd <nd@arm.com>; James Greenhalgh <James.Greenhalgh@arm.com>;
> > Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> > <Marcus.Shawcroft@arm.com>
> > Subject: [PATCH][GCC][AArch64] Cleanup the AArch64 testsuite when stack-
> > clash is on [Patch (6/6)]
> > 
> > Hi All,
> > 
> > This patch cleans up the testsuite when a run is done with stack clash
> > protection turned on.
> > 
> > Concretely this switches off -fstack-clash-protection for a couple of tests:
> > 
> > * sve: We don't yet support stack-clash-protection and sve, so for now turn
> > these off.
> > * assembler scan: some tests are quite fragile in that they check for exact
> >        assembly output, e.g. check for exact amount of sub etc.  These won't
> >        match now.
> > * vla: Some of the ubsan tests negative array indices. Because the arrays
> > weren't
> >        used before the incorrect $sp wouldn't have been used. The correct
> > value is
> >        restored on ret.  Now however we probe the $sp which causes a segfault.
> > * params: When testing the parameters we have to skip these on AArch64
> > because of our
> >           custom constraints on them.  We already test them separately so this
> > isn't a
> >           loss.
> > 
> > Note that the testsuite is not entire clean due to gdb failure caused by alloca
> > with stack clash. On AArch64 we output an incorrect .loc directive, but this is
> > already the case with the current implementation in GCC and is a bug
> > unrelated to this patch series.
> > 
> > Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
> > and no issues.
> > Both targets were tested with stack clash on and off by default.
> > 
> > Ok for trunk?
> > 
> > Thanks,
> > Tamar
> > 
> > gcc/testsuite/
> > 2018-07-11  Tamar Christina  <tamar.christina@arm.com>
> > 
> > 	PR target/86486
> > 	gcc.dg/pr82788.c: Skip for AArch64.
> > 	gcc.dg/guality/vla-1.c: Turn off stack-clash.
> > 	gcc.target/aarch64/subsp.c: Likewise.
> > 	gcc.target/aarch64/sve/mask_struct_load_3.c: Likewise.
> > 	gcc.target/aarch64/sve/mask_struct_store_3.c: Likewise.
> > 	gcc.target/aarch64/sve/mask_struct_store_4.c: Likewise.
> > 	gcc.dg/params/blocksort-part.c: Skip stack-clash checks
> > 	on AArch64.
> > 
> > --

> diff --git a/gcc/testsuite/c-c++-common/ubsan/vla-1.c b/gcc/testsuite/c-c++-common/ubsan/vla-1.c
> index 52ade3aab7566dce3ca7ef931ac65895005d5e13..c97465edae195442a71ee66ab25015a2ac4fc8fc 100644
> --- a/gcc/testsuite/c-c++-common/ubsan/vla-1.c
> +++ b/gcc/testsuite/c-c++-common/ubsan/vla-1.c
> @@ -1,5 +1,5 @@
>  /* { dg-do run } */
> -/* { dg-options "-fsanitize=vla-bound -Wall -Wno-unused-variable" } */
> +/* { dg-options "-fsanitize=vla-bound -Wall -Wno-unused-variable -fno-stack-clash-protection" } */
>  
>  typedef long int V;
>  int x = -1;
> diff --git a/gcc/testsuite/gcc.dg/params/blocksort-part.c b/gcc/testsuite/gcc.dg/params/blocksort-part.c
> index a9154f2e61ccd21b60153f20be3891b988f9ef2c..1e677878e7bd9c68b026f8c72b0de9f01e15459c 100644
> --- a/gcc/testsuite/gcc.dg/params/blocksort-part.c
> +++ b/gcc/testsuite/gcc.dg/params/blocksort-part.c
> @@ -1,3 +1,4 @@
> +/* { dg-skip-if "AArch64 does not support these bounds." { aarch64*-*-* } { "--param stack-clash-protection-*" } } */
>  
>  /*-------------------------------------------------------------*/
>  /*--- Block sorting machinery                               ---*/
> diff --git a/gcc/testsuite/gcc.dg/pr82788.c b/gcc/testsuite/gcc.dg/pr82788.c
> index a8f628fd7f66c3e56739f6ff491df38b23f4d4df..41c442f61a625c8b350e1e4c870a98d86b167031 100644
> --- a/gcc/testsuite/gcc.dg/pr82788.c
> +++ b/gcc/testsuite/gcc.dg/pr82788.c
> @@ -1,4 +1,5 @@
>  /* { dg-do run } */
>  /* { dg-options "-O2 -fstack-clash-protection --param stack-clash-protection-probe-interval=10 --param stack-clash-protection-guard-size=12" } */
>  /* { dg-require-effective-target supports_stack_clash_protection } */
> +/* { dg-skip-if "AArch64 does not support this interval." { aarch64*-*-* } } */
>  int main() { int a[1442]; return 0;}
> diff --git a/gcc/testsuite/gcc.dg/stack-check-10.c b/gcc/testsuite/gcc.dg/stack-check-10.c
> index a86956ad6925464e4a938a33e609fae5004201c7..2f5a090cb7a4ed6d2e6e4317492150a348a326ab 100644
> --- a/gcc/testsuite/gcc.dg/stack-check-10.c
> +++ b/gcc/testsuite/gcc.dg/stack-check-10.c
> @@ -39,3 +39,4 @@ f3 (void)
>     need a frame pointer.  Otherwise neither should.  */
>  /* { dg-final { scan-rtl-dump-times "Stack clash no frame pointer needed" 2 "pro_and_epilogue" { target { ! frame_pointer_for_non_leaf } } } } */
>  /* { dg-final { scan-rtl-dump-times "Stack clash frame pointer needed" 2 "pro_and_epilogue" { target { frame_pointer_for_non_leaf } } } } */
> +/* { dg-final { scan-rtl-dump-times "Stack clash no probe small stack adjustment in prologue" 2 "pro_and_epilogue" { target { aarch64*-*-* } } } } */
> diff --git a/gcc/testsuite/gcc.dg/stack-check-5.c b/gcc/testsuite/gcc.dg/stack-check-5.c
> index 604fa3cf6c5b502f74c1e3497b3b8d72a15bb3ea..0243147939c10e8f4632520b12714724af85b332 100644
> --- a/gcc/testsuite/gcc.dg/stack-check-5.c
> +++ b/gcc/testsuite/gcc.dg/stack-check-5.c
> @@ -66,7 +66,9 @@ f3 (void)
>  /* { dg-final { scan-rtl-dump-times "Stack clash no frame pointer needed" 4 "pro_and_epilogue" { target { ! frame_pointer_for_non_leaf } } } } */
>  /* { dg-final { scan-rtl-dump-times "Stack clash no frame pointer needed" 2 "pro_and_epilogue" { target { frame_pointer_for_non_leaf } } } } */
>  /* { dg-final { scan-rtl-dump-times "Stack clash frame pointer needed" 2 "pro_and_epilogue" { target { frame_pointer_for_non_leaf } } } } */
> -
> +/* AArch64 won't require a probe here due to the allocation amount being less than 1KB.  */
> +/* { dg-final { scan-rtl-dump-times "Stack clash no probe small stack adjustment in prologue" 3 "pro_and_epilogue" { target { aarch64*-*-* } } } } */
> +/* { dg-final { scan-rtl-dump-times "Stack clash no probe no stack adjustment in prologue" 1 "pro_and_epilogue" { target { aarch64*-*-* } } } } */
>  
>  /* We have selected the size of the array in f2/f3 to be large enough
>     to not live in the red zone on targets that support it.
> diff --git a/gcc/testsuite/gcc.dg/stack-check-6a.c b/gcc/testsuite/gcc.dg/stack-check-6a.c
> index 8fb9c621585957a85877ebadfbc4a8daabe4311c..68dd9bc48a0c26ecb84ddd2c09b8aa74d3276695 100644
> --- a/gcc/testsuite/gcc.dg/stack-check-6a.c
> +++ b/gcc/testsuite/gcc.dg/stack-check-6a.c
> @@ -5,6 +5,7 @@
>  /* { dg-options "-O2 -fstack-clash-protection -fdump-rtl-pro_and_epilogue -fno-optimize-sibling-calls --param stack-clash-protection-probe-interval=12 --param stack-clash-protection-guard-size=16" } */
>  /* { dg-require-effective-target supports_stack_clash_protection  } */
>  /* { dg-skip-if "" { *-*-* } { "-fstack-protector*" } { "" } } */
> +/* { dg-skip-if "" { aarch64*-*-* } } */
>  
>  
>  #include "stack-check-6.c"
> diff --git a/gcc/testsuite/gcc.target/aarch64/subsp.c b/gcc/testsuite/gcc.target/aarch64/subsp.c
> index 70d848c59d1f1e4df4314ca012c7a5d9d3b91ebc..6ef6b2c90ae694055749a94b68cbba5ee4aea882 100644
> --- a/gcc/testsuite/gcc.target/aarch64/subsp.c
> +++ b/gcc/testsuite/gcc.target/aarch64/subsp.c
> @@ -1,4 +1,4 @@
> -/* { dg-options "-O" } */
> +/* { dg-options "-O -fno-stack-clash-protection" } */
>  
>  int foo (void *);
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_load_3.c b/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_load_3.c
> index 29702ab55f249c3ebd0baf44981870524098e1e4..baeec61bb59aff56f0dcc20fc6ec6b93d517490e 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_load_3.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_load_3.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-O2 -ftree-vectorize -ffast-math" } */
> +/* { dg-options "-O2 -ftree-vectorize -ffast-math -fno-stack-clash-protection" } */
>  
>  #include <stdint.h>
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_3.c b/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_3.c
> index 001f5be8ff58bfcc75eccc4c050bef1e53faffeb..eae3be7a7b24dc124f7c1c26a97fb25400cc62d2 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_3.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_3.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-O2 -ftree-vectorize -ffast-math" } */
> +/* { dg-options "-O2 -ftree-vectorize -ffast-math -fno-stack-clash-protection" } */
>  
>  #include <stdint.h>
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_4.c b/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_4.c
> index 59e9ee49c4a214b731ed1975da0dcfa46c059f8b..ce9825f73e8495a7f5c1f4ab1d5bf1aaf5035e17 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/mask_struct_store_4.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-O2 -ftree-vectorize -ffast-math" } */
> +/* { dg-options "-O2 -ftree-vectorize -ffast-math -fno-stack-clash-protection" } */
>  
>  #include <stdint.h>
>  
> diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index 7ec350213e9225ad342d030afac30bd613851aa1..5624f361c59b97cb967b3485adc8db01da048a84 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -9280,10 +9280,6 @@ proc check_effective_target_supports_stack_clash_protection { } {
>  # Return 1 if the target creates a frame pointer for non-leaf functions
>  # Note we ignore cases where we apply tail call optimization here.
>  proc check_effective_target_frame_pointer_for_non_leaf { } {
> -  if { [istarget aarch*-*-*] } {
> -	return 1
> -  }
> -
>    # Solaris/x86 defaults to -fno-omit-frame-pointer.
>    if { [istarget i?86-*-solaris*] || [istarget x86_64-*-solaris*] } {
>      return 1
>