Bug 51643 - Incorrect code produced for tail-call of weak function with -O2/-O3 option
Summary: Incorrect code produced for tail-call of weak function with -O2/-O3 option
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.6.3
: P3 normal
Target Milestone: ---
Assignee: Richard Earnshaw
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-20 21:22 UTC by Alexander Osipenko
Modified: 2012-06-08 08:58 UTC (History)
0 users

See Also:
Host:
Target: arm-eabi
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-12-22 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Osipenko 2011-12-20 21:22:18 UTC
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)
Comment 1 Andrew Pinski 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.
Comment 2 Andrew Pinski 2011-12-20 21:27:15 UTC
Also the linker seems funny to replace a branch to null with a nop.
Comment 3 Alexander Osipenko 2011-12-20 21:39:11 UTC
This behavior is explicitly defined in ARM RealView compiler, and GCC seems try to follow this convention.
Comment 4 Alexander Osipenko 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.
Comment 5 joseph@codesourcery.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.
Comment 6 Alexander Osipenko 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.
Comment 7 Richard Biener 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.
Comment 8 Richard Earnshaw 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
Comment 9 Richard Earnshaw 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
Comment 10 Richard Earnshaw 2011-12-22 14:29:54 UTC
Fixed on 4.6 and trunk.
Comment 11 Richard Earnshaw 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
Comment 12 Richard Earnshaw 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
Comment 13 Alexander Osipenko 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?
Comment 14 Richard Earnshaw 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.
Comment 15 Alexander Osipenko 2011-12-23 00:40:13 UTC
Oh, I see, thanks!
Comment 16 jye2 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