[PATCH] Fix PR target/90458

[PATCH] Fix PR target/90458

[Eric Botcazou]

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

Hi,

this is the incompatibility of -fstack-clash-protection with Windows SEH.  Now 
the Windows ports always enable TARGET_STACK_PROBE, which means that the stack 
is always probed (out of line) so -fstack-clash-protection does nothing more.

Tested on x86-64/Windows and Linux, OK for all active branches?


2023-02-15  Eric Botcazou  <ebotcazou@adacore.com>

	* config/i386/i386.cc (ix86_compute_frame_layout): Disable the
	effects of -fstack-clash-protection for TARGET_STACK_PROBE.
	(ix86_expand_prologue): Likewise.

-- 
Eric Botcazou

[-- Attachment #2: pr90458.diff --]
[-- Type: text/x-patch, Size: 1186 bytes --]

diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
index 3cacf738c4a..22f444be23c 100644
--- a/gcc/config/i386/i386.cc
+++ b/gcc/config/i386/i386.cc
@@ -6876,7 +6876,9 @@ ix86_compute_frame_layout (void)
 	 stack clash protections are enabled and the allocated frame is
 	 larger than the probe interval, then use pushes to save
 	 callee saved registers.  */
-      || (flag_stack_clash_protection && to_allocate > get_probe_interval ()))
+      || (flag_stack_clash_protection
+	  && !ix86_target_stack_probe ()
+	  && to_allocate > get_probe_interval ()))
     frame->save_regs_using_mov = false;
 
   if (ix86_using_red_zone ()
@@ -8761,8 +8763,11 @@ ix86_expand_prologue (void)
       sse_registers_saved = true;
     }
 
