Page MenuHomePhabricator

[LLD][ELF][RISCV] Linker relaxation support for R_RISCV_CALL
Changes PlannedPublic

Authored by s.egerton on Apr 29 2020, 11:06 AM.

Details

Summary

This patch adds linker relaxation support for RISC-V to LLD. It contains a framework for relocation relaxation as well as support for relaxing R_RISCV_CALL relocations. This can be optionally enabled using the LLD “-relax” flag also added in this patch.

This patch is very similar in nature to https://reviews.llvm.org/D77694. Large amounts of this code were already written when this patch was posted however able to make use of some of the code in there for parts not yet implemented. Thank you for this @jrtc27. In a future patch, I plan to use this to integrate R_RISCV_ALIGN support.

Diff Detail

Event Timeline

s.egerton created this revision.Apr 29 2020, 11:06 AM
jrtc27 requested changes to this revision.Apr 29 2020, 11:32 AM

I'd argue more of the comments than those highlighted are also pointless. There's no point writing a comment spelling out what the next line of code clearly does. They should be reserved for overviews of what's going on, and the details of why the code is as it is. I'm also curious as to why you went for the optimisation relaxations rather than R_RISCV_ALIGN. Support for the latter is mandatory for correctness when using -mrelax, but the former is purely optional. Therefore, I would strongly argue that support for R_RISCV_ALIGN should be added before or in the same commit as any R_RISCV_RELAX optimisations, otherwise we're just shipping something that's knowingly broken.

lld/ELF/InputSection.cpp
144–145

Yeah, so, this gives rise to the painful O(n^2) binutils deletion approach. My patch avoids this by gathering up all deletions and performing them at once, giving O(nlogn) instead.

147

Again, fatal error.

158

This is unnecessary. makeMutableDataCopy already updates rawData.

162

Pointless comment.

163

Does not match the comment. Also wrong, as a relocation _at_ loc shouldn't be moved, given the existing relaxed relocation is modified rather than replaced.

164

Pointless comment.

198

// If another section is after this section, but I don't think this comment is necessary, it's just repeating (with a bug) what the code says.

lld/ELF/InputSection.h
199
  1. This should be loc + sizeof(T) > getSize()
  2. This should be a fatal error, not silently ignored
204

This is unnecessary. makeMutableDataCopy already updates rawData.

This revision now requires changes to proceed.Apr 29 2020, 11:32 AM
MaskRay requested changes to this revision.EditedApr 29 2020, 4:14 PM

I'd argue more of the comments than those highlighted are also pointless. There's no point writing a comment spelling out what the next line of code clearly does. They should be reserved for overviews of what's going on, and the details of why the code is as it is.

+1

I'm also curious as to why you went for the optimisation relaxations rather than R_RISCV_ALIGN. Support for the latter is mandatory for correctness when using -mrelax, but the former is purely optional.
Therefore, I would strongly argue that support for R_RISCV_ALIGN should be added before or in the same commit as any R_RISCV_RELAX optimisations, otherwise we're just shipping something that's knowingly broken.

+1. My thought is that we need a plan to support R_RISCV_ALIGN first (lld currently does not work with -mrelax code), then think about R_RISCV_RELAX.
However, there is also a backedge. Implementing R_RISCV_ALIGN (which does not require relocation scanning) requires a think about how it will affect R_RISCV_RELAX (which requires relocation scanning).

Implementing both is tricky due to the intertwined logic with scanRelocations and finalizeAddressDependentContent.

I feel that when https://reviews.llvm.org/D77694#1976922 was posted, the previous discussions were not incorporated.

Still, some comments on the code style part. It seems that the good practice in existing comments are simply ignored.

lld/ELF/Arch/RISCV.cpp
496

clang-format

509

Delete braces around simple statements

lld/test/ELF/riscv-relax-call-5.s
12

check prefixes are upper cased.

20

Use -NEXT: where applicable. This makes test updating easier.

49

Use ## for comments.

95

2 spaces are sufficient for indentation.

s.egerton planned changes to this revision.Apr 30 2020, 10:53 AM

Thank you both for the valuable feedback, I will be working through addressing both of your suggestions.

The logic behind supporting R_RISCV_CALL before R_RISCV_ALIGN is that I hope to implement more optimisation relaxations and wanted to have one in place to act as a base. Additionally, I did not expect this to land before R_RISCV_ALIGN support is present.

Based on your comments about having both is tricky, it may be worth having the two working together in the same patch.

s.egerton updated this revision to Diff 262445.May 6 2020, 12:14 PM

I've made some changes in response to your feedback. No R_RISCV_ALIGN support
added yet but it is work in progress.

Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 12:14 PM
s.egerton planned changes to this revision.May 6 2020, 12:19 PM
s.egerton marked 11 inline comments as done.

As mentioned, I plan to add R_RISCV_ALIGN support in future along with the suggested changes to the test cases.

lld/ELF/InputSection.cpp
151

Missed this one, will remove this in the next revision.

jrtc27 added inline comments.May 6 2020, 2:24 PM
lld/ELF/InputSection.cpp
175

So this is still quadratic (O(nm)).

192

Also quadratic.