When assembled, a jump to weak symbol produces a jump to an invalid address. I found that problem on m68k-unknown-netbsd, but it may affect all m68k-*-aout targets. $ cat bug.s jmp mylabel nop .weak mylabel mylabel: nop $ as bug.s -o bug.o $ objdump -d bug.o bug.o: file format a.out-m68k-netbsd Disassembly of section .text: 00000000 <mylabel-0x6>: 0: 4efa 000a jmp %pc@(c <mylabel+0x6>) 4: 4e71 nop 00000006 <mylabel>: 6: 4e71 nop The value of the jmp instruction is wrong : it should be %pc@(6 <mylabel>) It might not be a problem, because there is a relocation. $ objdump -r bug.o bug.o: file format a.out-m68k-netbsd RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 00000002 DISP16 .text Here, it is clearly wrong. The relocation offset is good, but the value should be "mylabel" instead of ".text" After linking, there is no more relocation, but the value of the jump is still bad. The same problem was present in gas 2.14 for m68k-unknown-netbsd (already using BFD). The problem was not present in gas 2.14 for m68k-linux-aout (not using BFD) : $ objdump -d bug.o # binutils 2.14 m68k-linux-aout bug.o: file format a.out-zero-big Disassembly of section .text: 00000000 <mylabel-0x6>: 0: 4efa 0004 jmp %pc@(6 <mylabel>) 4: 4e71 nop 00000006 <mylabel>: 6: 4e71 nop I think the above result is correct and the current version of gas for m68k- unknown-netbsd should behave like that.
Created attachment 1287 [details] Do not convert fixups against weak symbols
Hi Vincent, Please could you try the uploaded patch. I think that it fix the assembler for you. Cheers Nick
Sorry, your fix does not work because... it is not compiled !!! It is inside a #ifdef OBJ_ELF block, but the m68k-unknown-netbsd target uses the a.out format. Furthermore, I think you forgot to remove a double ampersand in your fix : S_IS_WEAK (&& fixP->fx_addsy)) Thank you for trying to solve this bug !
Hi Vincent, Boy I must have been asleep when I wrote that patch, it was completely bogus. Oh well, I have uploaded another one for you to try. This time I have actually tested it with the test case you supplied, so I think that it should work. Please let me know how you get on. Cheers Nick
Created attachment 1292 [details] When writing out a.out relocations, treat relocs against weak symbols as if they were against externals
A lot better this time ! But half-fixed. $ as bug.s -o bug.o $ objdump -d bug.o bug.o: file format a.out-m68k-netbsd Disassembly of section .text: 00000000 <mylabel-0x8>: 0: 4ef9 0000 0010 jmp 10 <mylabel+0x8> 6: 4e71 nop 00000008 <mylabel>: 8: 4e71 nop ... We can see a strange value for the jmp (0x10), but it dosn't matter because... $ objdump -r bug.o bug.o: file format a.out-m68k-netbsd RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 00000002 32 mylabel ... the relocation is good ! Let's go ahead and link it : $ ld bug.o -o bug $ objdump -d bug bug: file format a.out-m68k-netbsd Disassembly of section .text: 00002020 <bug.o>: 2020: 4ef9 0000 2038 jmp 2038 <__etext+0xc> 2026: 4e71 nop 00002028 <mylabel>: 2028: 4e71 nop ... 0000202c <__etext>: ... Bad : The value of the jump should be 0x2028, but it is 0x2038 $ objdump -r bug bug: file format a.out-m68k-netbsd And there is no relocation. Note that I obtain the same result when I link bug.o using ld 2.14. So I thing there is still a problem with gas. Nice work... just another effort and you'll get it !
I found an allusion to a weak-bug in bfd/aout-cris.c In MY (swap_ext_reloc_out) : ... if (bfd_is_und_section (bfd_get_section (sym)) /* Remember to check for weak symbols; they count as global. */ || (sym->flags & (BSF_GLOBAL | BSF_WEAK)) != 0) r_extern = 1; I applied that fix into bfd/aoutx.h, but it didn't change anything on the testcase described here :(
Created attachment 1739 [details] Write correct values and relocs into a.out object files After spending *weeks* in the debugger, I finally managed to get it work. My patch is attached to this message. It is is composed of 4 parts : 1) The patch provided by Nick in Comment #5. It is necessary to generate a reloc with the correct symbol name. 2) Never relax references to weak symbols, as they must be full absolute pointers in order to be relocated. 3) In md_apply_fix(): Always put zero values into frags referencing a weak symbol. 4) In tc_gen_reloc(): Adjust addend in order to force bfd_install_relocation() to put a zero value into the frags referencing a weak symbol. This patch affects only a.out and m68k, and only when weak symbols are used. It may not affect anything else. I think it works as expected. Could someone verify it is OK and check this in, please ? Vincent Here are the detailed results : $ cat bug.s jmp mylabel nop .weak mylabel mylabel: nop $ as bug.s -o bug.o $ objdump -d bug.o bug.o: file format a.out-m68k-netbsd Disassembly of section .text: 00000000 <mylabel-0x8>: 0: 4ef9 0000 0000 jmp 0 <mylabel-0x8> 6: 4e71 nop 00000008 <mylabel>: 8: 4e71 nop ... $ objdump -r bug.o bug.o: file format a.out-m68k-netbsd RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 00000002 32 mylabel $ nm bug.o 00000008 W mylabel $ ld bug.o -o bug $ objdump -d bug bug: file format a.out-m68k-netbsd Disassembly of section .text: 00002020 <bug.o>: 2020: 4ef9 0000 2028 jmp 2028 <mylabel> 2026: 4e71 nop 00002028 <mylabel>: 2028: 4e71 nop ... 0000202c <__etext>: ...
Hi Vincent, Thanks for the patch. I have exmained it and it is fine, so I have checked it into the sources along with these ChangeLog entries. Cheers Nick gas/ChangeLog 2007-05-03 Vincent Riviere <vincent.riviere@freesbee.fr> Nick Clifton <nickc@redhat.com> PR gas/3041 * config/tc-m68k.c (relaxable_symbol): Do not relax weak symbols. (tc_gen_reloc): Adjust the addend of relocs against weak symbols. (md_apply_fix): Put zero values into the frags referencing weak symbols. bfd/ChangeLog 2007-05-03 Vincent Riviere <vincent.riviere@freesbee.fr> Nick Clifton <nickc@redhat.com> PR gas/3041 * aoutx.h (swap_std_reloc_out): Treat relocs against weak symbols in the same way as relocs against external symbols.
Great ! It works as expected. Even the linker does its job when linking with another object containing a strong symbol with the same name. Thank you Nick !
Created attachment 1778 [details] Write correct offsets into a.out object files There is still a bug when the reference to the weak symbol contains an offset. With my previous patch, the offset is unconditionnaly set to 0. Here is the updated testcase : $ cat bug.s jmp mylabel+2 nop .weak mylabel mylabel: nop nop $ as bug.s -o bug.o $ objdump -d bug.o bug.o: file format a.out-m68k-netbsd Disassembly of section .text: 00000000 <mylabel-0x8>: 0: 4ef9 0000 0000 jmp 0 <mylabel-0x8> 6: 4e71 nop 00000008 <mylabel>: 8: 4e71 nop a: 4e71 nop We can see that the "+2" has disappeared. The attached patch fixes this problem. Here is the result after applying it : $ objdump -d bug.o bug.o: file format a.out-m68k-netbsd Disassembly of section .text: 00000000 <mylabel-0x8>: 0: 4ef9 0000 0002 jmp 2 <mylabel-0x6> 6: 4e71 nop 00000008 <mylabel>: 8: 4e71 nop a: 4e71 nop Now, the right offset is written into the code segment. The reloc and linker output are good, too. I think this time it should be correct. When searching for md_apply_fix() in BFD sources, I found various workarounds against bfd_install_relocation(). ELF targets seems to be OK, but every other target uses its own workaround... I'm sure that there are still hidden bugs with weak symbols. Could someone check this patch, please ? Vincent
Reopen the bug due to the previous comment.
Hi Vincent, Thanks for the revised patch. I have applied it, and added your revised test case as an entry in the m68k gas testsuite. Cheers Nick gas/ChangeLog 2007-05-15 Vincent Riviere <vincent.riviere@freesbee.fr> PR gas/3041 * config/tc-m68k.c (relaxable_symbol): Make sure that the correct addend is stored for relocs against weak symbols. (md_apply_fix): So not loose track of addend for relocs against weak symbols. gas/testsuite/ChangeLog 2007-05-15 Vincent Riviere <vincent.riviere@freesbee.fr> Nick Clifton <nickc@redhat.com> PR gas/3041 * gas/m68k/p3041.s: New test case. * gas/m68k/p3041.d: New expected disassembly. * gas/m68k/all.exp: Run new test for m68k-*-netbsd toolchains. Only run arch-cpu-1 test for ELF based toolchains.
Great ! Thank you, Nick. My testcase passes. But some tests fails : 1) gas/all/weakref1u.d It seems that this test should not be run on a.out targets. I propose to not-target it for m68k-*-netbsd. 2) gas/m68k/br-isa[abc].d They might have been broken by my "addend" fix, in case of relative relocations.
After looking at this a bit closer, I think these 4 failures are unrelated to my patch. Furthermore, I think the actual result is good - atleast for m68k-*- netbsd. But it should be checked by a m68k and a.out guru. Vincent
The resulting addend is still wrong when the referenced symbol is defined as weak in the .data or .bss section. This happens in binutils 2.19.1 and the latest 2.20 from CVS.
Created attachment 4250 [details] Fix addend for weak references to data symbols The attached patch fixes this problem. I included a detailed comment about the workaround, and I updated the testcases. This patch only affects weak symbols on m68k/a.out targets. Please commit it into HEAD and the 2.20 branch, and in the 2.19 branch if it is still opened.
Here are proposals for the ChangeLog. gas: 2009-10-07 Vincent Riviere <vincent.riviere@freesbee.fr> PR gas/3041 * config/tc-m68k.c (tc_gen_reloc): Fix addend for relocations located in data section an referencing a weak symbol. gas/testsuite: 2009-10-07 Vincent Riviere <vincent.riviere@freesbee.fr> PR gas/3041 * gas/m68k/all.exp: Added "p3041data". * gas/m68k/p3041.d, gas/m68k/p3041.s: Added tests of weak references from text section to all possible sections. * gas/m68k/p3041data.d, gas/m68k/p3041data.s: New test. Check weak references from data section.
Subject: Bug 3041 CVSROOT: /cvs/src Module name: src Changes by: nickc@sourceware.org 2009-10-13 08:55:31 Modified files: gas : ChangeLog gas/config : tc-m68k.c gas/testsuite : ChangeLog gas/testsuite/gas/m68k: all.exp p3041.d p3041.s Added files: gas/testsuite/gas/m68k: p3041data.d p3041data.s Log message: gas: 2009-10-07 Vincent Riviere <vincent.riviere@freesbee.fr> PR gas/3041 * config/tc-m68k.c (tc_gen_reloc): Fix addend for relocations located in data section an referencing a weak symbol. gas/testsuite: 2009-10-07 Vincent Riviere <vincent.riviere@freesbee.fr> PR gas/3041 * gas/m68k/all.exp: Added "p3041data". * gas/m68k/p3041.d, gas/m68k/p3041.s: Added tests of weak references from text section to all possible sections. * gas/m68k/p3041data.d, gas/m68k/p3041data.s: New test. Check weak references from data section. Patches: http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/ChangeLog.diff?cvsroot=src&r1=1.3982&r2=1.3983 http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/config/tc-m68k.c.diff?cvsroot=src&r1=1.111&r2=1.112 http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1566&r2=1.1567 http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041data.d.diff?cvsroot=src&r1=NONE&r2=1.1 http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041data.s.diff?cvsroot=src&r1=NONE&r2=1.1 http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/all.exp.diff?cvsroot=src&r1=1.19&r2=1.20 http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041.d.diff?cvsroot=src&r1=1.1&r2=1.2 http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041.s.diff?cvsroot=src&r1=1.1&r2=1.2
Hi Vincent, Thanks for the patches. I have applied them, along with your changelog entries. Cheers Nick
Many thanks, Nick. I see it is too late for 2.20.
This patch probably is relevant for other targets too. I can't ask you to look at them as I fully sympathize with your comment #8 regarding time spent in the debugger.
Yes, I believe other targets using a.out for other processors are affected, too, but I cowardly fixed the bug only for m68k. It would probably be more clean to fix bfd_install_relocation(), but that would affect all the targets and their own workarounds.
Created attachment 5133 [details] Patch for pc-relative weak references In reference to this commit: http://sourceware.org/ml/binutils/2010-09/msg00122.html I confirm there was an additional bug, and that patch fixed it. I have attached a new testcase for checking that. I have also enabled the p3041 testcase for all the m68k-aout targets. Please commit it into the 2.21 branch and the trunk, with the following changelog entry: gas/testsuite: 2010-10-XX Vincent Riviere <vincent.riviere@freesbee.fr> PR gas/3041 * gas/m68k/all.exp: Added "p3041pcrel" and enabled the p3041 tests for all the m68k-aout targets. * gas/m68k/p3041pcrel.d, gas/m68k/p3041pcrel.s: New test. Check pc-relative weak references.
CVSROOT: /cvs/src Module name: src Changes by: amodra@sourceware.org 2011-02-07 00:04:09 Modified files: gas/testsuite : ChangeLog gas/testsuite/gas/m68k: all.exp Added files: gas/testsuite/gas/m68k: p3041pcrel.d p3041pcrel.s Log message: PR gas/3041 * gas/m68k/p3041pcrel.s, * gas/m68k/p3041pcrel.d: New test. * gas/m68k/all.exp: Add "p3041pcrel" and enable p3041 tests for all m68k-aout targets. Patches: http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1844&r2=1.1845 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.d.diff?cvsroot=src&r1=NONE&r2=1.1 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.s.diff?cvsroot=src&r1=NONE&r2=1.1 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/all.exp.diff?cvsroot=src&r1=1.25&r2=1.26
CVSROOT: /cvs/src Module name: src Branch: binutils-2_21-branch Changes by: amodra@sourceware.org 2011-02-07 00:04:44 Modified files: gas/testsuite : ChangeLog gas/testsuite/gas/m68k: all.exp Added files: gas/testsuite/gas/m68k: p3041pcrel.d p3041pcrel.s Log message: PR gas/3041 * gas/m68k/p3041pcrel.s, * gas/m68k/p3041pcrel.d: New test. * gas/m68k/all.exp: Add "p3041pcrel" and enable p3041 tests for all m68k-aout targets. Patches: http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=1.1802.2.4&r2=1.1802.2.5 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.d.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=NONE&r2=1.1.2.1 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.s.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=NONE&r2=1.1.2.1 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/all.exp.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=1.24&r2=1.24.2.1