-  /* If stack clash protection is requested, then probe the stack.  */
-  if (allocate >= 0 && flag_stack_clash_protection)
+  /* If stack clash protection is requested, then probe the stack, unless it
+     is already probed on the target.  */
+  if (allocate >= 0
+      && flag_stack_clash_protection
+      && !ix86_target_stack_probe ())
     {
       ix86_adjust_stack_and_probe (allocate, int_registers_saved, false);
       allocate = 0;

Re: [PATCH] Fix PR target/90458

[Jeff Law]

On 2/15/23 08:24, Eric Botcazou via Gcc-patches wrote:
> Hi,
> 
> this is the incompatibility of -fstack-clash-protection with Windows SEH.  Now
> the Windows ports always enable TARGET_STACK_PROBE, which means that the stack
> is always probed (out of line) so -fstack-clash-protection does nothing more.
> 
> Tested on x86-64/Windows and Linux, OK for all active branches?
> 
> 
> 2023-02-15  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* config/i386/i386.cc (ix86_compute_frame_layout): Disable the
> 	effects of -fstack-clash-protection for TARGET_STACK_PROBE.
> 	(ix86_expand_prologue): Likewise.
> 
OK.  THanks for taking care of this.  I let it languish far too long.

jeff

Re: [PATCH] Fix PR target/90458

[NightStrike]

On Wed, Feb 15, 2023 at 10:24 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> this is the incompatibility of -fstack-clash-protection with Windows SEH.  Now
> the Windows ports always enable TARGET_STACK_PROBE, which means that the stack
> is always probed (out of line) so -fstack-clash-protection does nothing more.
>
> Tested on x86-64/Windows and Linux, OK for all active branches?
>
>
> 2023-02-15  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * config/i386/i386.cc (ix86_compute_frame_layout): Disable the
>         effects of -fstack-clash-protection for TARGET_STACK_PROBE.
>         (ix86_expand_prologue): Likewise.

This fixes dg.exp/stack-check-2.c, -7, 8, and -16.c, which is great!

-6 no longer ICEs, but still fails.

-3, -4, -5, and -9 didn't ICE, and still fail:

gcc.dg/stack-check-10.c: pattern found 0 times
FAIL: gcc.dg/stack-check-10.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no probe" 2
gcc.dg/stack-check-10.c: pattern found 0 times
FAIL: gcc.dg/stack-check-10.c scan-rtl-dump-times pro_and_epilogue
"Stack clash not noreturn" 2
gcc.dg/stack-check-10.c: pattern found 0 times
FAIL: gcc.dg/stack-check-10.c scan-rtl-dump-times pro_and_epilogue
"Stack clash residual allocation in prologue" 2
gcc.dg/stack-check-10.c: pattern found 0 times
FAIL: gcc.dg/stack-check-10.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no frame pointer needed" 2
gcc.dg/stack-check-3.c: pattern found 0 times
FAIL: gcc.dg/stack-check-3.c scan-rtl-dump-times expand "allocation
and probing residuals" 7
gcc.dg/stack-check-3.c: pattern found 0 times
FAIL: gcc.dg/stack-check-3.c scan-rtl-dump-times expand "allocation
and probing in loop" 7
gcc.dg/stack-check-4.c: pattern found 0 times
FAIL: gcc.dg/stack-check-4.c scan-rtl-dump-times pro_and_epilogue
"Stack clash noreturn" 1
gcc.dg/stack-check-4.c: pattern found 0 times
FAIL: gcc.dg/stack-check-4.c scan-rtl-dump-times pro_and_epilogue
"Stack clash not noreturn" 1
gcc.dg/stack-check-5.c: pattern found 0 times
FAIL: gcc.dg/stack-check-5.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no probe" 4
gcc.dg/stack-check-5.c: pattern found 0 times
FAIL: gcc.dg/stack-check-5.c scan-rtl-dump-times pro_and_epilogue
"Stack clash not noreturn" 4
gcc.dg/stack-check-5.c: pattern found 0 times
FAIL: gcc.dg/stack-check-5.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no frame pointer needed" 4
gcc.dg/stack-check-5.c: pattern found 0 times
FAIL: gcc.dg/stack-check-5.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no residual allocation in prologue" 1
gcc.dg/stack-check-5.c: pattern found 0 times
FAIL: gcc.dg/stack-check-5.c scan-rtl-dump-times pro_and_epilogue
"Stack clash residual allocation in prologue" 3
gcc.dg/stack-check-6.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6.c scan-rtl-dump-times pro_and_epilogue
"Stack clash inline probes" 2
gcc.dg/stack-check-6.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6.c scan-rtl-dump-times pro_and_epilogue
"Stack clash probe loop" 2
gcc.dg/stack-check-6.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6.c scan-rtl-dump-times pro_and_epilogue
"Stack clash residual allocation in prologue" 4
gcc.dg/stack-check-6.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6.c scan-rtl-dump-times pro_and_epilogue
"Stack clash not noreturn" 4
gcc.dg/stack-check-6.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no frame pointer needed" 4
gcc.dg/stack-check-6a.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6a.c scan-rtl-dump-times pro_and_epilogue
"Stack clash residual allocation in prologue" 4
gcc.dg/stack-check-6a.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6a.c scan-rtl-dump-times pro_and_epilogue
"Stack clash not noreturn" 4
gcc.dg/stack-check-6a.c: pattern found 0 times
FAIL: gcc.dg/stack-check-6a.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no frame pointer needed" 4
gcc.dg/stack-check-9.c: pattern found 0 times
FAIL: gcc.dg/stack-check-9.c scan-rtl-dump-times pro_and_epilogue
"Stack clash inline probes" 1
gcc.dg/stack-check-9.c: pattern found 0 times
FAIL: gcc.dg/stack-check-9.c scan-rtl-dump-times pro_and_epilogue
"Stack clash residual allocation in prologue" 1
gcc.dg/stack-check-9.c: pattern found 0 times
FAIL: gcc.dg/stack-check-9.c scan-rtl-dump-times pro_and_epilogue
"Stack clash not noreturn" 1
gcc.dg/stack-check-9.c: pattern found 0 times
FAIL: gcc.dg/stack-check-9.c scan-rtl-dump-times pro_and_epilogue
"Stack clash no frame pointer needed" 1


target/i386.exp/stack-check-12.c no longer ICEs but still fails.

-11, -18 and -19 still fail:

gcc.target/i386/stack-check-11.c: sub[ql] found 1 times
FAIL: gcc.target/i386/stack-check-11.c scan-assembler-times sub[ql] 4
gcc.target/i386/stack-check-11.c: or[ql] found 0 times
FAIL: gcc.target/i386/stack-check-11.c scan-assembler-times or[ql] 3

ia32882847.c:2:13: error: size of array 'dummy' is negative^M
ia32882847.c:4:55: error: '__i386__' undeclared here (not in a function)^M
compiler exited with status 1
FAIL: gcc.target/i386/stack-check-12.c scan-assembler pushq\t%rax
FAIL: gcc.target/i386/stack-check-12.c scan-assembler popq\t%rax

gcc.target/i386/stack-check-18.c: pattern found 0 times
FAIL: gcc.target/i386/stack-check-18.c scan-rtl-dump-times expand
"allocation and probing in loop" 1
gcc.target/i386/stack-check-18.c: pattern found 0 times
FAIL: gcc.target/i386/stack-check-18.c scan-rtl-dump-times expand
"allocation and probing residuals" 1
gcc.target/i386/stack-check-18.c: or[ql] found 0 times
FAIL: gcc.target/i386/stack-check-18.c scan-assembler-times or[ql] 1

gcc.target/i386/stack-check-19.c: pattern found 0 times
FAIL: gcc.target/i386/stack-check-19.c scan-rtl-dump-times expand
"allocation and probing in loop" 1
gcc.target/i386/stack-check-19.c: pattern found 0 times
FAIL: gcc.target/i386/stack-check-19.c scan-rtl-dump-times expand
"allocation and probing residuals" 1
gcc.target/i386/stack-check-19.c: or[ql] found 0 times
FAIL: gcc.target/i386/stack-check-19.c scan-assembler-times or[ql] 2
gcc.target/i386/stack-check-19.c: (?:je|jne) found 0 times
FAIL: gcc.target/i386/stack-check-19.c scan-assembler-times (?:je|jne) 3

Re: [PATCH] Fix PR target/90458

[Eric Botcazou]

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

> This fixes dg.exp/stack-check-2.c, -7, 8, and -16.c, which is great!

Try the attached patch.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 677 bytes --]

diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 227e3004077..d4f036a3f1e 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -11655,6 +11655,11 @@ proc check_effective_target_autoincdec { } {
 # 
 proc check_effective_target_supports_stack_clash_protection { } {
 
+    # Stack probing is done unconditionally out-of-line on Windows
+    if { [istarget *-*-cygwin*] || [istarget *-*-mingw*] } {
+	return 0
+    }
+
     if { [istarget x86_64-*-*] || [istarget i?86-*-*] 
 	  || [istarget powerpc*-*-*] || [istarget rs6000*-*-*]
 	  || [istarget aarch64*-**] || [istarget s390*-*-*]

Re: [PATCH] Fix PR target/90458

[NightStrike]

On Thu, Feb 16, 2023 at 3:16 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > This fixes dg.exp/stack-check-2.c, -7, 8, and -16.c, which is great!
>
> Try the attached patch.

Well... that patch just marks all of the tests as unsupported.  But
for example, the ones quoted above run, work, and pass.  And when they
didn't pass, they highlighted the ICE that you fixed.  So running the
test despite the nature of stack protection on Windows still has
value.  It is useful to ensure for example that stack protection
continues to work even if options are passed to disable it.  But the
tests that remain are looking for rtl patterns that (I assume)
shouldn't exist.  So it's perhaps useful to modify the test to say
that on windows, the scan-rtl-dump-times check should have zero hits.
If it found a match, that would be an error.

Is there a way to say that the test results should be inverted on a
particular platform (instead of purely unsupported)?

Re: [PATCH] Fix PR target/90458

[Eric Botcazou]

> Is there a way to say that the test results should be inverted on a
> particular platform (instead of purely unsupported)?

Yes, you can do pretty much what you want with the testsuite harness.

-- 
Eric Botcazou

Re: [PATCH] Fix PR target/90458

[Jeff Law]

On 2/17/23 01:56, NightStrike via Gcc-patches wrote:
> On Thu, Feb 16, 2023 at 3:16 AM Eric Botcazou <botcazou@adacore.com> wrote:
>>
>>> This fixes dg.exp/stack-check-2.c, -7, 8, and -16.c, which is great!
>>
>> Try the attached patch.
> 
> Well... that patch just marks all of the tests as unsupported.  But
> for example, the ones quoted above run, work, and pass.  And when they
> didn't pass, they highlighted the ICE that you fixed.  So running the
> test despite the nature of stack protection on Windows still has
> value.  It is useful to ensure for example that stack protection
> continues to work even if options are passed to disable it.  But the
> tests that remain are looking for rtl patterns that (I assume)
> shouldn't exist.  So it's perhaps useful to modify the test to say
> that on windows, the scan-rtl-dump-times check should have zero hits.
> If it found a match, that would be an error.
Those tests may compile, work and pass, but they're really there to 
check the probing decisions.  Windows has a totally different model and 
none of those tests really make sense in that model.
> 
> Is there a way to say that the test results should be inverted on a
> particular platform (instead of purely unsupported)?
You can express all kinds of things, I'm just not sure it's worth the 
effort for these tests.

Re: [PATCH] Fix PR target/90458

[Jeff Law]

On 2/16/23 01:16, Eric Botcazou via Gcc-patches wrote:
>> This fixes dg.exp/stack-check-2.c, -7, 8, and -16.c, which is great!
> 
> Try the attached patch.
I'd suggest going ahead and applying the fix to 
check_effective_target_supports_stack_clash_protection.

There's really not a strong reason to run those tests on a cygwin or 
mingw target given how probing is done (out of line).

Jeff

Re: [PATCH] Fix PR target/90458

[NightStrike]

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

The reason would be to show that they continue to not ICE as they used to.
No go paths are just as useful as go paths.

On Sat, Mar 11, 2023, 10:57 Jeff Law <jeffreyalaw@gmail.com> wrote:

>
>
> On 2/16/23 01:16, Eric Botcazou via Gcc-patches wrote:
> >> This fixes dg.exp/stack-check-2.c, -7, 8, and -16.c, which is great!
> >
> > Try the attached patch.
> I'd suggest going ahead and applying the fix to
> check_effective_target_supports_stack_clash_protection.
>
> There's really not a strong reason to run those tests on a cygwin or
> mingw target given how probing is done (out of line).
>
> Jeff
>