Reduce duplication in pcrel reloc creation

[• espindola]

Summary

This uses the same code for producing pcrel relocations in ELF, COFF and WebAsm.

[• rafael]

Trying to get phab to send email.

[sbc100]

lib/MC/MCAssembler.cpp

202

Maybe move this check down so getSymB is only called once?

[• rafael]

Rebase.
Address review comment.

[rnk]

lib/MC/MCAssembler.cpp

214

Should we check this first before we do work on SymB? It'll predict well.

test/MC/AArch64/error-location.s

7

Can we improve this diagnostic to mention SymbolB and maybe SymbolA? This test case makes it seem like our assembler diagnostics will get harder to understand. I was thinking:

error: No relocation available to represent the subtraction of 'undef' in this expression

[• rafael]

Improve error messages.

[rnk]

lgtm, nice

[• rafael]

Rebased.

[• rafael]

Rebased. Still blocked on the dependency.

[• rafael]

Rebased.

include/llvm/MC/MCWinCOFFObjectWriter.h

class raw_pwrite_stream;
   protected:
     MCWinCOFFObjectTargetWriter(unsigned Machine_);
 
   public:
     virtual ~MCWinCOFFObjectTargetWriter() = default;
 
     unsigned getMachine() const { return Machine; }
     virtual unsigned getRelocType(MCContext &Ctx, const MCReloc &Reloc,
-                                  bool IsCrossSection,
                                   const MCAsmBackend &MAB) const = 0;
     virtual bool recordRelocation(const MCReloc &) const { return true; }
   };
 
   /// \brief Construct a new Win COFF writer instance.
   ///
   /// \param MOTW - The target specific WinCOFF writer subclass.
   /// \param OS - The stream to write to.
   /// \returns The constructed object writer.
   MCObjectWriter *createWinCOFFObjectWriter(MCWinCOFFObjectTargetWriter *MOTW,
                                             raw_pwrite_stream &OS);
 } // end namespace llvm
 
 #endif // LLVM_MC_MCWINCOFFOBJECTWRITER_H

lib/MC/ELFObjectWriter.cpp

