[PATCH] D76746: [MC][ARM] Make .reloc support arbitrary relocation types

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 6 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCFixup.h:63
   // later.
-  MaxTargetFixupKind = (1 << 8)
+  // R_AARCH64_IRELATIVE = 1032
+  MaxTargetFixupKind = FirstLiteralRelocationKind + 1032,
----------------
psmith wrote:
> 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,
> ```
Thanks for the wording suggestion!

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


================
Comment at: llvm/test/MC/ARM/reloc-directive-err.s:1
+# RUN: llvm-mc -triple=armv7 %s 2>&1 | FileCheck --check-prefix=PRINT %s
+# RUN: not llvm-mc -filetype=obj -triple=armv7 %s -o /dev/null 2>&1 | FileCheck %s
----------------
grimar wrote:
> grimar wrote:
> > Should this test be `MC/ARM` and use `-triple=armv7`? Could it be a generic test?
> 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.
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/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76746/new/

https://reviews.llvm.org/D76746