Page MenuHomePhabricator

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

Authored by MaskRay on Mar 24 2020, 7:33 PM.

Details

Summary

Generalizes D61992. In GNU as, the .reloc directive supports arbitrary relocation types.

A MCFixupKind value V larger than or equal to FirstLiteralRelocationKind
is used to represent the relocation type whose number is V-FirstLiteralRelocationKind.

This is useful for linker tests. Without the feature the assembler
cannot produce certain relocation records (e.g. R_ARM_ALU_PC_G0/R_ARM_LDR_PC_G0)

Diff Detail

Event Timeline

MaskRay created this revision.Mar 24 2020, 7:33 PM
MaskRay updated this revision to Diff 252481.Mar 24 2020, 7:43 PM

Don't crash when an invalid relocation is seen. Add reloc-directive-err.s

MaskRay updated this revision to Diff 252491.Mar 24 2020, 8:54 PM

Raise MaxTargetFixupKind to FirstLiteralRelocationKind + 1032

MaskRay updated this revision to Diff 252493.Mar 24 2020, 9:01 PM

Delete a static local variable

I think this could work, I've got a few comments on the generic part, but otherwise looks reasonable to me. Would be nice to extend this to AArch64 as there are some relocs that GCC/GNU as can emit that llvm-mc does not and it would be good to support these in LLVM.

llvm/include/llvm/MC/MCFixup.h
58

It could be worth making it explicit that the range delimited by LiteralRelocationKind to MaxFixupKind (see next comment) is used for relocations coming from .reloc directive.

58–65

I recommend we rename MaxTargetFixupKind to MaxFixupKind as MaxTargetFixupKind is now 255

I'm guessing R_AARCH64_IRELATIVE is the largest reloc type in use throughout the targets? For a 64-bit RELA relocation this could be any 32-bit value. I think it will be worth giving a little room for aarch64 to expand and explaining what the limit is. For example:

Set limit to accommodate the highest reloc type in use for all Targets, currently R_AARCH64_IRELATIVE at 1032, including room for expansion.
MaxFixupKind = FirstLiteralRelocationKind + 1032 + 32,
grimar added inline comments.Mar 25 2020, 2:29 AM
llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
182

Deserves a comment I think.

llvm/test/MC/ARM/reloc-directive-err.s
2

Should this test be MC/ARM and use -triple=armv7? Could it be a generic test?

grimar added inline comments.Mar 25 2020, 2:34 AM
llvm/test/MC/ARM/reloc-directive-err.s
2

I mean R_INVALID looks like it is a generic invalid relocation and I'd expect to have a generic test for "unknown relocation name" message.
I am not an expert in llvm-mc though, perhaps I am missing something.

MaskRay marked 6 inline comments as done.Mar 25 2020, 11:00 AM
MaskRay added inline comments.
llvm/include/llvm/MC/MCFixup.h
58–65

Thanks for the wording suggestion!

MaxFixupKind is only used once in an assertion to forbid apparently erroneous fixup kinds.

llvm/test/MC/ARM/reloc-directive-err.s
2

This test corresponds to the -1u logic in ARMAsmBackend::getFixupKind. Sadly it is target specific and we need to duplicate the code for each target.

R_INVALID is just an arbitrary name not used as a relocation name (R_ARM_*). It can be anything. I am open to any suggestion if it can make the intention clear.

A llvm-mc test has to be target specific. For general ones we can create tests under test/MC/ or test/MC/ELF/

MaskRay updated this revision to Diff 252628.Mar 25 2020, 11:00 AM
MaskRay marked 6 inline comments as done.

Improve comments

Thanks for the update. I'm happy with the new comments.

I have no more comments either, but I do not feel I am a right person to accept.

Assuming Peter has accepted:) Will commit after local testing.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 27 2020, 12:36 PM
This revision was automatically updated to reflect the committed changes.