case ELF::STB_GNU_UNIQUE:
     return true;
   }
 }
 
 void ELFObjectWriter::recordRelocation(MCAssembler &Asm,
                                        const MCAsmLayout &Layout,
                                        const MCFragment *Fragment,
                                        MCReloc &Fixup) {
-  bool IsPCRel = Asm.getBackend().getFixupKindInfo(Fixup.getKind()).Flags &
-                 MCFixupKindInfo::FKF_IsPCRel;
   MCReloc &Target = Fixup;
   uint64_t &FixedValue = Fixup.getConstant();
   const MCSectionELF &FixupSection = cast<MCSectionELF>(*Fragment->getParent());
   uint64_t C = Target.getConstant();
   uint64_t FixupOffset = Layout.getFragmentOffset(Fragment) + Fixup.getOffset();
-  MCContext &Ctx = Asm.getContext();
-
-  if (const MCSymbol *SymBP = Target.getSymB()) {
-    // Let A, B and C being the components of Target and R be the location of
-    // the fixup. If the fixup is not pcrel, we want to compute (A - B + C).
-    // If it is pcrel, we want to compute (A - B + C - R).
-
-    // In general, ELF has no relocations for -B. It can only represent (A + C)
-    // or (A + C - R). If B = R + K and the relocation is not pcrel, we can
-    // replace B to implement it: (A - R - K + C)
-    if (IsPCRel) {
-      Ctx.reportError(
-          Fixup.getLoc(),
-          "No relocation available to represent this relative expression");
-      return;
-    }
-
-    const MCSymbol &SymB = *SymBP;
-
-    if (SymB.isUndefined()) {
-      Ctx.reportError(Fixup.getLoc(),
-                      Twine("symbol '") + SymB.getName() +
-                          "' can not be undefined in a subtraction expression");
-      return;
-    }
-
-    assert(!SymB.isAbsolute() && "Should have been folded");
-    const MCSection &SecB = SymB.getSection();
-    if (&SecB != &FixupSection) {
-      Ctx.reportError(Fixup.getLoc(),
-                      "Cannot represent a difference across sections");
-      return;
-    }
-
-    Fixup.setKind(Asm.getBackend().getPCRelKind(Fixup.getKind()));
-    uint64_t SymBOffset = Layout.getSymbolOffset(SymB);
-    uint64_t K = SymBOffset - FixupOffset;
-    IsPCRel = true;
-    C -= K;
-  }
+  assert(!Target.getSymB());
 
   // We either rejected the fixup or folded B into C at this point.
   const MCSymbolRefExpr *RefA = Target.getSymA();
   const auto *SymA = RefA ? cast<MCSymbolELF>(&RefA->getSymbol()) : nullptr;
 
   bool ViaWeakRef = false;
   if (SymA && SymA->isVariable()) {
     const MCExpr *Expr = SymA->getVariableValue();

lib/MC/MCAssembler.cpp

 #include "llvm/MC/MCCodeView.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCFixup.h"
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCFragment.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
const MCSymbol *MCAssembler::getAtom(const MCSymbol &S) const {
   if (!getContext().getAsmInfo()->isSectionAtomizableBySymbols(
           *S.getFragment()->getParent()))
     return nullptr;
 
   // Otherwise, return the atom for the containing fragment.
   return S.getFragment()->getAtom();
 }
 
+static bool tryConvertingToPcRel(const MCAsmLayout &Layout,
+                                 const MCFragment &Fragment,
+                                 MCReloc &Reloc) {
+  MCAssembler &Asm = Layout.getAssembler();
+  MCContext &Ctx = Asm.getContext();
+
+  // Don't do this for MachO. One would expect to only need to check
+  // the atom the same way we check the section, but there are still too many
+  // odd broken tests with that.
+  if (Ctx.getObjectFileInfo()->getObjectFileType() == MCObjectFileInfo::IsMachO)
+    return false;
+
+  // Let A, B and C being the components of Target and R be the location of
+  // the relocation. If the relocation is not pcrel, we want to compute (A - B +
+  // C). If it is pcrel, we want to compute (A - B + C - R).
+
+  // If B = R + K and the relocation is not pcrel, we can replace B to
+  // implement it: (A - R - K + C). This is particularly important for
+  // ELF and COFF which don't have relocations that subtract an
+  // arbitrary symbol.
+  const MCSymbol *SymB = Reloc.getSymB();
+  if (!SymB)
+    return false;
+  if (SymB->isUndefined()) {
+    Ctx.reportError(Reloc.getLoc(),
+                    Twine("symbol '") + SymB->getName() +
+                        "' can not be undefined in a subtraction expression");
+    return true;
+  }
+  assert(!SymB->isAbsolute() && "Should have been folded");
+
+  const MCSection &SecB = SymB->getSection();
+  const MCSection &RelocSection = *Fragment.getParent();
+  if (&SecB != &RelocSection) {
+    Ctx.reportError(Reloc.getLoc(),
+                    "Cannot represent a difference across sections");
+    return true;
+  }
+
+  MCFixupKind PCKind = Asm.getBackend().getPCRelKind(Reloc.getKind());
+  uint64_t SymBOffset = Layout.getSymbolOffset(*SymB);
+  uint64_t FixupOffset = Layout.getFragmentOffset(&Fragment) + Reloc.getOffset();
+  uint64_t Delta = SymBOffset - FixupOffset;
+  Reloc.setConstant(Reloc.getConstant() - Delta);
+  Reloc.setKind(PCKind);
+  Reloc.setSymB(nullptr);
+  return false;
+}
+
 bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout, const MCFixup &Fixup,
                                 const MCFragment *DF, MCValue &Target,
                                 MCReloc &Reloc) const {
   ++stats::evaluateFixup;
 
   // FIXME: This code has some duplication with recordRelocation. We should
   // probably merge the two into a single callback that tries to evaluate a
   // fixup and records a relocation if one is needed.
if (Target.getSymB()) {
       }
     }
   } else {
     IsResolved = Target.isAbsolute();
   }
 
   Value = Target.getConstant();
 
