MCValue: Store SymA specifier at Specifier

The relocation specifier should be accessed via MCValue::Specifier.
However, some targets encode the relocation specifier within SymA using
MCSymbolRefExpr::SubclassData and access it via getAccessVariant(), though
this method is now deprecated.

This change stores the SymA specifier at Specifier as well, unifying the
two code paths.

* CSKY: GOT- and PLT- relocations now suppress the STT_SECTION
  conversion.
* AArch64: https://reviews.llvm.org/D156505 added `getRefkind` check to
  prevent folding. This is a hack and is now removed.

MCValue: Unify relocation specifier storage by storing SymA specifier at Specifier

The relocation specifier is accessed via MCValue::Specifier, but some
targets encoded it within SymA using MCSymbolRefExpr::SubclassData and
retrieved it through the now-deprecated getAccessVariant() method. This
commit unifies the two approaches by storing the SymA specifier at
`Specifier` as well.

Additional changes:

- CSKY: GOT- and PLT- relocations now suppress STT_SECTION conversion.
- AArch64: Removed the `getRefkind` check hack (introduced in https://reviews.llvm.org/D156505) that prevented folding.
  Removed the assertion from `getRelocType`.
- RISCV: Removed the assertion from `getRelocType`.

Future plans:

- Replace MCSymbolRefExpr members with MCSymbol within MCValue.
- Remove `getSymSpecifier` (added for migration).

Author: MaskRay

llvm/include/llvm/MC/MCExpr.h

@@ -135,7 +135,8 @@ class MCExpr {
   static bool evaluateSymbolicAdd(const MCAssembler *, const SectionAddrMap *,
                                   bool, const MCValue &,
                                   const MCSymbolRefExpr *,
-                                  const MCSymbolRefExpr *, int64_t, MCValue &);
+                                  const MCSymbolRefExpr *, int64_t, uint32_t,
+                                  MCValue &);
 };
 
 inline raw_ostream &operator<<(raw_ostream &OS, const MCExpr &E) {

llvm/include/llvm/MC/MCValue.h

@@ -36,6 +36,10 @@ class MCValue {
   int64_t Cst = 0;
   uint32_t Specifier = 0;
 
+  // SymA has a relocation specifier. This is a workaround for targets
+  // that encode specifiers within MCSymbolRefExpr::SubclassData.
+  bool SymSpecifier = false;
+
   // SymB cannot have a specifier. Use getSubSym instead.
   const MCSymbolRefExpr *getSymB() const { return SymB; }
 
@@ -67,18 +71,23 @@ class MCValue {
 
   // Get the relocation specifier from SymA. This is a workaround for targets
   // that do not use MCValue::Specifier.
-  uint16_t getSymSpecifier() const { return SymA->getSpecifier(); }
-  // Get the relocation specifier from SymA, or 0 when SymA is null.
-  uint16_t getAccessVariant() const;
+  uint16_t getSymSpecifier() const { return Specifier; }
+  uint16_t getAccessVariant() const { return Specifier; }
 
   static MCValue get(const MCSymbolRefExpr *SymA,
-                     const MCSymbolRefExpr *SymB = nullptr,
-                     int64_t Val = 0, uint32_t RefKind = 0) {
+                     const MCSymbolRefExpr *SymB = nullptr, int64_t Val = 0,
+                     uint32_t Specifier = 0) {
     MCValue R;
     R.Cst = Val;
     R.SymA = SymA;
     R.SymB = SymB;
-    R.Specifier = RefKind;
+    R.Specifier = Specifier;
+    assert(!(Specifier && SymA && SymA->getSpecifier()) &&
+           "Specifier cannot be used with legacy SymSpecifier");
+    if (!Specifier && SymA && SymA->getSpecifier()) {
+      R.Specifier = SymA->getSpecifier();
+      R.SymSpecifier = true;
+    }
     return R;
   }
 

llvm/lib/MC/MCAssembler.cpp

@@ -168,7 +168,7 @@ bool MCAssembler::evaluateFixup(const MCFixup &Fixup, const MCFragment *DF,
       IsResolved = false;
     } else {
       auto &SA = *Target.getAddSym();
-      if (Target.getSymSpecifier() || SA.isUndefined()) {
+      if (Target.SymSpecifier || SA.isUndefined()) {
         IsResolved = false;
       } else {
         IsResolved = (FixupFlags & MCFixupKindInfo::FKF_Constant) ||

llvm/lib/MC/MCExpr.cpp

@@ -439,7 +439,7 @@ bool MCExpr::evaluateSymbolicAdd(const MCAssembler *Asm,
                                  const MCValue &LHS,
                                  const MCSymbolRefExpr *RhsAdd,
                                  const MCSymbolRefExpr *RhsSub, int64_t RHS_Cst,
-                                 MCValue &Res) {
+                                 uint32_t RhsSpec, MCValue &Res) {
   const MCSymbol *LHS_A = LHS.getAddSym();
   const MCSymbol *LHS_B = LHS.getSubSym();
   int64_t LHS_Cst = LHS.getConstant();
@@ -451,16 +451,16 @@ bool MCExpr::evaluateSymbolicAdd(const MCAssembler *Asm,
   int64_t Result_Cst = LHS_Cst + RHS_Cst;
 
   // If we have a layout, we can fold resolved differences.
-  if (Asm) {
+  if (Asm && !LHS.getSpecifier() && !RhsSpec) {
     // While LHS_A-LHS_B and RHS_A-RHS_B from recursive calls have already been
     // folded, reassociating terms in
     //   Result = (LHS_A - LHS_B + LHS_Cst) + (RHS_A - RHS_B + RHS_Cst).
     // might bring more opportunities.
-    if (LHS_A && RHS_B && !LHS.getSymA()->getSpecifier()) {
+    if (LHS_A && RHS_B) {
       attemptToFoldSymbolOffsetDifference(Asm, Addrs, InSet, LHS_A, RHS_B,
                                           Result_Cst);
     }
-    if (RHS_A && LHS_B && !RhsAdd->getSpecifier()) {
+    if (RHS_A && LHS_B) {
       attemptToFoldSymbolOffsetDifference(Asm, Addrs, InSet, RHS_A, LHS_B,
                                           Result_Cst);
     }
@@ -476,7 +476,12 @@ bool MCExpr::evaluateSymbolicAdd(const MCAssembler *Asm,
   auto *B = LHS_B ? LHS.getSymB() : RHS_B ? RhsSub : nullptr;
   if (B && B->getKind() != MCSymbolRefExpr::VK_None)
     return false;
+  auto Spec = LHS.getSpecifier();
+  if (!Spec)
+    Spec = RhsSpec;
   Res = MCValue::get(A, B, Result_Cst);
+  Res.Specifier = Spec;
+  Res.SymSpecifier = 0;
   return true;
 }
 
@@ -634,9 +639,6 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
         return false;
       case MCBinaryExpr::Add:
       case MCBinaryExpr::Sub:
-        // TODO: Prevent folding for AArch64 @AUTH operands.
-        if (LHSValue.getSpecifier() || RHSValue.getSpecifier())
-          return false;
         if (Op == MCBinaryExpr::Sub) {
           std::swap(RHSValue.SymA, RHSValue.SymB);
           RHSValue.Cst = -(uint64_t)RHSValue.Cst;
@@ -652,7 +654,8 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
           return true;
         }
         return evaluateSymbolicAdd(Asm, Addrs, InSet, LHSValue, RHSValue.SymA,
-                                   RHSValue.SymB, RHSValue.Cst, Res);
+                                   RHSValue.SymB, RHSValue.Cst,
+                                   RHSValue.SymSpecifier, Res);
       }
     }
 

llvm/lib/MC/MCValue.cpp

@@ -43,9 +43,3 @@ LLVM_DUMP_METHOD void MCValue::dump() const {
   print(dbgs());
 }
 #endif
-
-uint16_t MCValue::getAccessVariant() const {
-  if (!SymA)
-    return 0;
-  return SymA->getSpecifier();
-}

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

@@ -426,8 +426,13 @@ void AArch64AsmBackend::applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
     AArch64MCExpr::Specifier SymLoc = AArch64MCExpr::getSymbolLoc(RefKind);
     if (SymLoc == AArch64AuthMCExpr::VK_AUTH ||
         SymLoc == AArch64AuthMCExpr::VK_AUTHADDR) {
+      const auto *Expr = dyn_cast<AArch64AuthMCExpr>(Fixup.getValue());
+      if (!Expr) {
+        Asm.getContext().reportError(Fixup.getValue()->getLoc(),
+                                     "expected relocatable expression");
+        return;
+      }
       assert(Value == 0);
-      const auto *Expr = cast<AArch64AuthMCExpr>(Fixup.getValue());
       Value = (uint64_t(Expr->getDiscriminator()) << 32) |
               (uint64_t(Expr->getKey()) << 60) |
               (uint64_t(Expr->hasAddressDiversity()) << 63);

llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp

@@ -116,12 +116,6 @@ unsigned AArch64ELFObjectWriter::getRelocType(MCContext &Ctx,
   AArch64MCExpr::Specifier SymLoc = AArch64MCExpr::getSymbolLoc(RefKind);
   bool IsNC = AArch64MCExpr::isNotChecked(RefKind);
 
-  assert((!Target.getAddSym() ||
-          Target.getSymSpecifier() == AArch64MCExpr::None ||
-          Target.getSymSpecifier() == AArch64MCExpr::VK_PLT ||
-          Target.getSymSpecifier() == AArch64MCExpr::VK_GOTPCREL) &&
-         "Should only be expression-level modifiers here");
-
   switch (SymLoc) {
   case AArch64MCExpr::VK_DTPREL:
   case AArch64MCExpr::VK_GOTTPREL:

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp

@@ -196,9 +196,9 @@ unsigned PPCELFObjectWriter::getRelocType(MCContext &Ctx, const MCValue &Target,
       Type = ELF::R_PPC_ADDR14; // XXX: or BRNTAKEN?_
       break;
     case PPC::fixup_ppc_half16:
-      switch (RefKind) {
+      switch (Modifier) {
       default:
-        break;
+        llvm_unreachable("Unsupported specifier");
       case PPCMCExpr::VK_LO:
         return ELF::R_PPC_ADDR16_LO;
       case PPCMCExpr::VK_HI:
@@ -217,9 +217,7 @@ unsigned PPCELFObjectWriter::getRelocType(MCContext &Ctx, const MCValue &Target,
         return ELF::R_PPC64_ADDR16_HIGHEST;
       case PPCMCExpr::VK_HIGHESTA:
         return ELF::R_PPC64_ADDR16_HIGHESTA;
-      }
-      switch (Modifier) {
-      default: llvm_unreachable("Unsupported Modifier");
+
       case PPCMCExpr::VK_None:
         Type = ELF::R_PPC_ADDR16;
         break;
@@ -373,17 +371,12 @@ unsigned PPCELFObjectWriter::getRelocType(MCContext &Ctx, const MCValue &Target,
       break;
     case PPC::fixup_ppc_half16ds:
     case PPC::fixup_ppc_half16dq:
-      switch (RefKind) {
+      switch (Modifier) {
       default:
         Ctx.reportError(Fixup.getLoc(), "invalid VariantKind");
         return ELF::R_PPC64_NONE;
-      case PPCMCExpr::VK_None:
-        break;
       case PPCMCExpr::VK_LO:
         return ELF::R_PPC64_ADDR16_LO_DS;
-      }
-      switch (Modifier) {
-      default: llvm_unreachable("Unsupported Modifier");
       case PPCMCExpr::VK_None:
         Type = ELF::R_PPC64_ADDR16_DS;
         break;

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp

@@ -50,9 +50,6 @@ unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
                                             const MCValue &Target,
                                             const MCFixup &Fixup,
                                             bool IsPCRel) const {
-  assert((!Target.getAddSym() ||
-          Target.getSymSpecifier() == MCSymbolRefExpr::VK_None) &&
-         "sym@specifier should have been rejected");
   const MCExpr *Expr = Fixup.getValue();
   // Determine the type of the relocation
   unsigned Kind = Fixup.getTargetKind();

llvm/test/MC/AArch64/elf-reloc-ptrauth.s

@@ -164,15 +164,6 @@ _g9:
 // RUN: not llvm-mc -triple=aarch64 -filetype=obj --defsym=ERROBJ=1 %s -o /dev/null 2>&1 | \
 // RUN:   FileCheck %s --check-prefix=ERROBJ
 
-// ERROBJ: :[[#@LINE+1]]:7: error: expected relocatable expression
-.quad sym@AUTH(ia,42) + 1
-
-// ERROBJ: :[[#@LINE+1]]:7: error: expected relocatable expression
-.quad 1 + sym@AUTH(ia,42)
-
-// ERROBJ: :[[#@LINE+1]]:7: error: expected relocatable expression
-.quad 1 + sym@AUTH(ia,42) + 1
-
 // ERROBJ: :[[#@LINE+1]]:7: error: expected relocatable expression
 .quad sym@AUTH(ia,42) + sym@AUTH(ia,42)
 
@@ -181,11 +172,17 @@ _g9:
 // distance remains the same. Leave it in such state as for now since it
 // makes code simpler: subtraction of a non-AUTH symbol and of a constant
 // are handled identically.
-// ERROBJ: :[[#@LINE+1]]:7: error: expected relocatable expression
+// ERROBJ: :[[#@LINE+1]]:7: error: Cannot represent a difference across sections
 .quad _g9@AUTH(ia,42) - _g8
 
-// ERROBJ: :[[#@LINE+1]]:7: error: expected relocatable expression
+// ERROBJ: :[[#@LINE+1]]:7: error: Cannot represent a difference across sections
 .quad _g9@AUTH(ia,42) - _g8@AUTH(ia,42)
 .quad 0
 
+// ERROBJ: :[[#@LINE+1]]:23: error: expected relocatable expression
+.quad sym@AUTH(ia,42) + 1
+
+// ERROBJ: :[[#@LINE+1]]:9: error: expected relocatable expression
+.quad 1 + sym@AUTH(ia,42)
+
 .endif // ERROBJ