[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|arm-eabi                    |
          Component|rtl-optimization            |target
      Known to fail|4.6.1, 4.6.2                |
              Build|x86_64-redhat-linux         |
           Severity|major                       |normal

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-12-20 21:26:44 UTC ---
I think this code is undefined.  You are calling a weak function without
checking if it is null.

[Bug rtl-optimization/51643] New: Incorrect code produced for tail-call of weak function with -O2/-O3 option

[sipych at gmail dot com]

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

             Bug #: 51643
           Summary: Incorrect code produced for tail-call of weak function
                    with -O2/-O3 option
    Classification: Unclassified
           Product: gcc
           Version: 4.6.2
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: sipych@gmail.com


A tail-call of weak function produces incorrect code, compiled with -O2 or -O3.
Target: arm-eabi
Version: GCC 4.6.2 and 4.6.1

***** Test case: t00x1.c

<<<<<
extern void __attribute__((weak)) wfunc(void);
void main(void)
{
  wfunc();  // tail weak call fails!!!
  // __asm__ volatile ("nop":::"memory"); // workaround
}
>>>>>

$ /opt/arm/gnuarm-4.6.2/bin/arm-eabi-gcc -c -S -o t00x1.S -O2 t00x1.c

***** Contents of t00x1.S:
    .file    "t00x1.c"
    .section    .text.startup,"ax",%progbits
    .align    2
    .global    main
    .type    main, %function
main:
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    @ link register save eliminated.
    b    wfunc    @@@@ <<< Will be replaced by "NOP" in linker!!
                         @@@@ so, no return sequence!
    .size    main, .-main
    .weak    wfunc
    .ident    "GCC: (GNU) 4.6.2"

***** Disassembled (objdump) code: t00x1.lst:
00000240 <main>:
 240:    e1a00000     nop            ; (mov r0, r0)  @@@@ really, NOP

00000244 <__do_global_dtors_aux>:
 244:    e92d4010     push    {r4, lr}
 248:    e59f4028     ldr    r4, [pc, #40]    ; 278
                        .......
******
Call to function "wfunc" was converted to NOP by linker, as expected.
As a result, no return sequence from main(), falling througt to next code line.

For reference: the same program compiled with -O1
*******
    .file    "t00x1.c"
    .text
    .align    2
    .global    main
    .type    main, %function
main:
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    stmfd    sp!, {r3, lr}
    bl    wfunc
    ldmfd    sp!, {r3, pc}
    .size    main, .-main
    .weak    wfunc
    .ident    "GCC: (GNU) 4.6.2"

Code with workaround, compiled with -O2 correct:
$ /opt/arm/gnuarm-4.6.2/bin/arm-eabi-gcc -c -S -o t00x1.S -O2 t00x1.c
*******
    .file    "t00x1.c"
    .section    .text.startup,"ax",%progbits
    .align    2
    .global    main
    .type    main, %function
main:
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    stmfd    sp!, {r3, lr}
    bl    wfunc
@ 5 "t00x1.c" 1
    nop
@ 0 "" 2
    ldmfd    sp!, {r3, pc}
    .size    main, .-main
    .weak    wfunc
    .ident    "GCC: (GNU) 4.6.2"

*******
Compiler options:
$ /opt/arm/gnuarm-4.6.2/bin/arm-eabi-gcc -v
Using built-in specs.
COLLECT_GCC=/opt/arm/gnuarm-4.6.2/bin/arm-eabi-gcc
COLLECT_LTO_WRAPPER=/opt/arm/gnuarm-4.6.2/libexec/gcc/arm-eabi/4.6.2/lto-wrapper
Target: arm-eabi
Configured with: ../../src/gcc-4.6.2/configure --target=arm-eabi
--prefix=/opt/arm/gnuarm-4.6.2 --enable-multilib --enable-interwork
--enable-biendian --enable-fpu --with-newlib --with-gnu-ld --with-gnu-as
--disable-nls --disable-shared --with-arch=armv5te --with-fpu=vfp
--with-float=softfp --with-abi=aapcs-linux --enable-lto
--enable-languages=c,c++ --disable-threads --enable-ppl --enable-cloog
--enable-gmp --enable-mpfr --enable-lto
-with-headers=/opt/arm/gnuarm-4.6.2/arm-eabi/include
Thread model: single
gcc version 4.6.2 (GCC)

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-12-20 21:27:15 UTC ---
Also the linker seems funny to replace a branch to null with a nop.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[sipych at gmail dot com]

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

--- Comment #3 from Alexander Osipenko <sipych at gmail dot com> 2011-12-20 21:39:11 UTC ---
This behavior is explicitly defined in ARM RealView compiler, and GCC seems try
to follow this convention.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[sipych at gmail dot com]

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

--- Comment #4 from Alexander Osipenko <sipych at gmail dot com> 2011-12-20 21:58:39 UTC ---
>From ARM EABI specification (doc: ARM IHI 0044A)

"On platforms that do not support dynamic pre-emption of symbols an unresolved
weak reference to a symbol relocated by R_ARM_CALL shall be treated as a jump
to the next instruction (the call becomes a no-op). The behaviour of
R_ARM_JUMP24 in these conditions is unspecified."

At least, tail call + epilogue shall not be replaced by (undefined) JUMP.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[joseph at codesourcery dot com]

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

--- Comment #5 from joseph at codesourcery dot com <joseph at codesourcery dot com> 2011-12-20 22:34:43 UTC ---
On Tue, 20 Dec 2011, sipych at gmail dot com wrote:

> "On platforms that do not support dynamic pre-emption of symbols an unresolved
> weak reference to a symbol relocated by R_ARM_CALL shall be treated as a jump
> to the next instruction (the call becomes a no-op). The behaviour of
> R_ARM_JUMP24 in these conditions is unspecified."
> 
> At least, tail call + epilogue shall not be replaced by (undefined) JUMP.

That's an ABI specifying relocation handling, not an API specifying 
semantics of C source code.  The C semantics are as specified in the C 
standard, i.e. undefined behavior if the call is executed; C is a 
high-level language not a portable assembler and the semantics are those 
of the C standard not those of particular instructions you might guess 
have some relation to particular C operations.  The EABI semantics say 
what happens with a relocation that might have been generated from code 
where the call is properly conditional in the C source, or might have been 
generated from code with undefined behavior.

It would be valid for the compiler to rely on the EABI semantics to 
optimize code generation for "if (wfunc) wfunc ();", for example, but 
those semantics do not change the undefined nature of code that doesn't 
condition the call.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[sipych at gmail dot com]

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

--- Comment #6 from Alexander Osipenko <sipych at gmail dot com> 2011-12-20 23:33:06 UTC ---
It seems reasonable to expect minimal consistency, either generating invalid
(zero for example) reference for any direct weak function call, better marking
it with "warning" or even "error" message, or not depending on the optimization
options.

In any case, the semantics of weak referencies is beyond of the C standard
scope,
so even "if( wfunc ) wfunc();" may be treated only in terms of reasonable
consistency.

EABI specification follows a simple rule: "call to undefined weak function do
nothing".
Just the same do "if( wfunc ) wfunc();", a little bit longer, if we believe
that undefined wfunc == 0 always, but not 0xDEADBEEF eventually when optimized.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |arm-eabi

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-12-21 09:53:24 UTC ---
Technically this bug is invalid (undefined is undefined, not consistent).  ARM
maintainers may want to disallow tail/sibling calls to possibly weak targets.

The ABI is inconsistent itself, btw, as

 weak_fn ();

beahaves different than

 *(&weak_fn) ();

as I presume the linker cannot do anything for indirect calls to weak
functions.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rearnsha at gcc dot gnu.org]

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

Richard Earnshaw <rearnsha at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2011-12-22
     Ever Confirmed|0                           |1

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rearnsha at gcc dot gnu.org]

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