+  if (tryConvertingToPcRel(Layout, *DF, Reloc))
+    return true;
+
   // Let the backend force a relocation if needed.
   if (IsResolved && Backend.shouldForceRelocation(*this, Fixup, Target))
     IsResolved = false;
 
   // If we need a relocation, return now. recordRelocation will take
   // care of the rest.
   if (!IsResolved)
     return false;

lib/MC/WasmObjectWriter.cpp

void WasmObjectWriter::recordRelocation(MCAssembler &Asm,
                                         const MCAsmLayout &Layout,
                                         const MCFragment *Fragment,
                                         MCReloc &Fixup) {
   const auto &FixupSection = cast<MCSectionWasm>(*Fragment->getParent());
   MCReloc &Target = Fixup;
   uint64_t &FixedValue = Fixup.getConstant();
   uint64_t C = Target.getConstant();
   uint64_t FixupOffset = Layout.getFragmentOffset(Fragment) + Fixup.getOffset();
-  MCContext &Ctx = Asm.getContext();
   bool IsPCRel = Asm.getBackend().getFixupKindInfo(Fixup.getKind()).Flags &
                  MCFixupKindInfo::FKF_IsPCRel;
-
-  if (const MCSymbol *SymBP = Fixup.getSymB()) {
-    // Let A, B and C being the components of Target and R be the location of
-    // the fixup. If the fixup is not pcrel, we want to compute (A - B + C).
-    // If it is pcrel, we want to compute (A - B + C - R).
-
-    // In general, Wasm has no relocations for -B. It can only represent (A + C)
-    // or (A + C - R). If B = R + K and the relocation is not pcrel, we can
-    // replace B to implement it: (A - R - K + C)
-    if (IsPCRel) {
-      Ctx.reportError(
-          Fixup.getLoc(),
-          "No relocation available to represent this relative expression");
-      return;
-    }
-
-    const MCSymbol &SymB = *SymBP;
-
-    if (SymB.isUndefined()) {
-      Ctx.reportError(Fixup.getLoc(),
-                      Twine("symbol '") + SymB.getName() +
-                          "' can not be undefined in a subtraction expression");
-      return;
-    }
-
-    assert(!SymB.isAbsolute() && "Should have been folded");
-    const MCSection &SecB = SymB.getSection();
-    if (&SecB != &FixupSection) {
-      Ctx.reportError(Fixup.getLoc(),
-                      "Cannot represent a difference across sections");
-      return;
-    }
-
-    uint64_t SymBOffset = Layout.getSymbolOffset(SymB);
-    uint64_t K = SymBOffset - FixupOffset;
-    IsPCRel = true;
-    C -= K;
-  }
+  assert(!Fixup.getSymB());
 
   // We either rejected the fixup or folded B into C at this point.
   const MCSymbolRefExpr *RefA = Target.getSymA();
   const auto *SymA = RefA ? cast<MCSymbolWasm>(&RefA->getSymbol()) : nullptr;
 
   if (SymA && SymA->isVariable()) {
     const MCExpr *Expr = SymA->getVariableValue();
     const auto *Inner = cast<MCSymbolRefExpr>(Expr);

lib/MC/WinCOFFObjectWriter.cpp

void WinCOFFObjectWriter::recordRelocation(MCAssembler &Asm,
 
   MCSection *MCSec = Fragment->getParent();
 
   // Mark this symbol as requiring an entry in the symbol table.
   assert(SectionMap.find(MCSec) != SectionMap.end() &&
          "Section must already have been defined in executePostLayoutBinding!");
 
   COFFSection *Sec = SectionMap[MCSec];
-  const MCSymbol *B = Target.getSymB();
-
-  if (B) {
-    if (!B->getFragment()) {
-      Asm.getContext().reportError(
-          Fixup.getLoc(),
-          Twine("symbol '") + B->getName() +
-              "' can not be undefined in a subtraction expression");
-      return;
-    }
-
-    // Offset of the symbol in the section
-    int64_t OffsetOfB = Layout.getSymbolOffset(*B);
-
-    // Offset of the relocation in the section
-    int64_t OffsetOfRelocation =
-        Layout.getFragmentOffset(Fragment) + Fixup.getOffset();
-
-    FixedValue = (OffsetOfRelocation - OffsetOfB) + Target.getConstant();
-  } else {
-    FixedValue = Target.getConstant();
-  }
+  assert(!Target.getSymB());
 
   COFFRelocation Reloc;
 
   Reloc.Data.SymbolTableIndex = 0;
   Reloc.Data.VirtualAddress = Layout.getFragmentOffset(Fragment);
 
   // Turn relocations for temporary symbols into section relocations.
   if (A.isTemporary()) {
assert(
         "Symbol must already have been defined in executePostLayoutBinding!");
     Reloc.Symb = SymbolMap[&A];
   }
 
   ++Reloc.Symb->Relocations;
 
   Reloc.Data.VirtualAddress += Fixup.getOffset();
   Reloc.Data.Type = TargetObjectWriter->getRelocType(
-      Asm.getContext(), Fixup, B, Asm.getBackend());
+      Asm.getContext(), Fixup, Asm.getBackend());
 
   // FIXME: Can anyone explain what this does other than adjust for the size
   // of the offset?
   if ((Header.Machine == COFF::IMAGE_FILE_MACHINE_AMD64 &&
        Reloc.Data.Type == COFF::IMAGE_REL_AMD64_REL32) ||
       (Header.Machine == COFF::IMAGE_FILE_MACHINE_I386 &&
        Reloc.Data.Type == COFF::IMAGE_REL_I386_REL32))
     FixedValue += 4;

lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp

 class AArch64WinCOFFObjectWriter : public MCWinCOFFObjectTargetWriter {
 public:
   AArch64WinCOFFObjectWriter()
       : MCWinCOFFObjectTargetWriter(COFF::IMAGE_FILE_MACHINE_ARM64) {}
 
   ~AArch64WinCOFFObjectWriter() override = default;
 
   unsigned getRelocType(MCContext &Ctx, const MCReloc &Fixup,
-                        bool IsCrossSection,
                         const MCAsmBackend &MAB) const override;
 
   bool recordRelocation(const MCReloc &) const override;
 };
 
 } // end anonymous namespace
 
 unsigned
-AArch64WinCOFFObjectWriter::getRelocType(MCContext &Ctx, const MCReloc &Fixup,
-                                         bool IsCrossSection,
+AArch64WinCOFFObjectWriter::getRelocType(MCContext &Ctx,
+                                         const MCReloc &Fixup,
                                          const MCAsmBackend &MAB) const {
   const MCReloc &Target = Fixup;
   auto Modifier = Target.isAbsolute() ? MCSymbolRefExpr::VK_None
                                       : Target.getSymA()->getKind();
 
   switch (static_cast<unsigned>(Fixup.getKind())) {
   default: {
     const MCFixupKindInfo &Info = MAB.getFixupKindInfo(Fixup.getKind());

lib/Target/ARM/MCTargetDesc/ARMWinCOFFObjectWriter.cpp

public:
   ARMWinCOFFObjectWriter(bool Is64Bit)
     : MCWinCOFFObjectTargetWriter(COFF::IMAGE_FILE_MACHINE_ARMNT) {
     assert(!Is64Bit && "AArch64 support not yet implemented");
   }
 
   ~ARMWinCOFFObjectWriter() override = default;
 
   unsigned getRelocType(MCContext &Ctx, const MCReloc &Reloc,
-                        bool IsCrossSection,
                         const MCAsmBackend &MAB) const override;
 
   bool recordRelocation(const MCReloc &) const override;
 };
 
 } // end anonymous namespace
 
 unsigned ARMWinCOFFObjectWriter::getRelocType(MCContext &Context,
                                               const MCReloc &Fixup,
-                                              bool IsCrossSection,
                                               const MCAsmBackend &MAB) const {
   const MCReloc &Target = Fixup;
   assert(getMachine() == COFF::IMAGE_FILE_MACHINE_ARMNT &&
          "AArch64 support not yet implemented");
 
   MCSymbolRefExpr::VariantKind Modifier =
     Target.isAbsolute() ? MCSymbolRefExpr::VK_None : Target.getSymA()->getKind();
 

lib/Target/X86/MCTargetDesc/X86WinCOFFObjectWriter.cpp

 namespace {
 
 class X86WinCOFFObjectWriter : public MCWinCOFFObjectTargetWriter {
 public:
   X86WinCOFFObjectWriter(bool Is64Bit);
   ~X86WinCOFFObjectWriter() override = default;
 
   unsigned getRelocType(MCContext &Ctx, const MCReloc &Reloc,
-                        bool IsCrossSection,
                         const MCAsmBackend &MAB) const override;
 };
 
 } // end anonymous namespace
 
 X86WinCOFFObjectWriter::X86WinCOFFObjectWriter(bool Is64Bit)
     : MCWinCOFFObjectTargetWriter(Is64Bit ? COFF::IMAGE_FILE_MACHINE_AMD64
                                           : COFF::IMAGE_FILE_MACHINE_I386) {}
 
 unsigned X86WinCOFFObjectWriter::getRelocType(MCContext &Ctx,
                                               const MCReloc &Fixup,
-                                              bool IsCrossSection,
                                               const MCAsmBackend &MAB) const {
   const MCReloc &Target = Fixup;
   unsigned FixupKind = Fixup.getKind();
-  if (IsCrossSection) {
-    if (FixupKind != FK_Data_4 && FixupKind != llvm::X86::reloc_signed_4byte) {
-      Ctx.reportError(Fixup.getLoc(), "Cannot represent this expression");
-      return COFF::IMAGE_REL_AMD64_ADDR32;
-    }
-    FixupKind = FK_PCRel_4;
-  }
-
   MCSymbolRefExpr::VariantKind Modifier = Target.isAbsolute() ?
     MCSymbolRefExpr::VK_None : Target.getSymA()->getKind();
 
   if (getMachine() == COFF::IMAGE_FILE_MACHINE_AMD64) {
     switch (FixupKind) {
+    case FK_PCRel_1:
+    case FK_PCRel_2:
+    case FK_PCRel_8:
+      Ctx.reportError(Fixup.getLoc(), "Cannot represent this expression");
+      return COFF::IMAGE_REL_AMD64_ADDR32;
     case FK_PCRel_4:
     case X86::reloc_riprel_4byte:
     case X86::reloc_riprel_4byte_movq_load:
     case X86::reloc_riprel_4byte_relax:
     case X86::reloc_riprel_4byte_relax_rex:
       return COFF::IMAGE_REL_AMD64_REL32;
     case FK_Data_4:
     case X86::reloc_signed_4byte:

test/MC/AArch64/error-location.s

 // RUN: not llvm-mc -triple aarch64--none-eabi -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
 
 // Note: These errors are not always emitted in the order in which the relevant
 // source appears, this file is carefully ordered so that that is the case.
 
   .text
 // CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: symbol 'undef' can not be undefined in a subtraction expression
   .word (0-undef)
 
 // CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: expected relocatable expression
   .word -undef
 
-// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: No relocation available to represent this relative expression
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: symbol 'undef' can not be undefined in a subtraction expression
   adr x0, #a-undef
 
 // CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Cannot represent a difference across sections
   .word x_a - y_a
 
 // CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: 1-byte data relocations not supported
   .byte undef
 

test/MC/ARM/error-location.s

 @ RUN: not llvm-mc -triple armv7a--none-eabi -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
 
 @ Note: These errors are not always emitted in the order in which the relevant
 @ source appears, this file is carefully ordered so that that is the case.
 
   .text
 @ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: symbol 'undef' can not be undefined in a subtraction expression
   .word (0-undef)
 
 @ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: expected relocatable expression
   .word -undef
 
-@ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: No relocation available to represent this relative expression
+@ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: symbol 'undef' can not be undefined in a subtraction expression
   adr r0, #a-undef
 
 @ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Cannot represent a difference across sections
   .word x_a - y_a
 
 
 
 w:

test/MC/ELF/bad-expr2.s

 // RUN: not llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o /dev/null \
 // RUN: 2>&1 | FileCheck %s
 
-// CHECK: [[@LINE+2]]:{{[0-9]+}}: error: No relocation available to represent this relative expression
+// CHECK: [[@LINE+2]]:{{[0-9]+}}: error: Cannot represent a difference across sections
 // CHECK-NEXT: call foo - bar
         call foo - bar
 
         .section .foo
 foo:
         .section .bar
 bar: