[ARM] Support .reloc *, R_ARM_NONE, *

[MaskRay]

Summary

R_ARM_NONE can be used to create references among sections. When
--gc-sections is used, the referenced section will be retained if the
origin section is retained.

Add a generic MCFixupKind FK_NONE as this kind of no-op relocation is
ubiquitous on ELF and COFF, and probably available on many other binary
formats. See D62014.

[MaskRay]

Delete the change to getFixupKindContainerSizeBytes()

[MaskRay]

Set shouldForceRelocation to allow 0 symbol index, e.g. .reloc 0, R_ARM_NONE, 8

[peter.smith]

I get an assertion failure from this patch on the armv7eb-linux-gnueabi test case: llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:872! (the llvm_unreachable)

/// getFixupKindContainerSizeBytes - The number of bytes of the
/// container involved in big endian.
static unsigned getFixupKindContainerSizeBytes(unsigned Kind) {
  switch (Kind) {
  default:
    llvm_unreachable("Unknown fixup kind!");

  case FK_Data_1:
    return 1;
  case FK_Data_2:
    return 2;
  case FK_Data_4:
    return 4;

Looks like you'll need to add case ARM::fixup_arm_NONE:

[MaskRay]

Add back getFixupKindContainerSizeBytes() change

[peter.smith]

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.

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

50 ↗

(On Diff #199756)

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.

test/MC/ARM/reloc-directive.s

8 ↗

(On Diff #199756)

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.

17 ↗

(On Diff #199756)

I suggest adding the reason they are ignored. Something like # Addends are ignored for SHT_REL relocations with no addend field.

[MaskRay]

Improve tests as peter.smith suggested

[MaskRay]

Change to if (STI.getTargetTriple().isOSBinFormatELF() && Name == "R_ARM_NONE")

[MaskRay]

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

50 ↗

(On Diff #199756)

Good point! Added a check STI.getTargetTriple().isOSBinFormatELF()

% llvm-mc -filetype=obj -triple=armv7a-windows-itanium reloc-directive.s -o t.o
reloc-directive.s:17:13: error: unknown relocation name
  .reloc 4, R_ARM_NONE, foo+4

% llvm-mc -filetype=obj -triple=armv7-apple-macho reloc-directive.s -o t.o
reloc-directive.s:17:13: error: unknown relocation name
  .reloc 4, R_ARM_NONE, foo+4

Before, the diagnostic for coff is:

LLVM ERROR: unsupported relocation type: fixup_arm_NONE

macho:

reloc-directive.s:17:3: error: unsupported relocation on symbol                                                 
  .reloc 4, R_ARM_NONE, foo+4

I think X86 and MIPS need a similar fix..

[peter.smith]

Thanks, this looks good to me. May be worth waiting before committing to see if there are any comments from the US timezone.

[MaskRay]

Add generic FK_NONE as suggested by pcc in the R_{386,X86_64}_NONE patch

[MaskRay]

Fix a typo in the test

llvm/trunk/include/llvm/MC/MCFixup.h

 #include "llvm/Support/SMLoc.h"
 #include <cassert>
 
 namespace llvm {
 class MCExpr;
 
 /// Extensible enumeration to represent the type of a fixup.
 enum MCFixupKind {
-  FK_Data_1 = 0, ///< A one-byte fixup.
+  FK_NONE = 0,   ///< A no-op fixup.
+  FK_Data_1,     ///< A one-byte fixup.
   FK_Data_2,     ///< A two-byte fixup.
   FK_Data_4,     ///< A four-byte fixup.
   FK_Data_8,     ///< A eight-byte fixup.
   FK_PCRel_1,    ///< A one-byte pc relative fixup.
   FK_PCRel_2,    ///< A two-byte pc relative fixup.
   FK_PCRel_4,    ///< A four-byte pc relative fixup.
   FK_PCRel_8,    ///< A eight-byte pc relative fixup.
   FK_GPRel_1,    ///< A one-byte gp relative fixup.

llvm/trunk/lib/MC/MCAsmBackend.cpp

 }
 
 Optional<MCFixupKind> MCAsmBackend::getFixupKind(StringRef Name) const {
   return None;
 }
 
 const MCFixupKindInfo &MCAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
   static const MCFixupKindInfo Builtins[] = {
+      {"FK_NONE", 0, 0, 0},
       {"FK_Data_1", 0, 8, 0},
       {"FK_Data_2", 0, 16, 0},
       {"FK_Data_4", 0, 32, 0},
       {"FK_Data_8", 0, 64, 0},
       {"FK_PCRel_1", 0, 8, MCFixupKindInfo::FKF_IsPCRel},
       {"FK_PCRel_2", 0, 16, MCFixupKindInfo::FKF_IsPCRel},
       {"FK_PCRel_4", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"FK_PCRel_8", 0, 64, MCFixupKindInfo::FKF_IsPCRel},

llvm/trunk/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.h

public:
   unsigned getNumFixupKinds() const override {
     return ARM::NumTargetFixupKinds;
   }
 
   // FIXME: this should be calculated per fragment as the STI may be
   // different.
   bool hasNOP() const { return STI.getFeatureBits()[ARM::HasV6T2Ops]; }
 
+  Optional<MCFixupKind> getFixupKind(StringRef Name) const override;
+
   const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override;
 
   bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
                              const MCValue &Target) override;
 
   unsigned adjustFixupValue(const MCAssembler &Asm, const MCFixup &Fixup,
                             const MCValue &Target, uint64_t Value,
                             bool IsResolved, MCContext &Ctx,

llvm/trunk/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

 class ARMELFObjectWriter : public MCELFObjectTargetWriter {
 public:
   ARMELFObjectWriter(uint8_t OSABI)
       : MCELFObjectTargetWriter(/*Is64Bit*/ false, OSABI, ELF::EM_ARM,
                                 /*HasRelocationAddend*/ false) {}
 };
 } // end anonymous namespace
 
+Optional<MCFixupKind> ARMAsmBackend::getFixupKind(StringRef Name) const {
+  if (STI.getTargetTriple().isOSBinFormatELF() && Name == "R_ARM_NONE")
+    return FK_NONE;
+
+  return MCAsmBackend::getFixupKind(Name);
+}
+
 const MCFixupKindInfo &ARMAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
   const static MCFixupKindInfo InfosLE[ARM::NumTargetFixupKinds] = {
       // This table *must* be in the order that the fixup_* kinds are defined in
       // ARMFixupKinds.h.
       //
       // Name                      Offset (bits) Size (bits)     Flags
       {"fixup_arm_ldst_pcrel_12", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_ldst_pcrel_12", 0, 32,
if (A->hasSubsectionsViaSymbols() && Asm.isThumbFunc(&A->getSymbol()) &&
          Kind == ARM::fixup_t2_movt_hi16))
       Value |= 1;
   }
 
   switch (Kind) {
   default:
     Ctx.reportError(Fixup.getLoc(), "bad relocation fixup type");
     return 0;
+  case FK_NONE:
   case FK_Data_1:
   case FK_Data_2:
   case FK_Data_4:
     return Value;
   case FK_SecRel_2:
     return Value;
   case FK_SecRel_4:
     return Value;

 }
 
 bool ARMAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
                                           const MCFixup &Fixup,
                                           const MCValue &Target) {
   const MCSymbolRefExpr *A = Target.getSymA();
   const MCSymbol *Sym = A ? &A->getSymbol() : nullptr;
   const unsigned FixupKind = Fixup.getKind() ;
-  if ((unsigned)Fixup.getKind() == ARM::fixup_arm_thumb_bl) {
+  if (FixupKind == FK_NONE)
+    return true;
+  if (FixupKind == ARM::fixup_arm_thumb_bl) {
     assert(Sym && "How did we resolve this?");
 
     // If the symbol is external the linker will handle it.
     // FIXME: Should we handle it as an optimization?
 
     // If the symbol is out of range, produce a relocation and hope the
     // linker can handle it. GNU AS produces an error in this case.
     if (Sym->isExternal())

 }
 
 /// getFixupKindNumBytes - The number of bytes the fixup may change.
 static unsigned getFixupKindNumBytes(unsigned Kind) {
   switch (Kind) {
   default:
     llvm_unreachable("Unknown fixup kind!");
 
+  case FK_NONE:
+    return 0;
+
   case FK_Data_1:
   case ARM::fixup_arm_thumb_bcc:
   case ARM::fixup_arm_thumb_cp:
   case ARM::fixup_thumb_adr_pcrel_10:
     return 1;
 
   case FK_Data_2:
   case ARM::fixup_arm_thumb_br:

 
 /// getFixupKindContainerSizeBytes - The number of bytes of the
 /// container involved in big endian.
 static unsigned getFixupKindContainerSizeBytes(unsigned Kind) {
   switch (Kind) {
   default:
     llvm_unreachable("Unknown fixup kind!");
 
+  case FK_NONE:
+    return 0;
+
   case FK_Data_1:
     return 1;
   case FK_Data_2:
     return 2;
   case FK_Data_4:
     return 4;
 
   case ARM::fixup_arm_thumb_bcc:

llvm/trunk/lib/Target/ARM/MCTargetDesc/ARMELFObjectWriter.cpp

case ARM::fixup_arm_thumb_blx:
         return ELF::R_ARM_THM_CALL;
       }
     }
   }
   switch ((unsigned)Fixup.getKind()) {
   default:
     Ctx.reportFatalError(Fixup.getLoc(), "unsupported relocation on symbol");
     return ELF::R_ARM_NONE;
+  case FK_NONE:
+    return ELF::R_ARM_NONE;
   case FK_Data_1:
     switch (Modifier) {
     default:
       llvm_unreachable("unsupported Modifier");
     case MCSymbolRefExpr::VK_None:
       return ELF::R_ARM_ABS8;
     }
   case FK_Data_2:

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp

unsigned Idx = Endian == support::little
                        ? (microMipsLEByteOrder ? calculateMMLEIndex(i) : i)
                        : (FullSize - 1 - i);
     Data[Offset + Idx] = (uint8_t)((CurVal >> (i*8)) & 0xff);
   }
 }
 
 Optional<MCFixupKind> MipsAsmBackend::getFixupKind(StringRef Name) const {
   return StringSwitch<Optional<MCFixupKind>>(Name)
-      .Case("R_MIPS_NONE", (MCFixupKind)Mips::fixup_Mips_NONE)
+      .Case("R_MIPS_NONE", FK_NONE)
       .Case("R_MIPS_32", FK_Data_4)
       .Case("R_MIPS_GOT_PAGE", (MCFixupKind)Mips::fixup_Mips_GOT_PAGE)
       .Case("R_MIPS_CALL_HI16", (MCFixupKind)Mips::fixup_Mips_CALL_HI16)
       .Case("R_MIPS_CALL_LO16", (MCFixupKind)Mips::fixup_Mips_CALL_LO16)
       .Case("R_MIPS_CALL16", (MCFixupKind)Mips::fixup_Mips_CALL16)
       .Case("R_MIPS_GOT16", (MCFixupKind)Mips::fixup_Mips_GOT)
       .Case("R_MIPS_GOT_PAGE", (MCFixupKind)Mips::fixup_Mips_GOT_PAGE)
       .Case("R_MIPS_GOT_OFST", (MCFixupKind)Mips::fixup_Mips_GOT_OFST)

 
 const MCFixupKindInfo &MipsAsmBackend::
 getFixupKindInfo(MCFixupKind Kind) const {
   const static MCFixupKindInfo LittleEndianInfos[] = {
     // This table *must* be in same the order of fixup_* kinds in
     // MipsFixupKinds.h.
     //
     // name                    offset  bits  flags
-    { "fixup_Mips_NONE",         0,      0,   0 },
     { "fixup_Mips_16",           0,     16,   0 },
     { "fixup_Mips_32",           0,     32,   0 },
     { "fixup_Mips_REL32",        0,     32,   0 },
     { "fixup_Mips_26",           0,     26,   0 },
     { "fixup_Mips_HI16",         0,     16,   0 },
     { "fixup_Mips_LO16",         0,     16,   0 },
     { "fixup_Mips_GPREL16",      0,     16,   0 },
     { "fixup_Mips_LITERAL",      0,     16,   0 },
getFixupKindInfo(MCFixupKind Kind) const {
   static_assert(array_lengthof(LittleEndianInfos) == Mips::NumTargetFixupKinds,
                 "Not all MIPS little endian fixup kinds added!");
 
   const static MCFixupKindInfo BigEndianInfos[] = {
     // This table *must* be in same the order of fixup_* kinds in
     // MipsFixupKinds.h.
     //
     // name                    offset  bits  flags
-    { "fixup_Mips_NONE",         0,      0,   0 },
     { "fixup_Mips_16",          16,     16,   0 },
     { "fixup_Mips_32",           0,     32,   0 },
     { "fixup_Mips_REL32",        0,     32,   0 },
     { "fixup_Mips_26",           6,     26,   0 },
     { "fixup_Mips_HI16",        16,     16,   0 },
     { "fixup_Mips_LO16",        16,     16,   0 },
     { "fixup_Mips_GPREL16",     16,     16,   0 },
     { "fixup_Mips_LITERAL",     16,     16,   0 },

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp

 unsigned MipsELFObjectWriter::getRelocType(MCContext &Ctx,
                                            const MCValue &Target,
                                            const MCFixup &Fixup,
                                            bool IsPCRel) const {
   // Determine the type of the relocation.
   unsigned Kind = (unsigned)Fixup.getKind();
 
   switch (Kind) {
-  case Mips::fixup_Mips_NONE:
+  case FK_NONE:
     return ELF::R_MIPS_NONE;
   case FK_Data_1:
     Ctx.reportError(Fixup.getLoc(),
                     "MIPS does not support one byte relocations");
     return ELF::R_MIPS_NONE;
   case Mips::fixup_Mips_16:
   case FK_Data_2:
     return IsPCRel ? ELF::R_MIPS_PC16 : ELF::R_MIPS_16;

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsFixupKinds.h

namespace Mips {
   // one can have multiple fixup types for a given relocation and thus need
   // to be uniquely named.
   //
   // This table *must* be in the same order of
   // MCFixupKindInfo Infos[Mips::NumTargetFixupKinds]
   // in MipsAsmBackend.cpp.
   //
   enum Fixups {
-    // Branch fixups resulting in R_MIPS_NONE.
-    fixup_Mips_NONE = FirstTargetFixupKind,
-
     // Branch fixups resulting in R_MIPS_16.
-    fixup_Mips_16,
+    fixup_Mips_16 = FirstTargetFixupKind,
 
     // Pure 32 bit data fixup resulting in - R_MIPS_32.
     fixup_Mips_32,
 
     // Full 32 bit data relative data fixup resulting in - R_MIPS_REL32.
     fixup_Mips_REL32,
 
     // Jump 26 bit fixup resulting in - R_MIPS_26.

llvm/trunk/test/MC/ARM/reloc-directive.s

+# RUN: llvm-mc -triple=armv7-linux-gnueabi %s | FileCheck --check-prefix=PRINT %s
+# RUN: llvm-mc -triple=armv7eb-linux-gnueabi %s | FileCheck --check-prefix=PRINT %s
+
+# RUN: llvm-mc -filetype=obj -triple=armv7-linux-gnueabi %s -o %t
+# RUN: llvm-readobj -r %t | FileCheck %s
+# RUN: llvm-readelf -x .data %t | FileCheck --check-prefix=HEX %s
+
+# RUN: llvm-mc -filetype=obj -triple=armv7eb-linux-gnueabi %s -o %t
+# RUN: llvm-readobj -r %t | FileCheck %s
+# RUN: llvm-readelf -x .data %t | FileCheck --check-prefix=HEX %s
+
+.text
+  bx lr
+  nop
+  nop
+  .reloc 8, R_ARM_NONE, .data
+  .reloc 4, R_ARM_NONE, foo+4
+  .reloc 0, R_ARM_NONE, 8
+
+.data
+.globl foo
+foo:
+  .word 0
+  .word 0
+  .word 0
+
+# PRINT: .reloc 8, R_ARM_NONE, .data
+# PRINT: .reloc 4, R_ARM_NONE, foo+4
+# PRINT: .reloc 0, R_ARM_NONE, 8
+
+# ARM relocations use the Elf32_Rel format. Addends are neither stored in the
+# relocation entries nor applied in the referenced locations.
+# CHECK:      0x8 R_ARM_NONE .data 0x0
+# CHECK-NEXT: 0x4 R_ARM_NONE foo 0x0
+# CHECK-NEXT: 0x0 R_ARM_NONE - 0x0
+
+# HEX: 0x00000000 00000000 00000000