Minimal test case: .global a .global b .weak a .align 2 a: .long 0 b: .long a-. The output should contain a R_SH_REL32 relocation, but instead it contains a constant. This bug affects gcc, which produces such expressions for intra-DSO PIC function calls; if the callee has a local weak definition; strong definitions will fail to override at link time. This could be fixed on the GCC side by having gcc explicitly produce @PCREL references rather than using arithmetic with ., but either way I think this is a serious gas bug that needs to be fixed.
Created attachment 8526 [details] Disable expression optimization if weak symbols are involved. Hi Rich. Please could you try out this patch and let me know if it works for you ? Cheers Nick
No. I believe I actually tried something near-identical before reporting this bug, but I just tried your patch to be sure, and there is still no relocation in the output object file.
Created attachment 8541 [details] Disable SH expression optimization and force relocations for weak symbols. Hi Rich, Ah 0 yes, you are right. Please try this revised patch instead. Cheers Nick
Indeed, that fixes it. However I'm skeptical of the way you're making the change: 1. S_FORCE_RELOC already has logic to force relocations for weak symbols when the strict argument is nonzero, so I wonder if something is wrongly calling it with strict=0 instead of strict=1? 2. Resolving the value of a weak symbol at as-time is always wrong, regardless of target or 'strictness' (whatever that's intended to mean). If the bug were in an sh-specific file, I would be satisfied with an sh-specific fix, but since it seems to be in symbol.c, this may indicate a bug that's affecting other targets too, and that should be fixed globally rather than via a target-specific optional macro. Thoughts?
Hi Rich, > Indeed, that fixes it. However I'm skeptical of the way you're making the > change: Me too - it was just a suggestion. > 1. S_FORCE_RELOC already has logic to force relocations for weak symbols > when the strict argument is nonzero, so I wonder if something is wrongly > calling it with strict=0 instead of strict=1? This seems likely, although I have to wonder - why is this bug only surfacing now ? Surely a problem like this, if it is generic, should have been encountered a long time ago. > 2. Resolving the value of a weak symbol at as-time is always wrong, True. So maybe what we need is a patch to S_FORCE_RELOC to always force relocations for weak symbols... I will try one out locally and see if it works.
Created attachment 8542 [details] Have S_FORCE_RELOC always return TRUE for weak symbols. Hi Rich, This patch is working for me. What do you think of it ? Cheers Nick
I like it, but I think some effort should be put into understanding why the wrong code was there to begin with and what the intent was. Here is the commit that broke it: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=ae6063d440ba5ec28af81e9fc899cc099561339e;hp=f0abc2a11f47c3ecdfe0b54421092d17c70fc5f2 And like all awful GNU changelog messages, there is no explanation whatsoever of the motivation/purpose for the changes, only redundant statements of change content that could be reproduced by the version control logs. To me, the logic in generic_force_reloc looks wrong: non-null fix->fx_subsy is not a valid condition for doing a fixup instead of a relocation. But the problem may be deeper. It looks like many targets are affected, not just sh.
You'll find that there are expressions where a weak or global symbol should be resolved by the assembler, which is why S_FORCE_RELOC has a "strict" paramenter. In fact, most expressions on most targets involving subtraction of defined symbols ought to be resolved, because these expressions are calculating some property of the *local* object/function. eg. .size expressions, relative offset to function in debug info. (Yes I know gcc generally emits local symbols for such offsets in debug info, but this wasn't always the case. Does all handcrafted debug or .eh_frame info use local symbols?) If either of the patches Nick has attached here are applied I can almost 100% guarantee you will break things. That's what I found when making similar changes a long time ago, using exactly the same reasoning about weak symbols. So, given that resolving subtraction expressions is a long-standing assembler behaviour, I think this is simply a SH gcc bug.
If this is the intended behavior, it should be documented as such. At the very least it's highly counter-intuitive, and a regression, albeit a very very old one. In any case I'll submit the patch I have for the GCC side.
(In reply to Alan Modra from comment #8) > So, given that resolving subtraction expressions is a > long-standing assembler behaviour, I think this is simply a SH gcc bug. If that should be the case, please file a bug against gcc with the target set to sh*-*-* and component set to "target". If possible, please provide some sample C code which triggers the issue. Thanks a lot! Adrian
I filed the corresponding GCC bug, #66609, at the same time I filed this one. Here is the link for reference: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66609 The bug report also contains a suggested fix, but I'm not sure if it's complete.
Created attachment 8545 [details] SH only fix for the problem. Personally I think that we should not be evaluating the difference of two different symbols when at least one of them is weak. But since we obviously have been doing this for a long time, I will let it stand. Instead here is an SH specific patch to fix the problem. It does not introduce any regressions into the binutils testsuites for the various different SH targets that I tried, so I think that it should be safe. Cheers Nick
Nick, can you confirm that your latest patch does not break typical use of .size like Alan Modra was concerned it would?
Hi Rich, (In reply to Rich Felker from comment #13) > Nick, can you confirm that your latest patch does not break typical use of > .size like Alan Modra was concerned it would? Hmm, good point. No, my patch would not work. *sigh* It just feels wrong allowing weak symbols to be resolved in expressions, but Alan is right, it is the way gas works.
Well that's not necessarily a show-stopper if there's a way to make it "work" (evaluate at as-time) in .size or other contexts that aren't actually section content and where a relocation cannot be emitted. However, some of the older GCC versions I'm using that are affected by this but and not easily patchable with Kazumoto Kojima's proposed patch for the GCC side (GCC issue 66609) are even more broken with respect to this weak+hidden issue even at the compiler level: they're inlining weak definitions! So unless I want to spend a lot of effort fixing bugs that were fixed half a decade ago in mainline GCC I think I just need to turn off all optimization based on visibility, and that should fix this issue. On the bright side, I think Kazumoto Kojima's patch works at least back to GCC 4.7 (with some minor adjustments to make it apply but no actual code changes). TL;DR: I'm okay with closing this issue and dealing with everything on the GCC side.