[PATCH] D61992: [ARM] Support .reloc *, R_ARM_NONE, *

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I think this is looking close. I've got a small concern about what happens on non-ELF platforms Windows and Macho where R_ARM_NONE is not defined. Other than that it is just a few suggestions.



================
Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:50
+Optional<MCFixupKind> ARMAsmBackend::getFixupKind(StringRef Name) const {
+  if (Name == "R_ARM_NONE")
+    return (MCFixupKind)ARM::fixup_arm_NONE;
----------------
Is there a way of making R_ARM_NONE only accepted on ELF. There are also Windows (COFF) and MachO users that may come through this code-path, we need to make sure that they don't get a fixup they can't handle.


================
Comment at: test/MC/ARM/reloc-directive.s:8
+# RUN: llvm-readelf -x .data %t | FileCheck --check-prefix=HEX %s
+
+.data
----------------
Would it be possible to expand the test case to include something like:

```
        .text
        .globl foo
        .type foo, %function
foo:    bx lr

        .data
        .reloc 0, R_ARM_NONE, foo
```
As this will more closely match the real world usage for .reloc.

It may also be worth a test case that doesn't use the filetype=obj to make sure the asmprinter code path works.





================
Comment at: test/MC/ARM/reloc-directive.s:17
+
+# Addends are ignored.
+# CHECK:      0x8 R_ARM_NONE .data 0x0
----------------
I suggest adding the reason they are ignored. Something like # Addends are ignored for SHT_REL relocations with no addend field.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61992