Created attachment 8474 [details] Patch Currently, the OpenRISC1000 relocations R_OR1K_{8,16,32}_PCREL have pc_relative set to TRUE and pcrel_offset to FALSE. This causes incorrect linking of .eh_frame and .gcc_except_table sections, which typically contain PC-relative data relocations when the source file is compiled with -fPIC: with pcrel_offset=FALSE, the relocation value is the difference between the section start and the target, and with pcrel_offset=TRUE, the value is the difference between the relocation position and its target. DWARF specifies the latter. This appears to be a copy-paste bug, since the target after which elf-or1k.c was patterned, microblaze, has pcrel_offset=TRUE for *_{8,16,32}_PCREL relocations, as do most other targets. It has likely gone unnoticed because no one has used exceptions with OpenRISC before.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=dbac553d28887561e3f154654ec8e70195d89943 commit dbac553d28887561e3f154654ec8e70195d89943 Author: Peter Zotov <whitequark@whitequark.org> Date: Tue Aug 11 17:12:21 2015 +0100 Fix encoding or OpenRisk1000 PC relative relocations. PR ld/18759 * elf32-or1k.c (R_OR1K_32_PCREL): Set pcrel_offset to TRUE. (R_OR1K_16_PCREL): Likewise. (R_OR1K_8_PCREL): Likewise.
Hi Peter, Thanks for the bug report and patch. I have now checked your fix in. Cheers Nick
This patch wasn't tested. It causes FAIL: ld-elf/merge. By the look of or1k gas does not producing addends suited to pcrel relocs with pcrel_offset true.
I will take a look at this soon.
I have looked at this and it is not a bug I've introduced. The test xfails or32, which is how or1k used to be named. The test was not updated, which has in fact hidden this very bug. And I think ld-elf/eh-frame-hdr currently erroneously xfails or1k, which is another reason this bug was not visible. ld-elf/merge should be updated to xfail or1k, and ld-elf/eh-frame-hdr should be updated to not xfail or1k.
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9b57267f4ffa4f8a168f89630a4b68fb51a419de commit 9b57267f4ffa4f8a168f89630a4b68fb51a419de Author: Alan Modra <amodra@gmail.com> Date: Wed Aug 12 19:07:26 2015 +0930 Revert "Fix encoding or OpenRisk1000 PC relative relocations." This reverts commit dbac553d28887561e3f154654ec8e70195d89943. PR ld/18759 * elf32-or1k.c: Revert 2015-08-11 change.
Why did you revert this?
The test being disabled for or32 is not relevant. The test can fail for all sorts of reasons, most commonly that string merge is not supported by a target. What is relevant is that the test exercises R_OR1K_32_PCREL relocations, and produces an incorrect binary after your patch. Clearly, your patch is responsible so I have reverted it. Note that changing pcrel_offset is an ABI change. It is quite possible to define pcrelative relocations such that pcrel_offset should be false (but this would be unusual for ELF). So without looking at the ABI I can't say whether your patch is wrong or just lacks a gas change.
I have confirmed that, at least, gas emits PC-relative relocations incorrectly. I will fix that. Speaking of the ABI: OR1K does not have a document (any document) which specifies the R_OR1K_*_PCREL relocations. In fact, I believe that binutils is the only software that implements them.
Ok, I took a shot at fixing gas today. To recap... Here is the minimal testcase: .section .data1 .long 0 x: .long 0 .section .data2 .long 0 .long 0 y: .long (x-.) To compile: $ llvm-mc -triple=or1k test.s -filetype=obj -o mc.o $ ./gas/as-new test.s -o as.o We can now examine the relocations. $ ./binutils/objdump -r mc.o RELOCATION RECORDS FOR [.data2]: OFFSET TYPE VALUE 00000008 R_OR1K_32_PCREL .data1+0x00000004 $ ./binutils/objdump -r as.o RELOCATION RECORDS FOR [.data2]: OFFSET TYPE VALUE 00000008 R_OR1K_32_PCREL .data1-0x00000004 LLVM emits the correct value: the address of the `x` symbol. gas does not: it emits the address of the `x` symbol minus the offset of `y` in .data2. That is, it emits the relocation relative to the start of the section, as if pcrel_offset was FALSE. We can link the files and confirm that this is inconsistent with what ld does: $ ./ld/ld-new -shared mc.o -o mc.so $ ./ld/ld-new -shared as.o -o as.so $ ./binutils/objdump -s -j .data1 -j .data2 mc.so Contents of section .data1: 216c 00000000 00000000 ........ Contents of section .data2: 0000 00000000 00000000 00002168 ..........!h $ ./binutils/objdump -s -j .data1 -j .data2 as.so Contents of section .data1: 2180 00000000 00000000 ........ Contents of section .data2: 0000 00000000 00000000 00002174 ..........!t The place where gas decides the addend to emit is in tc_gen_reloc in tc-or1k.c. Currently, it delegates to gas_cgen_tc_gen_reloc (which is used in several other backends, none of which passes the ld-elf/merge test). gas_cgen_tc_gen_reloc uses fixP->fx_addnumber as the addend in all cases except when emitting vtable relocations, in which case it uses fixP->fx_offset. fixP->fx_addnumber is a scratch field which in case of cgen is set to *valP in gas_cgen_md_apply_fix. *valP=-4, which comes from fixup_segment in write.c:978: else if (sub_symbol_segment == this_segment && !S_FORCE_RELOC (fixP->fx_subsy, 0) && !TC_FORCE_RELOCATION_SUB_LOCAL (fixP, add_symbol_segment)) { add_number -= S_GET_VALUE (fixP->fx_subsy); fixP->fx_offset = (add_number + fixP->fx_dot_value + fixP->fx_dot_frag->fr_address); This code calculates add_number=-4, which is later passed as valP. It also recalculates the offset to the target symbol and assigns it fixP->fx_offset; the value it assigns is identical to the value that was there before. After that, it seems to do something else for pc-relative relocations in this condition at write.c:1053: if (fixP->fx_pcrel) { add_number -= MD_PCREL_FROM_SECTION (fixP, this_segment); ... except it doesn't; OR1K md_pcrel_from_section always returns 0 for fixP->fx_addsy which is in different section than the current one, as would always be the case when the assembler has to emit a relocation. Anyway, we're finally at write.c:1066, and invoking md_apply_fix ~ gas_cgen_md_apply_fix in or1k: if (!fixP->fx_done) md_apply_fix (fixP, &add_number, this_segment); After gas_cgen_md_apply_fix sets fixP->fx_addnumber to add_number, the next relevant bit of code is in gas_cgen_tc_gen_reloc is at cgen.c:1069: /* Use fx_offset for these cases. */ if (fixP->fx_r_type == BFD_RELOC_VTABLE_ENTRY || fixP->fx_r_type == BFD_RELOC_VTABLE_INHERIT) reloc->addend = fixP->fx_offset; else reloc->addend = fixP->fx_addnumber; ... which just puts the incorrect value it into the reloc->addend=-4. It seems that somewhere in the cgen code above there is a bug. I don't really know where and there is not enough information in the comments to determine. I am including the writeup above in case someone is motivated enough to figure that out. What I did is I ripped out the entire tc_gen_reloc in tc-or1k.c and replaced it with a sane and much simpler version from tc-aarch64.c. It just sets reloc->addend to fixp->fx_offset. I'm attaching the complete updated patch. It passes the entire testsuite.
Created attachment 8624 [details] Patch
(In reply to whitequark from comment #11) > Created attachment 8624 [details] I may be going mad here, but it looks to me like this patch has already been checked in... Cheers Nick
Nick, that can't be right. I've just looked at binutils gitweb (e.g. https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/elf32-or1k.c;h=a1eba0956e459653a4867d60d9a262c9cd51a60e;hb=HEAD) and it's not there.
(In reply to whitequark from comment #13) > Nick, that can't be right. I've just looked at binutils gitweb Ah - found it - for some reason your patch is reversed... OK so I will reverse apply the patch and then test it. Cheers Nick
Ah right, sorry. I messed up the git invocation somehow, it seems.
(In reply to whitequark from comment #11) I still see a linker testsuite failure for eh-frame-hdr. Looking in the log I see: Executing on host: sh -c {./ld-new -Lbinutils/current/ld/testsuite/ld-elf -e _start --eh-frame-hdr -o tmpdir/dump tmpdir/dump0.o 2>&1} /dev/null ld.tmp (timeout = 300) spawn [open ...] ./ld-new: unrecognized option '--eh-frame-hdr' ./ld-new: use the --help option for usage information failed with: <./ld-new: unrecognized option '--eh-frame-hdr' How did you configure your ork1 toolchain ? I am using "--target=or1k-elf" which may be why this is not working. Cheers Nick
I use --target=or1k-linux, since I need to generate shared object files. I still run them on bare metal but it looks like -linux is the closest I have.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8a9e7a9121490a8c64d8c17f5be510e43104f6d9 commit 8a9e7a9121490a8c64d8c17f5be510e43104f6d9 Author: Peter Zotov <whitequark@whitequark.org> Date: Fri Sep 25 15:21:14 2015 +0100 Correct the generation of OR1K pc-relative relocations. gas PR ld/18759 * config/tc-or1k.c (tc_gen_reloc): Correct computation of PC relative relocs. * config/tc-or1k.h (GAS_CGEN_PRCEL_R_TYPE): Delete. bfd * elf32-or1k.c (R_OR1K_32_PCREL): Set pcrel_offset to TRUE. (R_OR1K_16_PCREL): Likewise. (R_OR1K_8_PCREL): Likewise. ld/tests * ld-elf/eh-frame-hdr: Expect to pass on the or1k-linux target.
Hi Peter, OK - I have applied your patch, along with a small change the eh-frame-hdr.d file so that the test is only expected to pass for the or1k-linux variant, and the addition of changelog entries. Cheers Nick
Thanks!