diff mbox series

Fix PR target/90458

Message ID 8221231.NyiUUSuA9g@fomalhaut
State Accepted
Headers
Series Fix PR target/90458 |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Eric Botcazou Feb. 15, 2023, 3:24 p.m. UTC 
  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.

Comments

Jeff Law Feb. 15, 2023, 7:05 p.m. UTC | #1 
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
NightStrike Feb. 16, 2023, 3:27 a.m. UTC | #2 
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
Eric Botcazou Feb. 16, 2023, 8:16 a.m. UTC | #3 
> This fixes dg.exp/stack-check-2.c, -7, 8, and -16.c, which is great!

Try the attached patch.
NightStrike Feb. 17, 2023, 8:56 a.m. UTC | #4 
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)?
Eric Botcazou Feb. 17, 2023, 9:18 a.m. UTC | #5 
> 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.
Jeff Law Feb. 17, 2023, 7:49 p.m. UTC | #6 
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.
Jeff Law March 11, 2023, 3:56 p.m. UTC | #7 
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
NightStrike March 12, 2023, 12:35 a.m. UTC | #8 
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
>
diff mbox series

Patch

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;