Summary: | RISC-V: Linker relaxation may fail if there are R_RISCV_ALIGN type relocations | ||
---|---|---|---|
Product: | binutils | Reporter: | Yiting Wang <yitingwang16> |
Component: | binutils | Assignee: | Jim Wilson <wilson> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | wilson |
Priority: | P2 | ||
Version: | 2.32 | ||
Target Milestone: | --- | ||
Host: | Target: | ||
Build: | Last reconfirmed: | 2019-11-12 00:00:00 | |
Attachments: |
test case
untested patch to fix _bfd_riscv_relax_call |
Description
Yiting Wang
2019-11-10 03:31:16 UTC
Created attachment 12070 [details]
test case
I have a fix locally which changes the common ld/ldlang.c. I can post to binutils mailing list for review. I am not sure whether there is a possibility of a similar problem occurring *between* passes instead of within a single pass. I.e. what if a call in B is within range and so gets relaxed in one pass, then in a later pass B moves down due to relaxation in a previous section - but the destination stays in the same place due to alignment? Is that possible? I've not figured out how to send a proper patch to binutils mailing list. github is much easier. See https://github.com/riscv/riscv-binutils-gdb/pull/185 The way that this should work is that if the call crosses section boundaries, then we need to use the max alignment of any section between the call instruction and the target, including the call and target sections. Except we don't have code to compute that yet, so we just use the maximum alignment of all sections in the output. If the call does not cross section boundaries, then it should only use the alignment of the section it is in. This alignment value is added in to the offset to make sure that we don't relax something that might not fit after alignment is handled. This has the downside that sometimes we refuse to relax something that might be relaxable because of a section with large alignment constraint. But it is safe, in that we never accidentally relax something that should not be relaxed. This is how _bfd_riscv_relax_lui and _bfd_riscv_relax_pc work. They use max_alignment unconditionally, and set max_alignment to the section alignment if the lui and gp are in the same section and it isn't the abs section. This is unfortunately not how _bfd_riscv_relax_call works. It only adds in the alignment if the call and target are not in the same section. I'd call this a bug and that it needs to work the same as the other two functions. I attached a proposed patch that fixes _bfd_riscv_relax_call and adds your example as a testcase. Created attachment 12071 [details]
untested patch to fix _bfd_riscv_relax_call
You can create a patch with git diff and then attach it to the bug report. It needs to be mailed to the binutils list if it gets checked in, but you can always ask someone else to do that for you. Contributing a patch to Binutils requires an FSF copyright assignment. I don't see an assignment in your name, and it isn't clear who you work for. Though from github I see you are in Beijing, so Alibaba and Huawei would be my first two guesses. I see GCC assignments for both of them, but not Binutils assignments. If you are serious about trying to contribute to GNU toolchain, then you should try to get a copyright assignment. Meanwhile, it might be simpler if someone who already has an assignment writes a patch. Generally, I agree with Nelson Chu's github comment that we would prefer to avoid a ldlang.c change if we can. We might be able to get a better result by changing ldlang.c instead of the RISC-V port, but a change to ldlang.c requires a lot more testing because it affects all targets. Hi Jim, I am very grateful to you for fixing the defect very quickly and giving me the instructions on how to contributing a patch to Binutils! My colleague has verified your patch works fine. :) I am a WindRiver engineer, working on VxWorks. I mainly do system software and know very little toolchains. I just tried to contribute a patch. I see what you and Nelson Chu mean, we'd better not modify the common file, however, I didn't know how to fix elfnn-riscv.c. I sure am glad you resolved the defect, thank you again. :) The master branch has been updated by Jim Wilson <wilson@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=c6261a00c3e70dd8e508062ea43a1bcb6d547621 commit c6261a00c3e70dd8e508062ea43a1bcb6d547621 Author: Jim Wilson <jimw@sifive.com> Date: Tue Nov 12 15:50:48 2019 -0800 RISC-V: Fix ld relax failure with calls and align directives. Make _bfd_riscv_relax_call handle section alignment padding same as the _bfd_riscv_relax_lui and _bfd_riscv_relax_pc functions already do. Use the max section alignment if section boundaries are crossed, otherwise the alignment of the containing section. bfd/ PR 25181 * elfnn-riscv.c (_bfd_riscv_relax_call): Always add max_alignment to foff. If sym_sec->output_section and sec->output_section are the same and not *ABS* then set max_alignment to that section's alignment. ld/ PR 25181 * testsuite/ld-riscv-elf/call-relax-0.s: New file. * testsuite/ld-riscv-elf/call-relax-1.s: New file. * testsuite/ld-riscv-elf/call-relax-2.s: New file. * testsuite/ld-riscv-elf/call-relax-3.s: New file. * testsuite/ld-riscv-elf/call-relax.d: New test. * testsuite/ld-riscv-elf/ld-riscv-elf.exp: Run call-relax test. Change-Id: Iaf65cee52345abf1955f36e8e72c4f6cc0db8d9a Fixed on mainline. |