[Bug target/65358] New: parameter passing bug with tail call optimization on arm

[Bug target/65358] New: parameter passing bug with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

            Bug ID: 65358
           Summary: parameter passing bug with tail call optimization on
                    arm
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hong.gyu.kim at lge dot com

struct pack
{
    int fine;
    int victim;
    int killer;
};
int bar(int a, int b, struct pack p);
int foo(int arg1, int arg2, int arg3, struct pack p)
{
    return bar(arg2, arg3, p);
}

When I cross compile the above code with "-O2" option for arm, one variable in
"struct pack" is lost.

The "vitcim" value is overwritten by "killer" value while passing arguments in
"struct pack" from "foo" to "bar".

Initially the arguments are passed this way right after foo invoked.
r0: arg1
r1: arg2
r2: arg3
r3: p.fine
MEM[sp]: p.victim
MEM[sp-4]: p.killer

Parameter setting for bar must be this way right before bar invoked.
r0: arg2
r1: arg3
r2: p.fine
r3: p.victim
MEM[sp]: p.killer

But parameter passing is structured as follows:
(p.victim is overwritten by p.killer)
r0: arg2
r1: arg3
r2: p.fine
r3: p.killer (*)
MEM[sp]: p.killer


The assembly code of "foo" generated is as follows:

