Page MenuHomePhabricator

[MC][RISCV] Make .reloc support arbitrary relocation types
ClosedPublic

Authored by MaskRay on Mar 29 2020, 10:25 AM.

Details

Summary

Similar to D76746 (ARM), D76754 (AArch64) and llvmorg-11-init-6967-g152d14da64c (x86)

Diff Detail

Event Timeline

MaskRay created this revision.Mar 29 2020, 10:25 AM
MaskRay updated this revision to Diff 253433.Mar 29 2020, 10:27 AM

Forgot to upload reloc-directive-err.s

I think this looks good. I'll let @lenary have another look.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
93–94

Shouldn't this also check the upper bound?

385–386

Ditto.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
55–56

Ditto.

MaskRay marked 2 inline comments as done.Apr 2 2020, 10:59 AM
MaskRay added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
93–94

Not very necessarily.

D76746 touched the upper bound. The point of the upper bound is actually quite weak: it is used as an assert in MCFixup::create to reject certain apparently invalid relocations.

I want to keep the code as-is to be consistent with changes I made to ARM/AArch64/X86/PowerPC.

MaskRay marked an inline comment as done.Apr 7 2020, 1:30 PM

Ping

I think this looks good, but I definitely feel out of my depth in evaluating it, not really knowing this part of the backend and any invariants that need to be preserved. For this reason I'm unwilling to give an explicit approval/rejection.

I do think the testing seems adequate, and this is a useful feature that we should enable.

Thanks! I have made very similar changes to x86/PPC/AArch64/ARM. I think I am well-versed in MC. Given no objection, I will push this.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 10 2020, 10:44 AM
This revision was automatically updated to reflect the committed changes.