[RISCV] Add diff relocation support for RISC-V
Needs ReviewPublic

Authored by simoncook on Mon, Apr 2, 1:24 PM.

Details

Reviewers
asb
edward-jones
Summary

For RISC-V it is desirable to have relaxation happen in the linker once addresses are known, and as such the size between two instructions/byte sequences in a section could change.

For most assembler expressions, this is fine, as the absolute address results in the expression being converted to a fixup, and finally relocations. However, for expressions such as .quad .L2-.L1, the assembler folds this down to a constant once fragments are laid out, under the assumption that the difference can no longer change, although in the case of linker relaxation the differences can change at link time, so the constant is incorrect. One place where this commonly appears is in debug information, where the size of a function expression is in a form similar to the above.

This patch extends the assembler to allow an AsmBackend to declare that it does not want the assembler to fold down this expression, and instead generate a pair of relocations that allow the linker to carry out the calculation. In this case, the expression is not folded, but when it comes to emitting a fixup, the generic FK_Data_* fixups are converted into a pair, one for the addition half, one for the subtraction, and this is passed to the relocation generating methods as usual. I have named these FK_Data_Add_* and FK_Data_Sub_* to indicate which half these are for.

For RISC-V, which supports this via e.g. the R_RISCV_ADD64, R_RISCV_SUB64 pair of relocations, these are also set to always emit relocations relative to local symbols rather than section offsets. This is to deal with the fact that if relocations were calculated on e.g. .text+8 and .text+4, the result 12 would be stored rather than 4 as both addends are added in the linker.

Diff Detail

Repository
rL LLVM
simoncook created this revision.Mon, Apr 2, 1:24 PM
mgrang added a subscriber: mgrang.Wed, Apr 4, 5:13 PM
apazos added inline comments.Wed, Apr 4, 6:01 PM
include/llvm/MC/MCFixup.h
48

Comments need to be updated:
FK_Data_Add_8 -> A eight byte..
FK_Data_Sub_1 -> A one-byte..

lib/MC/MCAssembler.cpp
686

Comment update:
one for the add, and one for the addition, -> one for the add, and one for the sub,

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
39

Should this setting be conditional on the relax flag?
The relax flag is being introduced in https://reviews.llvm.org/D44888

asb added a comment.Thu, Apr 5, 4:05 AM

Thanks for this Simon.

Perhaps requiresDiffExpressionRelocations or similar might be better than supportsEmittingDiffExpressions? As Ana says, you might want to stick with the current behaviour when -mno-relax is set.

Just to make sure I understand the scope of this patch properly:

  • As far as you currently understand, does this cover all cases where constants are emitted where relocations are needed in the presence of linker relaxation. i.e. are there other known cases where you have planned follow-ups?
  • Does anyone know of other backends that could be affected by this issue? Linker relaxation seems to used somewhat sparingly as far as I can see, with targets like ARM preferring the less invasive 'range thunks'.
lib/MC/MCAssembler.cpp
684

different -> difference

asb added a comment.Thu, Apr 5, 4:12 AM

From a quick look in binutils, it seems micromips could be tripped up by similar issues.

mgrang added inline comments.Thu, Apr 5, 8:13 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
40

Remove empty line.

simoncook updated this revision to Diff 141454.Fri, Apr 6, 4:56 PM
simoncook edited the summary of this revision. (Show Details)

I've updated the patch to respond to comments, and deal with the couple of test failures I was seeing.

The only one I think may need some thoughts on is the changes I made to hilo-constaddr.s: Using %lo on an expression won't work in this case, gas doesn't accept it as input, and in the relaxation case I don't believe there are RISC-V relocations that could represent such an expression. I've split out the test case into two files, one for the working case, and one for the input I would consider invalid.

Does this seem reasonable, I think we might want to reject this all the time, and not just when linker relaxation is enabled. Otherwise I think it could cause confusion that some expressions are accepted by the assembler only when relaxation is on. (For reference, this is the change in RISCVMCExpr.cpp) Any thoughts on this?

Alex, as far as I'm aware, this covers all cases where constants are emitted from what I've seen so far.

Ana, we can tie this to linker relaxation, looking at D44886 which I think adds the MC part of conditional relaxation, my change in the AsmBackend can be easily swapped from return true to querying the SubTargetInfo for return enableLinkerRelax() when that lands.

simoncook marked 4 inline comments as done.Fri, Apr 6, 4:57 PM
simoncook updated this revision to Diff 141458.Fri, Apr 6, 5:09 PM

The previous diff had one change missing from my original patch, that is now restored.

simoncook updated this revision to Diff 141512.Sat, Apr 7, 2:16 PM
simoncook retitled this revision from [RISCV WIP] Add diff relocation support for RISC-V to [RISCV] Add diff relocation support for RISC-V.

Take part of test I removed as it now produces and error, and turn it into an explicit test for an error message.

asb added a comment.Thu, Apr 12, 6:26 AM

I think this patch and its tests would be best if based on D44886, so it can be clear which behaviour is conditional on linker relaxation being enabled.

I'd be happy for hilo-constaddr-invalid.s to always be rejected. It never worked in gas, and we can always support it for builds without linker relaxation in the future. Perhaps add a note to the test file to document that we're unconditionally rejecting for now for consistency, but that it would be easy in the future to support when linker relaxation is disabled.

lib/MC/MCAssembler.cpp
685

indiciated -> indicated

simoncook updated this revision to Diff 142607.Mon, Apr 16, 3:29 AM

I've rebased the change on top of D44886 to indicate what is conditional based on linker relaxation.

I haven't made hilo-constaddr-invalid.s rejected unconditionally. I took a look at how to do this, and it seems it's more difficult to do that (considering fixup types on parent expressions when evaluating subexpressions) than it may be worth it, than to have this accepted based on the linker flag. If the latter is the behavior we want in the end, we already have that and I propose leaving it this way around.

simoncook marked 2 inline comments as done.Mon, Apr 16, 3:29 AM
asb added inline comments.Mon, Apr 16, 8:10 AM
test/MC/RISCV/hilo-constaddr.s
3–6

We shouldn't lose the CHECK-REL check from this file, and probably would want to keep the check for the difference of two labels.

asb added a subscriber: sdardis.Tue, Apr 17, 5:59 AM

CC @sdardis - micromips seems to use aggressive linker relaxation in binutils ld meaning this may have some current or future relevance to the Mips LLVM backend.

simoncook updated this revision to Diff 143061.Thu, Apr 19, 2:05 AM

I've readded the reloc check line to hilo-constaddr.s and renamed hilo-constaddr-invalid.s to hilo-constaddr-expr.s since it is no longer checking something that is always invalid.

simoncook marked an inline comment as done.Thu, Apr 19, 2:05 AM