Minimal test case (compile with -fPIC and -Os or higher, for sh2 or later): __attribute__((__weak__,__visibility__("hidden"))) void foo() { } void bar() { foo(); } For the call from bar to foo, gcc generates: mov.l .L3,r1 braf r1 .LPCS0: nop .align 2 .L3: .long foo-(.LPCS0+2) Gas in turn ignores the fact that foo is weak and assembles the literal to a constant with no relocation. This is the following bug I just reported in binutils: https://sourceware.org/bugzilla/show_bug.cgi?id=18561 However, GCC can and probably should work around it, by instead generating the equivalent form that gas assembles correctly: .long foo@PCREL-(.LPCS0+2-.) This is easily changed at the following location: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/sh/sh.c;h=e5fcd7683ef7e06d56a10427d792ba018d1af884;hb=HEAD#l1662 and changing it seems to have solved the problem for me, but I did not yet test extensively. I'm not sure if the subsequent case for UNSPEC_PCREL_SYMOFF also needs changes.
Created attachment 36246 [details] patch > .long foo@PCREL-(.LPCS0+2-.) looks to be reasonable. It seems that sh backend shouldn't use UNSPEC_SYMOFF for that case in the first place. Does the patch attached work for you?
The patch in comment 1 applies successfully to GCC 5.2.0 and fixes both the test case and the real-world code I was experiencing problems with. Unfortunately it doesn't apply to some of the older GCC versions I need to support, but I think I can backport it. Thanks!
(In reply to Rich Felker from comment #2) Thanks for the confirmation. I'll commit the patch after the undergoing additional test done.
Author: kkojima Date: Mon Aug 24 23:23:00 2015 New Revision: 227155 URL: https://gcc.gnu.org/viewcvs?rev=227155&root=gcc&view=rev Log: PR target/66609 * [SH] Take into account weak symbols for pc relative calls/sibcalls. Modified: trunk/gcc/ChangeLog trunk/gcc/config/sh/sh-protos.h trunk/gcc/config/sh/sh.c trunk/gcc/config/sh/sh.md
Kaz, do you think it's OK to backport this to GCC 5? It looks like the patch is not so intrusive...
(In reply to Oleg Endo from comment #5) > Kaz, do you think it's OK to backport this to GCC 5? It looks like the > patch is not so intrusive... Changing relocation is always intrusive, I think. I won't object to backport it, though. The patch does the right thing and will affect only very limited cases.
Created attachment 36359 [details] preprocessed source still affected by the bug Oddly I'm still experiencing this bug for some functions but not others, even with the patch applied. I've attached preprocessed output of a file which is being miscompiled: the reference to __do_cleanup_push wrongly binds locally, but the reference to __do_cleanup_pop is overridable like it should be.
Perhaps hold off on worrying about this; it's only happening with -mfdpic (with my forward-port of the fdpic patch applied) so it's possible that the bug is on my end in code that's not in upstream gcc. I'll follow up on this more later.
Indeed, the fdpic patch I forward-ported introduced new duplicates of some of the fragments that were changed in sh.md by the above patch. Once I fixed those, the problem went away. Sorry for the noise.
I think we can close this as fixed.
FYI a workaround for this and similar bugs, for users who are unable to upgrade, is to always use -ffunction-sections -fdata-sections. This inhibits the assembler's "optimization" differences between symbols simply because cross-section differences are never constant.