GCC Bugzilla has been upgraded from version 4.4.9 to 5.0rc3. If you see any problem, please report it to bug 64968.
Bug 44199 - ppc64 glibc miscompilation
Summary: ppc64 glibc miscompilation
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
: 44200 44201 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-05-19 18:26 UTC by Jakub Jelinek
Modified: 2010-05-26 16:10 UTC (History)
7 users (show)

See Also:
Host:
Target: powerpc64-linux
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
gcc46-pr44199.patch (755 bytes, patch)
2010-05-19 20:31 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2010-05-19 18:26:17 UTC
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.
Comment 1 Andrew Pinski 2010-05-19 18:30:26 UTC
Looks related to PR 30282.
Comment 2 Jakub Jelinek 2010-05-19 19:09:09 UTC
*** Bug 44200 has been marked as a duplicate of this bug. ***
Comment 3 Jakub Jelinek 2010-05-19 19:09:44 UTC
*** Bug 44201 has been marked as a duplicate of this bug. ***
Comment 4 Jakub Jelinek 2010-05-19 19:23:47 UTC
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.
Comment 5 Jakub Jelinek 2010-05-19 19:27:27 UTC
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.
Comment 6 Jakub Jelinek 2010-05-19 20:31:37 UTC
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?
Comment 7 Peter Bergner 2010-05-19 21:52:55 UTC
Pat is going to SPEC test the patch and will report back here with his results.
Comment 8 Alan Modra 2010-05-20 04:31:17 UTC
FWIW, Jakub's patch looks a reasonable fix to me.
Comment 9 Pat Haugen 2010-05-20 16:23:04 UTC
Spec testing went fine, differences in the noise range.
Comment 10 Jakub Jelinek 2010-05-20 16:31:34 UTC
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.
Comment 11 Pat Haugen 2010-05-20 17:59:36 UTC
No I didn't bootstrap/regtest. I can take care of trunk if you want to do 4.4.
Comment 12 Jakub Jelinek 2010-05-20 18:23:45 UTC
If you could, it would be very much appreciated.  Starting 4.4 bootstrap/regtest now.
Comment 13 Pat Haugen 2010-05-21 02:32:44 UTC
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
Comment 14 Jakub Jelinek 2010-05-21 08:08:10 UTC
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.
Comment 15 Peter Bergner 2010-05-21 19:14:38 UTC
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.
Comment 16 Iain Sandoe 2010-05-21 19:24:17 UTC
is : http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44229
potentially a similar problem?
Comment 17 Dominique d'Humieres 2010-05-22 13:31:04 UTC
> 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.
Comment 18 Jakub Jelinek 2010-05-26 06:01:11 UTC
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

Comment 19 Jakub Jelinek 2010-05-26 06:02:53 UTC
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

Comment 20 Jakub Jelinek 2010-05-26 06:05:54 UTC
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

Comment 21 Jakub Jelinek 2010-05-26 06:07:53 UTC
Should be fixed now.
Comment 22 Pat Haugen 2010-05-26 15:51:55 UTC
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'
Comment 23 Jakub Jelinek 2010-05-26 16:09:49 UTC
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

Comment 24 Jakub Jelinek 2010-05-26 16:10:22 UTC
Oops sorry, forgot redhat/gcc-4_4-branch has this function backported.
Fixed now.
Comment 25 Peter Bergner 2010-05-27 16:31:25 UTC
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

Comment 26 Peter Bergner 2010-06-02 15:40:37 UTC
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