diff mbox series

testsuite/gcc.dg/pr84877.c: Add machinery to stabilize stack aligmnent

Message ID 20240905154452.9B4052044A@pchp3.se.axis.com
State Committed
Commit b1ea710b1bcdda233f96538c5404228d2b244e01
Headers
Series testsuite/gcc.dg/pr84877.c: Add machinery to stabilize stack aligmnent |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed

Commit Message

Hans-Peter Nilsson Sept. 5, 2024, 3:44 p.m. UTC 
  Tested adding 0..more-than-four environment variables,
running cris-sim+cris-elf.  I also checked that foo stays
the same generated code regardless of the new code: this is
not obviously true as foo is "just" noinline, not __noipa__.

Ok to commit?

-- >8 --
This test awkwardly "blinks"; xfails and xpasses apparently
randomly for cris-elf using the "gdb simulator".  On
inspection, I see that the stack address depends on the
number of environment variables, deliberately passed to the
simulator, each adding the size of a pointer.

This test is IMHO important enough not to be just skipped
just because it blinks (fixing the actual problem is a
different task).

I guess a random non-16 stack-alignment could happen for
other targets as well, so let's try and add a generic
machinery to "stabilize" the test as failing, by allocating
a dynamic amount to make sure it's misaligned.  The most
target-dependent item here is an offset between the incoming
stack-pointer value (within main in the added framework) and
outgoing (within "xmain" as called from main when setting up
the p0 parameter).  I know there are other wonderful stack
shapes, but such targets would fall under the "complicated
situations"-label and are no worse off than before.

	* gcc.dg/pr84877.c: Try to make the test result	consistent by
	misaligning the stack.
---
 gcc/testsuite/gcc.dg/pr84877.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Hans-Peter Nilsson Sept. 12, 2024, 11:46 p.m. UTC | #1 
