As PR rtl-optimization/64151 showed, the longjmp expansion on i386 is incorrect if the base register is spilled. It turns out it is trivial to write an example that reproduces this without my patch: void broken_longjmp(int x, void *buf[20]) { if (x == 0) return; __builtin_longjmp (buf, 1); } With -O2 this produces: pushl %ebp movl %esp, %ebp movl 8(%ebp), %eax testl %eax, %eax jne .L5 leave ret .L5: movl 12(%ebp), %eax movl 4(%eax), %eax movl 12(%ebp), %edx movl (%edx), %ebp *** load new ebp movl 12(%ebp), %ecx *** try to use old ebp movl 8(%ecx), %esp jmp *%eax
I don't think this test is valid. You can search for a builtin longjmp documentation bug of mine.
Dup of PR 59039?
(In reply to H.J. Lu from comment #2) > Dup of PR 59039? No that talks about not using __builtin_setjmp and __builtin_longjmp within the same function. I only used longjmp. Or are they so broken you need to put both the __builtin_longjmp and __builtin_setjmp in their own non-inlineable functions and make sure you never pass the jmpbuf via an argument? If so what is the point?
I can cause this to fail on AArch64 as well with a local buffer - the issue is blatantly obvious in the expansion of longjmp.
Author: law Date: Fri Nov 30 23:06:51 2018 New Revision: 266697 URL: https://gcc.gnu.org/viewcvs?rev=266697&root=gcc&view=rev Log: PR middle-end/64242 * builtins.c (expand_builtin_longjmp): Use a temporary when restoring the frame pointer. (expand_builtin_nonlocal_goto): Likewise. * gcc.c-torture/execute/pr64242.c: New test. Added: trunk/gcc/testsuite/gcc.c-torture/execute/pr64242.c Modified: trunk/gcc/ChangeLog trunk/gcc/builtins.c trunk/gcc/testsuite/ChangeLog
Fixed on trunk by Wilco's patch.
Hello, I can see the new test failing on arm-none-eabi when: - GCC is configured with default cpu/mode, QEMU simulates arm926 - GCC is configured --with-mode=thumb --with-cpu=cortex-m3, QEMU simulates cortex-m3 FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test I'm attaching the traces from QEMU, starting with main().
Created attachment 45137 [details] QEMU traces for --with-cpu=default / QEMU --cpu arm926
Created attachment 45138 [details] QEMU traces for --with-cpu=cortex-m3 / QEMU --cpu cortex-m3
The testcase also FAILs on x86_64-linux and i686-linux for me. The testcase is just invalid, you can't assume that __builtin_alloca (0) will not allocate anything on the stack at all, on many targets it does. E.g. on x86_64 it does: movslq x(%rip), %rax addq $23, %rax andq $-16, %rax subq %rax, %rsp leaq 15(%rsp), %rax andq $-16, %rax movq %rax, p(%rip) so for 0 it subtracts 16 bytes from %rsp. What was the failure on i386? I see .cfi_startproc pushl %ebp .cfi_def_cfa_offset 8 .cfi_offset 5, -8 movl %esp, %ebp .cfi_def_cfa_register 5 subl $32, %esp movl 8(%ebp), %eax movl (%eax), %edx movl 8(%eax), %ecx movl %edx, -20(%ebp) movl 4(%eax), %edx movl %ecx, -12(%ebp) movl 12(%eax), %ecx movl 16(%eax), %eax movl %edx, -16(%ebp) movl %ecx, -8(%ebp) movl %eax, -4(%ebp) movl -20(%ebp), %ebp movl -12(%ebp), %esp jmp *%edx .cfi_endproc At the start of the movl -20(%ebp), %ebp instruction both -12(%ebp) and -12(-20(%ebp)) contain the same value though, it has been stored there a few instructions earlier (movl %ecx, -12(%ebp) above it).
The testcase failed on many targets.
Reopen
I wonder about following, on i686-linux it FAILs with older trunk and succeeds with current trunk. Without the (useless) stack realignment the right value of stack pointer happened to be in exactly that slot from which it read memory. While still not fully portable, I think if the two alloca (0) are more than 1024 bytes appart, something is wrong with the target or at least alloca is helplessly expensive there. --- gcc/testsuite/gcc.c-torture/execute/pr64242.c 2018-12-01 00:25:08.082009500 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr64242.c 2018-12-03 16:43:33.343875994 +0100 @@ -11,20 +11,40 @@ broken_longjmp(void *p) __builtin_longjmp (buf, 1); } +__attribute ((noipa)) __UINTPTR_TYPE__ +foo(void *p) +{ + return (__UINTPTR_TYPE__) p; +} + +__attribute ((noipa)) void +bar(void *p) +{ + asm volatile ("" : : "r" (p)); +} + volatile int x = 0; -volatile void *p; +void *volatile p; +void *volatile q; int main (void) { void *buf[5]; + struct __attribute__((aligned (32))) S { int a[4]; } s; + bar (&s); p = __builtin_alloca (x); - if (!__builtin_setjmp (buf)) broken_longjmp (buf); /* Fails if stack pointer corrupted. */ - if (p != __builtin_alloca (x)) - abort(); + q = __builtin_alloca (x); + if (foo (p) < foo (q)) + { + if (foo (q) - foo (p) >= 1024) + abort (); + } + else if (foo (p) - foo (q) >= 1024) + abort (); return 0; }
(In reply to Jakub Jelinek from comment #13) > I wonder about following, on i686-linux it FAILs with older trunk and > succeeds with current trunk. Without the (useless) stack realignment the > right value of stack pointer happened to be in exactly that slot from which > it read memory. We could just increase the alloca size to 1 to avoid that (however there is always a possibility that the loaded value happens to be valid). > While still not fully portable, I think if the two alloca (0) are more than > 1024 bytes appart, something is wrong with the target or at least alloca is > helplessly expensive there. If alloca (x) always allocates extra bytes for no reason then that's a separate issue with alloca - I fixed this in the generic code last year. > --- gcc/testsuite/gcc.c-torture/execute/pr64242.c 2018-12-01 > 00:25:08.082009500 +0100 > +++ gcc/testsuite/gcc.c-torture/execute/pr64242.c 2018-12-03 > 16:43:33.343875994 +0100 > @@ -11,20 +11,40 @@ broken_longjmp(void *p) > __builtin_longjmp (buf, 1); > } > > +__attribute ((noipa)) __UINTPTR_TYPE__ > +foo(void *p) > +{ > + return (__UINTPTR_TYPE__) p; > +} > + > +__attribute ((noipa)) void > +bar(void *p) > +{ > + asm volatile ("" : : "r" (p)); > +} > + > volatile int x = 0; > -volatile void *p; > +void *volatile p; > +void *volatile q; > int > main (void) > { > void *buf[5]; > + struct __attribute__((aligned (32))) S { int a[4]; } s; > + bar (&s); Not sure what the purpose of this would be? > p = __builtin_alloca (x); > - > if (!__builtin_setjmp (buf)) > broken_longjmp (buf); > > /* Fails if stack pointer corrupted. */ > - if (p != __builtin_alloca (x)) > - abort(); > + q = __builtin_alloca (x); > + if (foo (p) < foo (q)) > + { > + if (foo (q) - foo (p) >= 1024) > + abort (); > + } > + else if (foo (p) - foo (q) >= 1024) > + abort (); I think that could just become __builtin_absl (foo (q) - foo (p)) > 64. > return 0; > }
Author: jakub Date: Mon Dec 3 20:57:14 2018 New Revision: 266766 URL: https://gcc.gnu.org/viewcvs?rev=266766&root=gcc&view=rev Log: PR middle-end/64242 * gcc.c-torture/execute/pr64242.c (foo, bar): New functions. (p): Make it void *volatile instead of volatile void *. (q): New variable. (main): Add a dummy 32-byte aligned variable and escape its address. Don't require that the two __builtin_alloca (0) calls return the same address, just require that their difference is smaller than 1024 bytes. Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.c-torture/execute/pr64242.c
(In reply to Christophe Lyon from comment #9) > Created attachment 45138 [details] > QEMU traces for --with-cpu=cortex-m3 / QEMU --cpu cortex-m3 Thanks, I can reproduce this now. It fails due to sched1 scheduling the move before the sp restore (correct at this point since it doesn't use a frame pointer) and reload then forward propagates a frame pointer use into it. Even if I add additional clobbers for sfp it won't block the substitution. So maybe we need an unspec around the address generation to be safe.
Or just emit a blockage insn to avoid the undesirable code motion.
(In reply to Jeffrey A. Law from comment #17) > Or just emit a blockage insn to avoid the undesirable code motion. I tried that already, it doesn't affect the forward substitution - I guess it simply assumes it is always valid, and a function never writes to SFP/SP/FP.
It's fails on mips64el-unknown-linux-gnu FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test FAIL: gcc.c-torture/execute/pr64242.c -Os execution test
The new testcase also FAILs on sparc-sun-solaris2.11 (both 32 and 64-bit): +FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test +FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto execution test +FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto -flto-partition=none execution test +FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test +FAIL: gcc.c-torture/execute/pr64242.c -Os execution test Thread 2 received signal SIGSEGV, Segmentation fault. [Switching to Thread 1 (LWP 1)] 0x00000008 in ?? () (gdb) where #0 0x00000008 in ?? () Backtrace stopped: previous frame identical to this frame (corrupt stack?) Single-stepping, I find that this happens at the very end of main: 1: x/i $pc => 0x10de4 <main+268>: return %i7 + 8 (gdb) 0x00010de8 in main () at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.c-torture/execute/pr64242.c:50 50 return 0; 1: x/i $pc => 0x10de8 <main+272>: nop (gdb) 0x00000008 in ?? () 1: x/i $pc => 0x8: <error: Cannot access memory at address 0x8> Obviously the stack is corrupted beyond repair. I tried to avoid this by replacing the return 0 with exit (0) to no avail. The original testcase (before Jakub's patch) would abort instead: Thread 2 received signal SIGABRT, Aborted. [Switching to Thread 1 (LWP 1)] 0xfec7e044 in __lwp_sigqueue () from /lib/libc.so.1 (gdb) where #0 0xfec7e044 in __lwp_sigqueue () from /lib/libc.so.1 #1 0xfebb9898 in raise () from /lib/libc.so.1 #2 0xfeb8b1d0 in abort () from /lib/libc.so.1 #3 0x00010ce8 in main () at /homes/ro/pr64242.c:27
(In reply to Rainer Orth from comment #20) > The new testcase also FAILs on sparc-sun-solaris2.11 (both 32 and 64-bit): > > +FAIL: gcc.c-torture/execute/pr64242.c -O2 execution test > +FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto execution test > +FAIL: gcc.c-torture/execute/pr64242.c -O2 -flto -flto-partition=none > execution test > +FAIL: gcc.c-torture/execute/pr64242.c -O3 -g execution test > +FAIL: gcc.c-torture/execute/pr64242.c -Os execution test > > Thread 2 received signal SIGSEGV, Segmentation fault. > [Switching to Thread 1 (LWP 1)] > 0x00000008 in ?? () > (gdb) where > #0 0x00000008 in ?? () > Backtrace stopped: previous frame identical to this frame (corrupt stack?) > > Single-stepping, I find that this happens at the very end of main: > > 1: x/i $pc > => 0x10de4 <main+268>: return %i7 + 8 > (gdb) > 0x00010de8 in main () > at > /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.c-torture/execute/pr64242.c:50 > 50 return 0; > 1: x/i $pc > => 0x10de8 <main+272>: nop > (gdb) > 0x00000008 in ?? () > 1: x/i $pc > => 0x8: <error: Cannot access memory at address 0x8> > > Obviously the stack is corrupted beyond repair. I tried to avoid this by > replacing the return 0 with exit (0) to no avail. My latest patch detects this stack corruption with 100% certainty again, see https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00459.html. However sparc has a custom nonlocal_goto MD pattern which would need fixing too.
Also fails on hppa*-*-*.
*** Bug 89837 has been marked as a duplicate of this bug. ***
Author: wilco Date: Mon Jun 3 13:55:15 2019 New Revision: 271870 URL: https://gcc.gnu.org/viewcvs?rev=271870&root=gcc&view=rev Log: Fix PR64242 - Longjmp expansion incorrect Improve the fix for PR64242. Various optimizations can change a memory reference into a frame access. Given there are multiple virtual frame pointers which may be replaced by multiple hard frame pointers, there are no checks for writes to the various frame pointers. So updates to a frame pointer tends to generate incorrect code. Improve the previous fix to also add clobbers of several frame pointers and add a scheduling barrier. This should work in most cases until GCC supports a generic "don't optimize across this instruction" feature. Bootstrap OK. Testcase passes on AArch64 and x86-64. Inspected x86, Arm, Thumb-1 and Thumb-2 assembler which looks correct. gcc/ PR middle-end/64242 * builtins.c (expand_builtin_longjmp): Add frame clobbers and schedule block. (expand_builtin_nonlocal_goto): Likewise. testsuite/ PR middle-end/64242 * gcc.c-torture/execute/pr64242.c: Update test. Modified: trunk/gcc/ChangeLog trunk/gcc/builtins.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.c-torture/execute/pr64242.c
I believe this is now fixed for generic code - however targets which implement the nonlocal_goto expander (eg. pa, sparc, vax) need similar fixes in their backends.
On 2019-06-10 9:51 a.m., wilco at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64242 > > --- Comment #25 from Wilco <wilco at gcc dot gnu.org> --- > I believe this is now fixed for generic code - however targets which implement > the nonlocal_goto expander (eg. pa, sparc, vax) need similar fixes in their > backends. > The test no longer fails on pa. We need a test that demonstrates the problem in the current pa expander.
(In reply to dave.anglin from comment #26) > On 2019-06-10 9:51 a.m., wilco at gcc dot gnu.org wrote: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64242 > > > > --- Comment #25 from Wilco <wilco at gcc dot gnu.org> --- > > I believe this is now fixed for generic code - however targets which implement > > the nonlocal_goto expander (eg. pa, sparc, vax) need similar fixes in their > > backends. > > > The test no longer fails on pa. We need a test that demonstrates the > problem in the current > pa expander. It allocates the buf variables at exactly the same offset in both frames, so the code below accidentally reads the right values from the wrong address: broken_longjmp: .PROC .CALLINFO FRAME=192,CALLS,SAVE_RP,SAVE_SP,ENTRY_GR=3 .ENTRY copy %r3,%r1 stw %r2,-20(%r30) copy %r30,%r3 stwm %r1,192(%r30) copy %r26,%r25 ldi 20,%r24 bl memcpy,%r2 ldo 8(%r3),%r26 ldw 8(%r3),%r28 -> load frame pointer of parent function ldo -8(%r28),%r3 -> set hard_frame_pointer ldw 16(%r3),%r30 -> use wrong frame pointer to read stack pointer ldw 12(%r3),%r1 -> use wrong frame pointer to read return address be,n 0(%sr4,%r1) Given there are many possible stack layouts, the easiest option would be to clear the input buffer so it will jump to a null pointer. Eg. __attribute ((noinline)) void broken_longjmp (void *p) { void *buf[32]; __builtin_memcpy (buf, p, 5 * sizeof (void*)); __builtin_memset (p, 0, 5 * sizeof (void*)); /* Corrupts stack pointer... */ __builtin_longjmp (buf, 1); }
On 2019-06-14 10:13 a.m., wilco at gcc dot gnu.org wrote: > Given there are many possible stack layouts, the easiest option would be to > clear the input buffer so it will jump to a null pointer. Eg. > > __attribute ((noinline)) void > broken_longjmp (void *p) > { > void *buf[32]; > __builtin_memcpy (buf, p, 5 * sizeof (void*)); > __builtin_memset (p, 0, 5 * sizeof (void*)); > /* Corrupts stack pointer... */ > __builtin_longjmp (buf, 1); > } Yes, the above fixes test. I think I have a fix for the pa longjmp and nonlocal_goto expanders.
Author: danglin Date: Sun Jun 16 21:27:14 2019 New Revision: 272361 URL: https://gcc.gnu.org/viewcvs?rev=272361&root=gcc&view=rev Log: PR middle-end/64242 * config/pa/pa.md (nonlocal_goto): Restore frame pointer last. Add frame clobbers and schedule block. (builtin_longjmp): Likewise. Modified: trunk/gcc/ChangeLog trunk/gcc/config/pa/pa.md
Author: danglin Date: Sun Jun 16 21:44:08 2019 New Revision: 272362 URL: https://gcc.gnu.org/viewcvs?rev=272362&root=gcc&view=rev Log: PR middle-end/64242 * config/pa/pa.md (nonlocal_goto): Restore frame pointer last. Add frame clobbers and schedule block. (builtin_longjmp): Likewise. Modified: branches/gcc-9-branch/gcc/ChangeLog branches/gcc-9-branch/gcc/config/pa/pa.md
Author: danglin Date: Sun Jun 16 21:46:47 2019 New Revision: 272363 URL: https://gcc.gnu.org/viewcvs?rev=272363&root=gcc&view=rev Log: PR middle-end/64242 * config/pa/pa.md (nonlocal_goto): Restore frame pointer last. Add frame clobbers and schedule block. (builtin_longjmp): Likewise. Modified: branches/gcc-8-branch/gcc/ChangeLog branches/gcc-8-branch/gcc/config/pa/pa.md
Author: danglin Date: Sun Jun 16 21:49:19 2019 New Revision: 272364 URL: https://gcc.gnu.org/viewcvs?rev=272364&root=gcc&view=rev Log: PR middle-end/64242 * config/pa/pa.md (nonlocal_goto): Restore frame pointer last. Add frame clobbers and schedule block. (builtin_longjmp): Likewise. Modified: branches/gcc-7-branch/gcc/ChangeLog branches/gcc-7-branch/gcc/config/pa/pa.md
Author: wilco Date: Mon Jun 17 11:25:12 2019 New Revision: 272382 URL: https://gcc.gnu.org/viewcvs?rev=272382&root=gcc&view=rev Log: Improve PR64242 testcase Clear the input array to avoid the testcase accidentally passing with an incorrect frame pointer. Committed as obvious. testsuite/ PR middle-end/64242 * gcc.c-torture/execute/pr64242.c: Improve test. Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.c-torture/execute/pr64242.c
Author: ebotcazou Date: Mon Jul 1 16:26:38 2019 New Revision: 272889 URL: https://gcc.gnu.org/viewcvs?rev=272889&root=gcc&view=rev Log: PR middle-end/64242 * config/sparc/sparc.md (nonlocal_goto): Restore frame pointer last. Add frame clobber and schedule blockage. Modified: trunk/gcc/ChangeLog trunk/gcc/config/sparc/sparc.md
Author: ebotcazou Date: Mon Jul 1 16:27:06 2019 New Revision: 272890 URL: https://gcc.gnu.org/viewcvs?rev=272890&root=gcc&view=rev Log: PR middle-end/64242 * config/sparc/sparc.md (nonlocal_goto): Restore frame pointer last. Add frame clobber and schedule blockage. Modified: branches/gcc-9-branch/gcc/ChangeLog branches/gcc-9-branch/gcc/config/sparc/sparc.md
MIPS is still broken. I might look into MIPS brokenness next week.
(In reply to Andrew Pinski from comment #36) > MIPS is still broken. I might look into MIPS brokenness next week. Yes it seems builtin_longjmp has the exact same fp corruption issue: move $fp,$17 lw $sp,8($fp) jr $16 lw $28,12($fp)
(In reply to Wilco from comment #37) > (In reply to Andrew Pinski from comment #36) > > MIPS is still broken. I might look into MIPS brokenness next week. > > Yes it seems builtin_longjmp has the exact same fp corruption issue: > > move $fp,$17 > lw $sp,8($fp) > jr $16 > lw $28,12($fp) Yes the same issue. I will be testing a patch soon.
Created attachment 47830 [details] MIPS patch This is the MIPS patch, I will post it by the end of next week.
(In reply to Andrew Pinski from comment #39) > Created attachment 47830 [details] > MIPS patch > > This is the MIPS patch, I will post it by the end of next week. any update ?
The master branch has been updated by Chenghua Xu <paulhua@gcc.gnu.org>: https://gcc.gnu.org/g:68e605c93d57c17f07edd50f7e1c80f9216befd2 commit r11-2835-g68e605c93d57c17f07edd50f7e1c80f9216befd2 Author: Andrew Pinski <apinski@marvell.com> Date: Tue Aug 25 14:17:52 2020 +0800 MIPS: Fix __builtin_longjmp (PR 64242) The problem here is mips has its own builtin_longjmp pattern and it was not fixed when expand_builtin_longjmp was fixed. We need to read the new fp and gp before restoring the stack as the buffer might be a local variable. 2020-08-25 Andrew Pinski <apinski@marvell.com> gcc/ChangeLog: PR middle-end/64242 * config/mips/mips.md (builtin_longjmp): Restore the frame pointer and stack pointer and gp.