--- Comment #8 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-12-22 14:13:16 UTC ---
Author: rearnsha
Date: Thu Dec 22 14:13:09 2011
New Revision: 182621

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182621
Log:
    PR target/51643
    * arm.c (arm_function_ok_for_sibcall): Don't try to tailcall a
    weak function on bare-metal EABI targets.

    * gcc.target/arm/sibcall-2.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/arm/sibcall-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/testsuite/ChangeLog

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rearnsha at gcc dot gnu.org]

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

--- Comment #9 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-12-22 14:28:45 UTC ---
Author: rearnsha
Date: Thu Dec 22 14:28:39 2011
New Revision: 182622

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182622
Log:
    PR target/51643
    * arm.c (arm_function_ok_for_sibcall): Don't try to tailcall a
    weak function on bare-metal EABI targets.

    * gcc.target/arm/sibcall-2.c: New test.

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.target/arm/sibcall-2.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/arm/arm.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rearnsha at gcc dot gnu.org]

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

Richard Earnshaw <rearnsha at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
            Version|4.6.2                       |4.6.3
         Resolution|                            |FIXED

--- Comment #10 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-12-22 14:29:54 UTC ---
Fixed on 4.6 and trunk.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rearnsha at gcc dot gnu.org]

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

--- Comment #11 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-12-22 17:31:58 UTC ---
Author: rearnsha
Date: Thu Dec 22 17:31:50 2011
New Revision: 182628

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182628
Log:
    PR target/51643
    * arm.c (arm_function_ok_for_sibcall): Use DECL_WEAK in previous
    change.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rearnsha at gcc dot gnu.org]

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

