[Bug bootstrap/61078] New: ESA mode bootstrap failure since r209897

[Bug bootstrap/61078] New: ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

            Bug ID: 61078
           Summary: ESA mode bootstrap failure since r209897
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: bootstrap
          Assignee: unassigned at gcc dot gnu.org
          Reporter: krebbel at gcc dot gnu.org

Regression hunt indicates that r209897 might have caused a bootstrap failure on
s390 ESA mode:

Bootstrap dies reproducable with a segfault in bash:
...
make install-leaf DESTDIR=../.././gcc \
  slibdir= libsubdir= MULTIOSDIR=.
make[4]: Entering directory
`/build/patched/gcc-4.10.0-build/s390-ibm-linux-gnu/libgcc'
/bin/sh /build/patched/gcc/libgcc/../mkinstalldirs ../.././gcc
/usr/bin/install -c -m 644 libgcc_eh.a ../.././gcc/
chmod 644 ../.././gcc/libgcc_eh.a
ranlib ../.././gcc/libgcc_eh.a
/bin/sh /build/patched/gcc/libgcc/../mkinstalldirs ../.././gcc;
/usr/bin/install -c -m 644 ./libgcc_s.so.1 ../.././gcc/libgcc_s.so.1; rm -f
../.././gcc/libgcc_s.so; ln -s libgcc_s.so.1 ../.././gcc/libgcc_s.so
/bin/sh /build/patched/gcc/libgcc/../mkinstalldirs ../.././gcc
make[4]: *** [install-leaf] Segmentation fault
make[4]: Leaving directory
`/build/patched/gcc-4.10.0-build/s390-ibm-linux-gnu/libgcc'
make[3]: *** [all] Error 2
make[3]: Leaving directory
`/build/patched/gcc-4.10.0-build/s390-ibm-linux-gnu/libgcc'
make[2]: *** [all-stage2-target-libgcc] Error 2
make[2]: Leaving directory `/build/patched/gcc-4.10.0-build'
make[1]: *** [stage2-bubble] Error 2
make[1]: Leaving directory `/build/patched/gcc-4.10.0-build'
make: *** [all] Error 2


[pid  1082] vfork(Process 1088 attached
 <unfinished ...>
[pid  1088] rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[pid  1088] prlimit64(0, RLIMIT_STACK, {rlim_cur=8192*1024,
rlim_max=RLIM64_INFINITY}, NULL) = 0
[pid  1088] execve("/bin/sh", ["/bin/sh", "-c", "subdirs='testsuite';
\\\ntarget=`e"...], [/* 172 vars */] <unfinished ...>
[pid  1082] <... vfork resumed> )       = 1088
[pid  1082] rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[pid  1082] wait4(-1,  <unfinished ...>
[pid  1088] <... execve resumed> )      = 0
....
[pid  1088] --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR,
si_errno=1825521056, si_addr=0} ---
[pid  1088] +++ killed by SIGSEGV +++
[pid  1082] <... wait4 resumed> [{WIFSIGNALED(s) && WTERMSIG(s) == SIGSEGV}],
0, NULL) = 1088
[pid  1082] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_KILLED, si_pid=1088,
si_status=SIGSEGV, si_utime
=0, si_stime=0} ---
[pid  1082] sigreturn() (mask [])       = 1088

I've verified that reverting the patch "fixes" the problem. I haven't figured
out yet how the patch is related to the problem. Perhaps the execve arguments
for /bin/sh somehow get messed up with the patch?!

[Bug bootstrap/61078] [4.10 Regression] ESA mode bootstrap failure since r209897

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.10.0
            Summary|ESA mode bootstrap failure  |[4.10 Regression] ESA mode
                   |since r209897               |bootstrap failure since
                   |                            |r209897

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-08-25
     Ever confirmed|0                           |1

--- Comment #2 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
The bash crash comes from using a miscompiled libgcc_s.so during gcc build.
There are "la r15,0(r15)" instructions everywhere in the code used for
decrementing the stack pointer where 0 is wrongly used as displacement. This in
turn is the result of being compiled by a miscompiled cc1plus. The following
negation of  the frame size value in s390_emit_prologue function is wrongly
split by the negdi2_31 splitter.