Ping...

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Thu, 5 Sep 2024 17:44:52 +0200
> 
> Tested adding 0..more-than-four environment variables,
> running cris-sim+cris-elf.  I also checked that foo stays
> the same generated code regardless of the new code: this is
> not obviously true as foo is "just" noinline, not __noipa__.
> 
> Ok to commit?
> 
> -- >8 --
> This test awkwardly "blinks"; xfails and xpasses apparently
> randomly for cris-elf using the "gdb simulator".  On
> inspection, I see that the stack address depends on the
> number of environment variables, deliberately passed to the
> simulator, each adding the size of a pointer.
> 
> This test is IMHO important enough not to be just skipped
> just because it blinks (fixing the actual problem is a
> different task).
> 
> I guess a random non-16 stack-alignment could happen for
> other targets as well, so let's try and add a generic
> machinery to "stabilize" the test as failing, by allocating
> a dynamic amount to make sure it's misaligned.  The most
> target-dependent item here is an offset between the incoming
> stack-pointer value (within main in the added framework) and
> outgoing (within "xmain" as called from main when setting up
> the p0 parameter).  I know there are other wonderful stack
> shapes, but such targets would fall under the "complicated
> situations"-label and are no worse off than before.
> 
> 	* gcc.dg/pr84877.c: Try to make the test result	consistent by
> 	misaligning the stack.
> ---
>  gcc/testsuite/gcc.dg/pr84877.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/gcc/testsuite/gcc.dg/pr84877.c b/gcc/testsuite/gcc.dg/pr84877.c
> index e82991f42dd4..2f2e29578df9 100644
> --- a/gcc/testsuite/gcc.dg/pr84877.c
> +++ b/gcc/testsuite/gcc.dg/pr84877.c
> @@ -3,6 +3,32 @@
>  
>  #include <inttypes.h>
>  
> +#ifdef __CRIS__
> +#define OUTGOING_SP_OFFSET (-sizeof (void *))
> +/* Suggestion: append #elif defined(__<TARGET-IDENTIFIER>__) after this comment,
> +   either defining OUTGOING_SP_OFFSET to whatever the pertinent amount is at -O2,
> +   if that makes your target consistently fail this test, or define
> +   DO_NOT_TAMPER for more complicated situations.  Either way, compile with
> +   -DDO_NO_TAMPER to avoid any meddling.  */
> +#endif
> +
> +#if defined (OUTGOING_SP_OFFSET) && !defined (DO_NOT_TAMPER)
> +extern int xmain () __attribute__ ((__noipa__));
> +int main ()
> +{
> +  uintptr_t misalignment
> +    = (OUTGOING_SP_OFFSET
> +        + (15 & (uintptr_t) __builtin_stack_address ()));
> +  /* Allocate a minimal amount if the stack was accidentally aligned.  */
> +  void *q = __builtin_alloca (misalignment == 0);
> +  xmain ();
> +  /* Fake use to avoid the "allocation" being optimized out.  */
> +  asm volatile ("" : : "rm" (q));
> +  return 0;
> +}
> +#define main xmain
> +#endif
> +
>  struct U {
>      int M0;
>      int M1;
> -- 
> 2.30.2
>
Jeff Law Sept. 18, 2024, 5:56 p.m. UTC | #2 
On 9/5/24 9:44 AM, Hans-Peter Nilsson wrote:
> Tested adding 0..more-than-four environment variables,
> running cris-sim+cris-elf.  I also checked that foo stays
> the same generated code regardless of the new code: this is
> not obviously true as foo is "just" noinline, not __noipa__.
> 
> Ok to commit?
> 
> -- >8 --
> This test awkwardly "blinks"; xfails and xpasses apparently
> randomly for cris-elf using the "gdb simulator".  On
> inspection, I see that the stack address depends on the
> number of environment variables, deliberately passed to the
> simulator, each adding the size of a pointer.
> 
> This test is IMHO important enough not to be just skipped
> just because it blinks (fixing the actual problem is a
> different task).
> 
> I guess a random non-16 stack-alignment could happen for
> other targets as well, so let's try and add a generic
> machinery to "stabilize" the test as failing, by allocating
> a dynamic amount to make sure it's misaligned.  The most
> target-dependent item here is an offset between the incoming
> stack-pointer value (within main in the added framework) and
> outgoing (within "xmain" as called from main when setting up
> the p0 parameter).  I know there are other wonderful stack
> shapes, but such targets would fall under the "complicated
> situations"-label and are no worse off than before.
> 
> 	* gcc.dg/pr84877.c: Try to make the test result	consistent by
> 	misaligning the stack.
So I've got this test marked as flakey and thus it's currently being 
skipped everywhere.

If one was to look at the changes it's pretty clear that it only affects 
cris right now.    I wouldn't be surprised if other targets need similar 
handling.

Ok for the trunk.  I'll remove it from my list of flakey tests after you 
push this to the trunk.  Hopefully we'll see any flip-flops in behavior 
on other targets over time and we can fix them.

jeff
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.dg/pr84877.c b/gcc/testsuite/gcc.dg/pr84877.c
index e82991f42dd4..2f2e29578df9 100644
--- a/gcc/testsuite/gcc.dg/pr84877.c
+++ b/gcc/testsuite/gcc.dg/pr84877.c
@@ -3,6 +3,32 @@ 
 
 #include <inttypes.h>
 
+#ifdef __CRIS__
+#define OUTGOING_SP_OFFSET (-sizeof (void *))
+/* Suggestion: append #elif defined(__<TARGET-IDENTIFIER>__) after this comment,
+   either defining OUTGOING_SP_OFFSET to whatever the pertinent amount is at -O2,
+   if that makes your target consistently fail this test, or define
+   DO_NOT_TAMPER for more complicated situations.  Either way, compile with
+   -DDO_NO_TAMPER to avoid any meddling.  */
+#endif
+
+#if defined (OUTGOING_SP_OFFSET) && !defined (DO_NOT_TAMPER)
+extern int xmain () __attribute__ ((__noipa__));
+int main ()
+{
+  uintptr_t misalignment
+    = (OUTGOING_SP_OFFSET
+        + (15 & (uintptr_t) __builtin_stack_address ()));
+  /* Allocate a minimal amount if the stack was accidentally aligned.  */
+  void *q = __builtin_alloca (misalignment == 0);
+  xmain ();
+  /* Fake use to avoid the "allocation" being optimized out.  */
+  asm volatile ("" : : "rm" (q));
+  return 0;
+}
+#define main xmain
+#endif
+
 struct U {
     int M0;
     int M1;