| Summary: | ppc64 glibc miscompilation | ||
|---|---|---|---|
| Product: | gcc | Reporter: | Jakub Jelinek <jakub> |
| Component: | target | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | amodra, bergner, dje, gcc-bugs, meissner, pthaugen, siarhei.siamashka |
| Priority: | P3 | Keywords: | wrong-code |
| Version: | 4.6.0 | ||
| Target Milestone: | --- | ||
| Host: | Target: | powerpc64-linux | |
| Build: | Known to work: | ||
| Known to fail: | Last reconfirmed: | ||
| Attachments: | gcc46-pr44199.patch | ||
*** Bug 44200 has been marked as a duplicate of this bug. *** *** Bug 44201 has been marked as a duplicate of this bug. *** Yes, it is related, but solving it in the scheduler in generic way isn't going to be trivial.
E.g. x86_64 emits memory_blockage early in ix86_expand_epilogue:
/* See the comment about red zone and frame
pointer usage in ix86_expand_prologue. */
if (frame_pointer_needed && frame.red_zone_size)
emit_insn (gen_memory_blockage ());
Another testcase:
extern void *alloca (__SIZE_TYPE__);
__attribute__((noinline, noclone))
long *bar (long *p)
{
asm volatile ("" : : "r" (p) : "memory");
return p;
}
long
foo (long x)
{
long *p = (long *) alloca (x * sizeof (long));
long *q = bar (p);
return q[0];
}
which shows that some blockage or preventing scheduling over the stack release is needed even when the frame size is smaller than the red zone size and that the memory address doesn't need to be obviously based on stack pointer (it would be just safe to allow moving over stack accesses that are provably in the red zone or above.
rs6000.c already has rs6000_emit_stack_tie and calls it in several places in the epilogue generation, I guess we just should add another call to this. Created attachment 20705 [details] gcc46-pr44199.patch Untested patch. Unfortunately, rs6000_emit_stack_tie isn't good enough. 1) it uses frame alias set, but the stack block can have arbitrary alias sets in it and we need to avoid moving all of them over 2) for the frame_pointer_needed case using sp based BLK mem isn't sufficient, as then fp based mem can still be scheduled over it. Using stack_tie insn just with hard frame pointer based BLK mem doesn't work either, then the sp restore insn can be moved over it. Could anyone please test this on SPEC to see if it causes meassurable differences? Pat is going to SPEC test the patch and will report back here with his results. FWIW, Jakub's patch looks a reasonable fix to me. Spec testing went fine, differences in the noise range. Have you also bootstrapped/regtested the patch? I can do so (easily for 4.4 branch, with more effort for the trunk), but haven't done that yet. No I didn't bootstrap/regtest. I can take care of trunk if you want to do 4.4. If you could, it would be very much appreciated. Starting 4.4 bootstrap/regtest now. Bootstrap of trunk went fine. Regression test (contrib/test_summary) showed the following difference between base/patched versions, but didn't have any testcases show up in the diff, so not sure what to make of that. This is for 32-bit gcc testsuite. < # of expected passes 59078 --- > # of expected passes 59075 98c98 < # of unsupported tests 872 --- > # of unsupported tests 875 For me it bootstrapped/regtested on 4.4 branch without any testsuite changes (both 32-bit and 64-bit). To see the unsupported tests difference, you can grep ^UNSUPPORTED gcc/testsuite/gcc/gcc.log | sort between the unpatched/patched builds and see what the differences are I guess. I also did a powerpc64-linux bootstrap and regtest (both 32-bit and 64-bit) and I didn't see any new failures and I also did not see any extra UNSUPPORTED tests. The only time UNSUPPORTED showed up in the test_summary output (UNRESOLVED: one_time_plugin.c compilation,...) was due to different source directory paths. is : http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44229 potentially a similar problem? > is : http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44229 > potentially a similar problem? It does not look like: the patch in comment #6 does not fix pr44229. Subject: Bug 44199 Author: jakub Date: Wed May 26 06:00:44 2010 New Revision: 159853 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159853 Log: PR target/44199 * config/rs6000/rs6000.c (rs6000_emit_epilogue): If cfun->calls_alloca or total_size is larger than red zone size for non-V4 ABI, emit a stack_tie resp. frame_tie insn before stack pointer restore. * config/rs6000/rs6000.md (frame_tie): New insn. Modified: trunk/gcc/ChangeLog trunk/gcc/config/rs6000/rs6000.c trunk/gcc/config/rs6000/rs6000.md Subject: Bug 44199 Author: jakub Date: Wed May 26 06:02:30 2010 New Revision: 159854 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159854 Log: PR target/44199 * config/rs6000/rs6000.c (rs6000_emit_epilogue): If cfun->calls_alloca or total_size is larger than red zone size for non-V4 ABI, emit a stack_tie resp. frame_tie insn before stack pointer restore. * config/rs6000/rs6000.md (frame_tie): New insn. Modified: branches/gcc-4_5-branch/gcc/ChangeLog branches/gcc-4_5-branch/gcc/config/rs6000/rs6000.c branches/gcc-4_5-branch/gcc/config/rs6000/rs6000.md Subject: Bug 44199 Author: jakub Date: Wed May 26 06:05:29 2010 New Revision: 159855 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159855 Log: PR target/44199 * config/rs6000/rs6000.c (rs6000_emit_epilogue): If cfun->calls_alloca or total_size is larger than red zone size for non-V4 ABI, emit a stack_tie resp. frame_tie insn before stack pointer restore. * config/rs6000/rs6000.md (frame_tie): New insn. Modified: branches/gcc-4_4-branch/gcc/ChangeLog branches/gcc-4_4-branch/gcc/config/rs6000/rs6000.c branches/gcc-4_4-branch/gcc/config/rs6000/rs6000.md Should be fixed now. The 4.4 patch isn't complete. /home/gccbuild/gcc_4.4_anonsvn/gcc/gcc/config/rs6000/rs6000.c:17166: undefined reference to `offset_below_red_zone_p' /home/gccbuild/gcc_4.4_anonsvn/gcc/gcc/config/rs6000/rs6000.c:17188: undefined reference to `offset_below_red_zone_p' Subject: Bug 44199 Author: jakub Date: Wed May 26 16:09:25 2010 New Revision: 159878 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159878 Log: PR target/44199 * config/rs6000/rs6000.c (rs6000_emit_epilogue): Fix up a backport glitch. Modified: branches/gcc-4_4-branch/gcc/config/rs6000/rs6000.c Oops sorry, forgot redhat/gcc-4_4-branch has this function backported. Fixed now. Subject: Bug 44199 Author: bergner Date: Thu May 27 16:31:05 2010 New Revision: 159930 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159930 Log: Backport from mainline: 2010-05-26 Jakub Jelinek <jakub@redhat.com> PR target/44199 * config/rs6000/rs6000.c (rs6000_emit_epilogue): If cfun->calls_alloca or total_size is larger than red zone size for non-V4 ABI, emit a stack_tie resp. frame_tie insn before stack pointer restore. * config/rs6000/rs6000.md (frame_tie): New insn. Modified: branches/ibm/gcc-4_4-branch/gcc/ChangeLog.ibm branches/ibm/gcc-4_4-branch/gcc/config/rs6000/rs6000.c branches/ibm/gcc-4_4-branch/gcc/config/rs6000/rs6000.md Subject: Bug 44199 Author: bergner Date: Wed Jun 2 15:40:09 2010 New Revision: 160160 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=160160 Log: Backport from GCC 4.4: 2010-05-26 Jakub Jelinek <jakub@redhat.com> PR target/44199 * config/rs6000/rs6000.c (rs6000_emit_epilogue): Fix up a backport glitch. Modified: branches/ibm/gcc-4_4-branch/gcc/ChangeLog.ibm branches/ibm/gcc-4_4-branch/gcc/config/rs6000/rs6000.c |
vfscanf is miscompiled by gcc 4.4, here is a shorter testcase that is miscompiled also by 4.6: extern void *alloca (__SIZE_TYPE__); extern void bar (long *, char *); long foo (long x) { long buf[400]; char *p = alloca (x); bar (buf, p); return buf[0]; } at -O2 -m64 results in: ... bl .bar nop addi 1,31,3328 ld 3,112(31) ld 0,16(1) ld 31,-8(1) mtlr 0 blr Note that sched2 swapped incorrectly the stack frame release and ld 3,112(31), so the ld insn reads from memory location 3216 below the stack (which is much lower than 288 bytes of ppc64 red zone). If a signal comes in in between addi and ld and something clobbers the stack, this function (or, with 4.4 vfscanf) will return incorrect value. There is nothing in the epilogue stack release insn that would make it a memory barrier.