...
  if (cfun_frame_layout.frame_size > 0)
    {
      rtx frame_off = GEN_INT (-cfun_frame_layout.frame_size);
...

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

--- Comment #3 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
(In reply to jgreenhalgh from comment #1)
> I'm happy to take a look at this, but I have no access to an s390 ESA mode
> environment, so will struggle to make much progress.
> 
> If it is the case that s390 relies on PUSH_ARGS_REVERSED == 0, then r209897
> will need to be reverted as it is clearly erroneous.

s390 indeed uses PUSH_ARGS_REVERSED == 0 but the code correctness does not
actually depend on this since every argument has a dedicated slot either in a
register or a stack slot.  In the case described above your patch just revealed
a backend problem:
https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02313.html

On the other hand I think the asm code looks more obvious on S/390 with
PUSH_ARGS_REVERSED=0 since the operands then are assigned in the expected
order. There is simply no reason for doing it the other way around on S/390.
While I don't have a strong opinion about this I would prefer to go back to the
old behavior.

Also there seem to be other code (gimplify.c) which still depends on
PUSH_ARGS_REVERSED?!

I'm also not sure about the performance impact of this. On S/390 the generated
code changes a lot with your patch.

> 
> Otherwise, any reduced testcase you can find where we do the wrong thing
> preparing stack arguments will be of great help hunting the bug.

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

--- Comment #4 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
While the patch fixes this particular case the problem appears to be more
fundamental. The S/390 backend only considers GPR register pairs starting with
an even register to be valid (REGNO_PAIR_OK in s390.h). However, thanks to our
ABI we might encounter invalid pairs with hard regs during parameter passing.
This used to be resolved by the call preparation code which loads the parameter
into a pseudo first. That's why our move patterns are capable of dealing with
overlapping register pairs. However, the other patterns aren't. In that case it
was the negdi pattern but there might be similar issues with add, sub, ... .

In this example the invalid register pair in the negdi pattern comes from
combining the move which has been generated during call preparation (insn 18)
with the negdi pattern:

before combine:

(insn 17 16 18 5 (parallel [
            (set (reg:DI 49 [ D.1409 ])
                (neg:DI (reg:DI 44 [ D.1409 ])))
            (clobber (reg:CC 33 %cc))
        ]) t.c:20 491 {*negdi2_31}
     (expr_list:REG_DEAD (reg:DI 44 [ D.1409 ])
        (expr_list:REG_UNUSED (reg:CC 33 %cc)
            (nil))))
(insn 18 17 19 5 (set (reg:DI 3 %r3)
        (reg:DI 49 [ D.1409 ])) t.c:20 64 {*movdi_31}
     (expr_list:REG_DEAD (reg:DI 49 [ D.1409 ])
        (nil)))

after combine:

(insn 18 17 19 5 (parallel [
            (set (reg:DI 3 %r3)
                (neg:DI (reg:DI 44 [ D.1409 ])))
            (clobber (reg:CC 33 %cc))
        ]) t.c:20 491 {*negdi2_31}
     (expr_list:REG_UNUSED (reg:CC 33 %cc)
        (expr_list:REG_DEAD (reg:DI 44 [ D.1409 ])
            (nil))))

r44 then is assigned to hard reg r2 creating the problematic overlap:

(insn 18 20 19 5 (parallel [
            (set (reg:DI 3 %r3)
                (neg:DI (reg:DI 2 %r2 [orig:44 D.1409 ] [44])))
            (clobber (reg:CC 33 %cc))
        ]) t.c:20 491 {*negdi2_31}
     (nil))

I'm not sure what the best way is to fix this. Instead of implementing
splitters for all these cases the easiest might to mark the destination
operands as early clobber in the split patterns. That way reload is forced to
repair the situation.

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

--- Comment #5 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Created attachment 33411
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33411&action=edit
Reduced testcase

compile with -O2 -mesa -m31 -march=g5

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[jgreenhalgh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

--- Comment #6 from jgreenhalgh at gcc dot gnu.org ---
> Also there seem to be other code (gimplify.c) which still depends on
> PUSH_ARGS_REVERSED?!

Indeed. I left that out of mercy to those users (erroneously) relying on the
order of evaluating function arguments. C and C++ explicitly mark the
evaluation order as unspecified, but there seems no good reason to break user
expectations.

If we were to revert my patch, we would have to either remove the tie between
what gimple thinks of as PUSH_ARGS_REVERSED and what expand thinks of as
PUSH_ARGS_REVERSED, or break (admittedly broken and not-portable) user code.

> I'm also not sure about the performance impact of this. On S/390 the generated
> code changes a lot with your patch.

As mentioned in the original patch submission [1], the effect is to allow
removal the removal of temporary registers when shuffling arguments for a call.

I can't speak to the performance on S/390. Certainly if your arguments are
predominantly stack-based I wouldn't expect to see any benefit, but I
wouldn't expect to see much harm either, assuming and reading and writing to
descending stack locations was as efficient as reading and writing to ascending
stack locations. That assumption might well not hold for S/390.

If S/390 passes small numbers of arguments in registers, I'd expect the net
result to be positive for performance.

[1]: https://gcc.gnu.org/ml/gcc-patches/2014-03/msg01252.html

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

--- Comment #7 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
(In reply to jgreenhalgh from comment #6)
> As mentioned in the original patch submission [1], the effect is to allow
> removal the removal of temporary registers when shuffling arguments for a
> call.

Ok. In fact it is exactly the removal of a temporary reg which revealed the
back-end bug. The overlapping live ranges so far protected us from running into
this particular problem.

> If S/390 passes small numbers of arguments in registers, I'd expect the net
> result to be positive for performance.

I agree. S/390 should be able to benefit from that so forget about reverting
your patch :)  I'll find a way to fix the patterns in question. Thanks for the
explanation!

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

--- Comment #8 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Author: krebbel
Date: Wed Sep  3 08:06:09 2014
New Revision: 214850

URL: https://gcc.gnu.org/viewcvs?rev=214850&root=gcc&view=rev
Log:
2014-09-03  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

    PR target/61078
    * config/s390/s390.md ("*negdi2_31"): Add s390_split_ok_p check
    and add a second splitter to handle the remaining cases.

2014-09-03  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

    PR target/61078
    * gcc.target/s390/pr61078.c: New testcase.



Added:
    trunk/gcc/testsuite/gcc.target/s390/pr61078.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/s390/s390.md
    trunk/gcc/testsuite/ChangeLog

[Bug bootstrap/61078] [5 Regression] ESA mode bootstrap failure since r209897

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61078

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Fixed per comment 8