--- Comment #12 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-12-22 17:33:04 UTC ---
Author: rearnsha
Date: Thu Dec 22 17:32:58 2011
New Revision: 182629

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182629
Log:
    PR target/51643
    * arm.c (arm_function_ok_for_sibcall): Use DECL_WEAK in previous
    change.

Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/arm/arm.c

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[sipych at gmail dot com]

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

--- Comment #13 from Alexander Osipenko <sipych at gmail dot com> 2011-12-22 21:31:56 UTC ---
Thanks, Richard!

It now works fine with -mabi=aapcs
Perhaps I don't understand some details, could you tell me why the
-mabi=aapcs-linux is not included in this fix?

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[rearnsha at gcc dot gnu.org]

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

--- Comment #14 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-12-22 23:27:29 UTC ---
Because the ABI says it only works for bare metal.

On a system with shared libraries, you can't tell at static link time if a weak
symbol will be resolved by a shared library, so it has to left up to the
dynamic linker which will fill in a PLT stub.  Once you have those, it's pretty
hard to make the call become a stub (and even if it were, it wouldn't be a NOP
(the linker would have to turn the call into something that simply returned as
the PLT sequence can't be patched).  Tail-calling PLT sequences is perfectly
safe, so there's no reason not to perform the optimization.

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[sipych at gmail dot com]

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

--- Comment #15 from Alexander Osipenko <sipych at gmail dot com> 2011-12-23 00:40:13 UTC ---
Oh, I see, thanks!

[Bug target/51643] Incorrect code produced for tail-call of weak function with -O2/-O3 option

[jye2 at gcc dot gnu.org]

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

--- Comment #16 from jye2 at gcc dot gnu.org 2012-06-08 08:58:02 UTC ---
Author: jye2
Date: Fri Jun  8 08:57:53 2012
New Revision: 188332

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188332
Log:
2012-06-08  Joey Ye  <joey.ye@arm.com>

    Backport r184442 from mainline
    2012-02-21  Richard Earnshaw  <rearnsha@arm.com>

    PR target/52294
    * thumb2.md (thumb2_shiftsi3_short): Split register and         
    immediate shifts.  For register shifts tie operands 0 and 1.
    (peephole2 for above): Check that register-controlled shifts
    have suitably tied operands.

    Backport r183756 from mainline
    2012-01-31  Matthew Gretton-Dann  <matthew.gretton-dann@arm.com>

    * config/arm/thumb2.md (thumb2_mov_notscc): Use MVN for true
    condition.

    Backport r183349 from mainline
    2012-01-20  Jakub Jelinek  <jakub@redhat.com>

    PR target/51915
    * config/arm/arm.c (arm_count_output_move_double_insns): Call
    output_move_double on a copy of operands array.

    Backport r183095 from mainline
    2012-01-11  Matthew Gretton-Dann  <matthew.gretton-dann@arm.com>

    * config/arm/arm.md (mov_notscc): Use MVN for false condition.

    Backport r182628 from mainline
    2011-12-21  Richard Earnshaw  <rearnsha@arm.com>

    PR target/51643
    * arm.c (arm_function_ok_for_sibcall): Use DECL_WEAK in previous
    change.

    Backport r182621 from mainline
    2011-12-21  Richard Earnshaw  <rearnsha@arm.com>

    PR target/51643
    * arm.c (arm_function_ok_for_sibcall): Don't try to tailcall a
    weak function on bare-metal EABI targets.

Testsuite:
    Backport r183349 from mainline
    2012-01-20  Jakub Jelinek  <jakub@redhat.com>

    PR target/51915
    * gcc.target/arm/pr51915.c: New test.

    Backport r183095 from mainline
    2012-01-11  Matthew Gretton-Dann  <matthew.gretton-dann@arm.com>

    * gcc.c-torture/execute/20120110-1.c: New testcase.

    Backport r182621 from mainline
    2011-12-21  Richard Earnshaw  <rearnsha@arm.com>

    PR target/51643
    * gcc.target/arm/sibcall-2.c: New test.


Added:
   
branches/ARM/embedded-4_6-branch/gcc/testsuite/gcc.c-torture/execute/20120111-1.c
    branches/ARM/embedded-4_6-branch/gcc/testsuite/gcc.target/arm/pr51915.c
    branches/ARM/embedded-4_6-branch/gcc/testsuite/gcc.target/arm/sibcall-2.c
Modified:
    branches/ARM/embedded-4_6-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-4_6-branch/gcc/config/arm/arm.c
    branches/ARM/embedded-4_6-branch/gcc/config/arm/arm.md
    branches/ARM/embedded-4_6-branch/gcc/config/arm/thumb2.md
    branches/ARM/embedded-4_6-branch/gcc/testsuite/ChangeLog.arm