foo:
        @ args = 16, pretend = 8, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        sub     sp, sp, #8
        mov     r0, r1
        str     lr, [sp, #-4]!
        add     ip, sp, #8
(1)     ldr     lr, [sp, #16]
        mov     r1, r2
        str     r3, [sp, #8]
(2)     str     lr, [sp, #12]
        ldr     lr, [sp], #4
        ldmia   ip, {r2, r3}
        add     sp, sp, #8
        b       bar

The point is that (1) loads "p.killer", then (2) overwrites "p.victim" value.
Until this point, "p.victim" is never copied anyway, which makes the value
disappear.

This bug is not shown when compiled with "-fno-optimize-sibling-calls".

This bug is shown in gcc-4.9.2 and also in gcc-5.0.0.
I also compiled the same program for x86 and x86_64 and those do not generate
this kind of buggy code. This bug is only shown in arm code.

I found that this bug is detected right after the RTL expand phase. (with
-fdump-rtl-expand)

[Bug target/65358] parameter passing bug with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

Honggyu Kim <hong.gyu.kim at lge dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |arm-unknown-linux-gnueabi
               Host|                            |x86_64-build_unknown-linux-
                   |                            |gnu
              Build|                            |x86_64-build_unknown-linux-
                   |                            |gnu

--- Comment #1 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
"gcc -v" is here:

$ arm-unknown-linux-gnueabi-gcc -v
Using built-in specs.
COLLECT_GCC=arm-unknown-linux-gnueabi-gcc
COLLECT_LTO_WRAPPER=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/x-tools/arm-unknown-linux-gnueabi/libexec/gcc/arm-unknown-linux-gnueabi/5.0.0/lto-wrapper
Target: arm-unknown-linux-gnueabi
Configured with:
/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/src/crosstool-ng/.build/src/gcc-custom/configure
--build=x86_64-build_unknown-linux-gnu --host=x86_64-build_unknown-linux-gnu
--target=arm-unknown-linux-gnueabi
--prefix=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/x-tools/arm-unknown-linux-gnueabi
--with-sysroot=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/x-tools/arm-unknown-linux-gnueabi/arm-unknown-linux-gnueabi/sysroot
--enable-languages=c,c++,fortran --with-cpu=cortex-a9 --with-fpu=neon
--with-float=softfp --with-pkgversion='crosstool-NG 1.20.0 - 5.0.0-x86_64'
--enable-__cxa_atexit --enable-libmudflap --enable-libgomp --enable-libssp
--enable-libquadmath --enable-libquadmath-support --enable-libsanitizer
--with-gmp=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/src/crosstool-ng/.build/arm-unknown-linux-gnueabi/buildtools
--with-mpfr=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/src/crosstool-ng/.build/arm-unknown-linux-gnueabi/buildtools
--with-mpc=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/src/crosstool-ng/.build/arm-unknown-linux-gnueabi/buildtools
--with-isl=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/src/crosstool-ng/.build/arm-unknown-linux-gnueabi/buildtools
--with-cloog=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/src/crosstool-ng/.build/arm-unknown-linux-gnueabi/buildtools
--with-libelf=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/src/crosstool-ng/.build/arm-unknown-linux-gnueabi/buildtools
--with-host-libstdcxx='-static-libgcc -Wl,-Bstatic,-lstdc++,-Bdynamic -lm'
--enable-threads=posix --enable-target-optspace --enable-linker-build-id
--with-linker-hash-style=gnu --enable-plugin --enable-gold --disable-nls
--disable-multilib
--with-local-prefix=/home/hong.gyu.kim/work.hard/kasan/toolchain/kasan/x-tools/arm-unknown-linux-gnueabi/arm-unknown-linux-gnueabi/sysroot
--enable-c99 --enable-long-long
Thread model: posix
gcc version 5.0.0 20150306 (experimental) (crosstool-NG 1.20.0 - 5.0.0-x86_64)

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #2 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
Sorry, I miss typed the initial argument status for "foo"
I will modified "MEM[sp-4]: p.killer" to "MEM[sp+4]: p.killer" as follows:

r0: arg1
r1: arg2
r2: arg3
r3: p.fine
MEM[sp]: p.victim
MEM[sp+4]: p.killer

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[mikpelinux at gmail dot com]

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

Mikael Pettersson <mikpelinux at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpelinux at gmail dot com

--- Comment #3 from Mikael Pettersson <mikpelinux at gmail dot com> ---
Created attachment 34986
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34986&action=edit
test case

Here's a runtime test case that reproduces the wrong-code for me on
armv5tel-linux-gnueabi and armv7l-linux-gnueabi, with (at least) gcc 4.9 and
4.8.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #4 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
Dear Mikael Pettersson

I also have a runtime testcase, which is different from dejagnu format.
Can I add this testcase with your modification as my first gcc contribution? :)
I was trying to fix this bug myself for about 2 weeks but maybe I need some
help from the community.
I appreciate your help.

Honggyu

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #5 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
I just wrote foo function code separately to debug gcc more easily by compile
only problematic code.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[jgreenhalgh at gcc dot gnu.org]

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

James Greenhalgh <jgreenhalgh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-03-10
                 CC|                            |jgreenhalgh at gcc dot gnu.org
               Host|x86_64-build_unknown-linux- |*-*-*
                   |gnu                         |
            Version|5.0                         |4.6.3
     Ever confirmed|0                           |1
      Known to fail|                            |4.6.3

--- Comment #7 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
Confirmed with native and cross compilers for arm-*-*, at least as far back as
GCC 4.6.3.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #8 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
(In reply to Mikael Pettersson from comment #6)
> (In reply to Honggyu Kim from comment #4)
> 
> > Can I add this testcase with your modification as my first gcc contribution?
> > :)
> 
> Sure, just attach it to this PR as a new test case.
I've just submitted a patch for testcase by making it to check the entire
argument values.
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00704.html

Thanks for you help.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

Honggyu Kim <hong.gyu.kim at lge dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |4.9.2

--- Comment #9 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
(In reply to James Greenhalgh from comment #7)
> Confirmed with native and cross compilers for arm-*-*, at least as far back
> as GCC 4.6.3.
Thanks for the confirmation.
I also added gcc-4.9.2 in Known to fail list.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ktkachov at gcc dot gnu.org

--- Comment #10 from ktkachov at gcc dot gnu.org ---
Hmmm, I have a fix to check_sibcall_argument_overlap in calls.c that's supposed
to catch the overlap in accesses on the stack and correctly identify the
conflict. This detection has the effect of getting gcc to decide that it can't
do a tail-call here. I wonder, is this the way to go i.e. should we indeed be
disabling sibcalls in this case? I think so, if the ABI demands that part of
the struct is passed on the stack...

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

--- Comment #11 from ktkachov at gcc dot gnu.org ---
Thinking about it again, there's no reason not to do sibcalls, it's just the
code gets confused on how to shuffle the arguments around. Will investigate
deeper

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #12 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
(In reply to ktkachov from comment #11)
> Thinking about it again, there's no reason not to do sibcalls, it's just the
> code gets confused on how to shuffle the arguments around. Will investigate
> deeper

Yes, right. There's no reason not to do sibcall in this case.

>From the original assembly output as below:
foo:
        @ args = 16, pretend = 8, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        sub     sp, sp, #8
        mov     r0, r1
        str     lr, [sp, #-4]!
        add     ip, sp, #8
        ldr     lr, [sp, #16]
        mov     r1, r2
        str     r3, [sp, #8]
(a)     str     lr, [sp, #12]
        ldr     lr, [sp], #4
(b)     ldmia   ip, {r2, r3}
        add     sp, sp, #8
        b       bar

We can place instruction (b) before (a).
In that case, it works fine and sibcall optimization is also done.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #13 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
Created attachment 35041
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35041&action=edit
backport patch from linaro

Jongsung Kim (neidhard.kim@lge.com) found a patch that generates this error.
I think this patch is not the root cause but it just triggers some buggy code
by enabling sibcall optimization in this case.
When we applied a patch in a reverse way, sibcall optimiztion is off in this
case then it works fine.
I have attached the patch for your reference.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

--- Comment #15 from ktkachov at gcc dot gnu.org ---
Hmmm, actually it's not that simple, as testing showed.
The comment at the final load-to-regs code says:
  /* If part should go in registers, copy that part
     into the appropriate registers.  Do this now, at the end,
     since mem-to-mem copies above may do function calls.  */

So just moving this at the beginning is not going to work.
Another question that comes up is: why didn't the code in calls.c not catch
that we're reading from a clobbered location and cancel the tail call?
It's supposed to do that with check_sibcall_argument_overlap at various points
in the expansion, but it doesn't catch this case.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |ktkachov at gcc dot gnu.org

--- Comment #16 from ktkachov at gcc dot gnu.org ---
I'm working on a patch btw.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #17 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
(In reply to ktkachov from comment #16)
> I'm working on a patch btw.

This bug is only shown in arm code so maybe the bug is in gcc/config/arm
directory.
I was trying to fix it myself but I may need more experience in gcc code to fix
this kind of problem.
Thank you.

[Bug target/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

--- Comment #18 from ktkachov at gcc dot gnu.org ---
(In reply to Honggyu Kim from comment #17)
> (In reply to ktkachov from comment #16)
> > I'm working on a patch btw.
> 
> This bug is only shown in arm code so maybe the bug is in gcc/config/arm
> directory.
> I was trying to fix it myself but I may need more experience in gcc code to
> fix this kind of problem.
> Thank you.

I believe this bug could be triggered on any target/ABI that can pass aggregate
arguments (i.e. structs) partially in regs and partially on the stack and the
fix I'm working on is in the midend.

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |middle-end

--- Comment #19 from ktkachov at gcc dot gnu.org ---
Change component

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #20 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
Hi all,

Kyrill submitted a bug fix patch about 2 weeks ago.
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01014.html

I have tested his patch and found that the problem is clearly fixed.
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01497.html

Can anyone please review and approve the patch to fix this bug before releasing
gcc-5.0.0?
Thanks,

Honggyu

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[jgreenhalgh at gcc dot gnu.org]

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

James Greenhalgh <jgreenhalgh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #21 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
As this has been failing since GCC 4.6.3, it is not a regression and therefore
Kyrill's fix would not be appropriate for Stage 4.

It may be that the release managers make an exception for this fix, given that
it can cause wrong-code generation on a primary target. Otherwise, I'd expect
this to get fixed in 6.0 and backported as appropriate. Richard/Jakub?

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

--- Comment #22 from ktkachov at gcc dot gnu.org ---
(In reply to James Greenhalgh from comment #21)
> As this has been failing since GCC 4.6.3, it is not a regression and
> therefore Kyrill's fix would not be appropriate for Stage 4.
> 
> It may be that the release managers make an exception for this fix, given
> that it can cause wrong-code generation on a primary target. Otherwise, I'd
> expect this to get fixed in 6.0 and backported as appropriate. Richard/Jakub?

Yeah, I agree it's not a regression on the release branches, but we did have
wrong-code fixes in stage4 before (like for PR 65235).
My fix should only ever trigger in the case where we would definitely
miscompile otherwise, and should not impact codegen in any other case.

I don't mind waiting till stage 1 and backporting later. Up to the
maintainers/release managers.

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[rguenth at gcc dot gnu.org]

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

--- Comment #23 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to ktkachov from comment #22)
> (In reply to James Greenhalgh from comment #21)
> > As this has been failing since GCC 4.6.3, it is not a regression and
> > therefore Kyrill's fix would not be appropriate for Stage 4.
> > 
> > It may be that the release managers make an exception for this fix, given
> > that it can cause wrong-code generation on a primary target. Otherwise, I'd
> > expect this to get fixed in 6.0 and backported as appropriate. Richard/Jakub?
> 
> Yeah, I agree it's not a regression on the release branches, but we did have
> wrong-code fixes in stage4 before (like for PR 65235).
> My fix should only ever trigger in the case where we would definitely
> miscompile otherwise, and should not impact codegen in any other case.
> 
> I don't mind waiting till stage 1 and backporting later. Up to the
> maintainers/release managers.

It's fine to fix wrong-code regressions in stage4 if they are obviously safe.

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

--- Comment #24 from ktkachov at gcc dot gnu.org ---
Author: ktkachov
Date: Wed May 27 13:25:01 2015
New Revision: 223753

URL: https://gcc.gnu.org/viewcvs?rev=223753&root=gcc&view=rev
Log:
[expr.c] PR target/65358 Avoid clobbering partial argument during sibcall

        PR target/65358
        * expr.c (memory_load_overlap): New function.
        (emit_push_insn): When pushing partial args to the stack would
        clobber the register part load the overlapping part into a pseudo
        and put it into the hard reg after pushing.  Change return type
        to bool.  Add bool argument.
        * expr.h (emit_push_insn): Change return type to bool.
        Add bool argument.
        * calls.c (expand_call): Cancel sibcall optimization when encountering
        partial argument on targets with ARGS_GROW_DOWNWARD and
        !STACK_GROWS_DOWNWARD.
        (emit_library_call_value_1): Update callsite of emit_push_insn.
        (store_one_arg): Likewise. 

        PR target/65358
        * gcc.dg/pr65358.c: New test. 


Added:
    trunk/gcc/testsuite/gcc.dg/pr65358.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/calls.c
    trunk/gcc/expr.c
    trunk/gcc/expr.h
    trunk/gcc/testsuite/ChangeLog

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[ktkachov at gcc dot gnu.org]

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

--- Comment #25 from ktkachov at gcc dot gnu.org ---
This should be fixed on trunk for GCC 6.
I'll keep this open for a few days to make sure there are no glaring complaints
about the patch as it gets through the auto-testers

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[hong.gyu.kim at lge dot com]

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

--- Comment #26 from Honggyu Kim <hong.gyu.kim at lge dot com> ---
(In reply to ktkachov from comment #25)
> This should be fixed on trunk for GCC 6.
> I'll keep this open for a few days to make sure there are no glaring
> complaints about the patch as it gets through the auto-testers

I have checked that the patch has been merged.
I appreciate your work. Thank you.

Honggyu

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[ramana at gcc dot gnu.org]

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

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |6.0

--- Comment #27 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> ---
Fixed for 6.0

[Bug middle-end/65358] wrong parameter passing code with tail call optimization on arm

[mikpelinux at gmail dot com]

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

--- Comment #28 from Mikael Pettersson <mikpelinux at gmail dot com> ---
See PR67226 for another testcase that fails on ARM with the final fix for this
PR (r223753), but succeeds with the initial proposed fix posted in:
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01014.html