MCValue: Replace MCSymbolRefExpr members with MCSymbol

Commit 0999cbd (2014) introduced
`MCValue::RefKind` for AArch64 ELF as a clean approach to encode the
relocation specifier.

Following numerous migration commits, direct references to getSymA and
getSymB have been eliminated. This allows us to seamlessly update SymA
and SymB, replacing MCSymbolRefExpr with MCSymbol.

Removeing reliance on a MCAssembler::evaluateFixup hack
(`if (Target.SymSpecifier || SA.isUndefined()) {` (previosuly
`if (A->getKind() != MCSymbolRefExpr::VK_None || SA.isUndefined()) {`))
requires 38c3ad3 and 4182d2d

Revert the temporary RISCV/LoongArch workaround
(7e62715) during migration.

MCAssembler::evaluateFixup needs an extra `!Add->isAbsolute()` case
to support `movq abs@GOTPCREL(%rip), %rax; abs = 42` in llvm/test/MC/ELF/relocation-alias.s
(ELFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl asserts if
called on an absolute symbol).

Author: MaskRay

llvm/include/llvm/MC/MCExpr.h

@@ -133,9 +133,7 @@ class MCExpr {
   /// @}
 
   static bool evaluateSymbolicAdd(const MCAssembler *, const SectionAddrMap *,
-                                  bool, const MCValue &,
-                                  const MCSymbolRefExpr *,
-                                  const MCSymbolRefExpr *, int64_t, uint32_t,
+                                  bool, const MCValue &, const MCValue &,
                                   MCValue &);
 };
 
@@ -263,6 +261,9 @@ class MCSymbolRefExpr : public MCExpr {
 
   const MCSymbol &getSymbol() const { return *Symbol; }
 
+  // Some targets encode the relocation specifier within SymA using
+  // MCSymbolRefExpr::SubclassData and access it via getAccessVariant(), though
+  // this method is now deprecated.
   VariantKind getKind() const {
     return (VariantKind)(getSubclassData() & VariantKindMask);
   }

llvm/include/llvm/MC/MCValue.h

@@ -25,40 +25,24 @@ class raw_ostream;
 // Not all targets support SymB. For PC-relative relocations, a specifier is
 // typically used instead of setting SymB to DOT.
 //
-// Some targets encode the relocation specifier within SymA using
-// MCSymbolRefExpr::SubclassData and access it via getAccessVariant(), though
-// this method is now deprecated.
-//
 // This class must remain a simple POD value class, as it needs to reside in
 // unions and similar structures.
 class MCValue {
-  const MCSymbolRefExpr *SymA = nullptr, *SymB = nullptr;
+  const MCSymbol *SymA = nullptr, *SymB = nullptr;
   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; }
-
 public:
   friend class MCAssembler;
   friend class MCExpr;
   MCValue() = default;
   int64_t getConstant() const { return Cst; }
-  const MCSymbolRefExpr *getSymA() const { return SymA; }
   uint32_t getRefKind() const { return Specifier; }
   uint32_t getSpecifier() const { return Specifier; }
   void setSpecifier(uint32_t S) { Specifier = S; }
 
-  const MCSymbol *getAddSym() const {
-    return SymA ? &SymA->getSymbol() : nullptr;
-  }
-  const MCSymbol *getSubSym() const {
-    return SymB ? &SymB->getSymbol() : nullptr;
-  }
+  const MCSymbol *getAddSym() const { return SymA; }
+  const MCSymbol *getSubSym() const { return SymB; }
 
   /// Is this an absolute (as opposed to relocatable) value.
   bool isAbsolute() const { return !SymA && !SymB; }
@@ -72,22 +56,16 @@ 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 Specifier; }
+  // Get the relocation specifier from SymA, or 0 when SymA is null.
   uint16_t getAccessVariant() const { return Specifier; }
 
-  static MCValue get(const MCSymbolRefExpr *SymA,
-                     const MCSymbolRefExpr *SymB = nullptr, int64_t Val = 0,
-                     uint32_t Specifier = 0) {
+  static MCValue get(const MCSymbol *SymA, const MCSymbol *SymB = nullptr,
+                     int64_t Val = 0, uint32_t Specifier = 0) {
     MCValue R;
     R.Cst = Val;
     R.SymA = SymA;
     R.SymB = SymB;
     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

@@ -161,34 +161,23 @@ bool MCAssembler::evaluateFixup(const MCFixup &Fixup, const MCFragment *DF,
     return getBackend().evaluateTargetFixup(*this, Fixup, DF, Target, STI,
                                             Value, WasForced);
 
+  const MCSymbol *Add = Target.getAddSym();
+  const MCSymbol *Sub = Target.getSubSym();
   bool IsPCRel = FixupFlags & MCFixupKindInfo::FKF_IsPCRel;
   bool IsResolved = false;
-  if (IsPCRel) {
-    if (Target.getSubSym() || !Target.getAddSym()) {
-      IsResolved = false;
-    } else {
-      auto &SA = *Target.getAddSym();
-      if (Target.SymSpecifier || SA.isUndefined()) {
-        IsResolved = false;
-      } else {
-        IsResolved = (FixupFlags & MCFixupKindInfo::FKF_Constant) ||
-                     getWriter().isSymbolRefDifferenceFullyResolvedImpl(
-                         *this, SA, *DF, false, true);
-      }
-    }
-  } else {
+  if (!IsPCRel) {
     IsResolved = Target.isAbsolute();
+  } else if (Add && !Sub && !Add->isUndefined() && !Add->isAbsolute()) {
+    IsResolved = (FixupFlags & MCFixupKindInfo::FKF_Constant) ||
+                 getWriter().isSymbolRefDifferenceFullyResolvedImpl(
+                     *this, *Add, *DF, false, true);
   }
 
   Value = Target.getConstant();
-
-  if (const MCSymbol *Add = Target.getAddSym()) {
-    if (Add->isDefined())
-      Value += getSymbolOffset(*Add);
-  }
-  if (const MCSymbol *Sub = Target.getSubSym())
-    if (Sub->isDefined())
-      Value -= getSymbolOffset(*Sub);
+  if (Add && Add->isDefined())
+    Value += getSymbolOffset(*Add);
+  if (Sub && Sub->isDefined())
+    Value -= getSymbolOffset(*Sub);
 
   bool ShouldAlignPC = FixupFlags & MCFixupKindInfo::FKF_IsAlignedDownTo32Bits;
   assert((ShouldAlignPC ? IsPCRel : true) &&
@@ -208,8 +197,8 @@ bool MCAssembler::evaluateFixup(const MCFixup &Fixup, const MCFragment *DF,
   // kind. AVR needs the fixup value to bypass the assembly time overflow with a
   // relocation.
   if (IsResolved) {
-    auto TargetVal = MCValue::get(Target.getSymA(), Target.getSymB(), Value,
-                                  Target.getRefKind());
+    auto TargetVal = Target;
+    TargetVal.Cst = Value;
     if (Fixup.getKind() >= FirstLiteralRelocationKind ||
         getBackend().shouldForceRelocation(*this, Fixup, TargetVal, STI)) {
       IsResolved = false;

llvm/lib/MC/MCExpr.cpp

@@ -436,22 +436,21 @@ static void attemptToFoldSymbolOffsetDifference(const MCAssembler *Asm,
 // early.
 bool MCExpr::evaluateSymbolicAdd(const MCAssembler *Asm,
                                  const SectionAddrMap *Addrs, bool InSet,
-                                 const MCValue &LHS,
-                                 const MCSymbolRefExpr *RhsAdd,
-                                 const MCSymbolRefExpr *RhsSub, int64_t RHS_Cst,
-                                 uint32_t RhsSpec, MCValue &Res) {
+                                 const MCValue &LHS, const MCValue &RHS,
+                                 MCValue &Res) {
   const MCSymbol *LHS_A = LHS.getAddSym();
   const MCSymbol *LHS_B = LHS.getSubSym();
   int64_t LHS_Cst = LHS.getConstant();
 
-  const MCSymbol *RHS_A = RhsAdd ? &RhsAdd->getSymbol() : nullptr;
-  const MCSymbol *RHS_B = RhsSub ? &RhsSub->getSymbol() : nullptr;
+  const MCSymbol *RHS_A = RHS.getAddSym();
+  const MCSymbol *RHS_B = RHS.getSubSym();
+  int64_t RHS_Cst = RHS.getConstant();
 
   // Fold the result constant immediately.
   int64_t Result_Cst = LHS_Cst + RHS_Cst;
 
   // If we have a layout, we can fold resolved differences.
-  if (Asm && !LHS.getSpecifier() && !RhsSpec) {
+  if (Asm && !LHS.getSpecifier() && !RHS.getSpecifier()) {
     // 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).
@@ -472,16 +471,12 @@ bool MCExpr::evaluateSymbolicAdd(const MCAssembler *Asm,
 
   // At this point, we have at most one additive symbol and one subtractive
   // symbol -- find them.
-  auto *A = LHS_A ? LHS.getSymA() : RHS_A ? RhsAdd : nullptr;
-  auto *B = LHS_B ? LHS.getSymB() : RHS_B ? RhsSub : nullptr;
-  if (B && B->getKind() != MCSymbolRefExpr::VK_None)
-    return false;
+  auto *A = LHS_A ? LHS_A : RHS_A;
+  auto *B = LHS_B ? LHS_B : RHS_B;
   auto Spec = LHS.getSpecifier();
   if (!Spec)
-    Spec = RhsSpec;
-  Res = MCValue::get(A, B, Result_Cst);
-  Res.Specifier = Spec;
-  Res.SymSpecifier = 0;
+    Spec = RHS.getSpecifier();
+  Res = MCValue::get(A, B, Result_Cst, Spec);
   return true;
 }
 
@@ -532,18 +527,16 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
                                                             InSet || IsMachO)) {
         if (Kind != MCSymbolRefExpr::VK_None) {
           if (Res.isAbsolute()) {
-            Res = MCValue::get(SRE, nullptr, 0);
+            Res = MCValue::get(&Sym, nullptr, 0, Kind);
             return true;
           }
           // If the reference has a variant kind, we can only handle expressions
           // which evaluate exactly to a single unadorned symbol. Attach the
           // original VariantKind to SymA of the result.
-          if (Res.getRefKind() != MCSymbolRefExpr::VK_None || !Res.getSymA() ||
-              Res.getSubSym() || Res.getConstant())
+          if (Res.getRefKind() != MCSymbolRefExpr::VK_None ||
+              !Res.getAddSym() || Res.getSubSym() || Res.getConstant())
             return false;
-          Res = MCValue::get(
-              MCSymbolRefExpr::create(Res.getAddSym(), Kind, Asm->getContext()),
-              Res.getSymB(), Res.getConstant(), Res.getRefKind());
+          Res.Specifier = Kind;
         }
         if (!IsMachO)
           return true;
@@ -565,7 +558,7 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
       }
     }
 
-    Res = MCValue::get(SRE, nullptr, 0);
+    Res = MCValue::get(&Sym, nullptr, 0, Kind);
     return true;
   }
 
@@ -587,7 +580,7 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
         return false;
 
       // The cast avoids undefined behavior if the constant is INT64_MIN.
-      Res = MCValue::get(Value.getSymB(), Value.getSymA(),
+      Res = MCValue::get(Value.getSubSym(), Value.getAddSym(),
                          -(uint64_t)Value.getConstant());
       break;
     case MCUnaryExpr::Not:
@@ -653,9 +646,11 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
           Res = RHSValue;
           return true;
         }
-        return evaluateSymbolicAdd(Asm, Addrs, InSet, LHSValue, RHSValue.SymA,
-                                   RHSValue.SymB, RHSValue.Cst,
-                                   RHSValue.SymSpecifier, Res);
+        if (LHSValue.SymB && LHSValue.Specifier)
+          return false;
+        if (RHSValue.SymB && RHSValue.Specifier)
+          return false;
+        return evaluateSymbolicAdd(Asm, Addrs, InSet, LHSValue, RHSValue, Res);
       }
     }
 

llvm/lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp

@@ -80,8 +80,6 @@ bool AVRMCExpr::evaluateAsRelocatableImpl(MCValue &Result,
     if (!Asm || !Asm->hasLayout())
       return false;
 
-    MCContext &Context = Asm->getContext();
-    const MCSymbolRefExpr *Sym = nullptr;
     auto Spec = AVRMCExpr::VK_None;
     if (Value.getSymSpecifier() != MCSymbolRefExpr::VK_None)
       return false;
@@ -90,8 +88,8 @@ bool AVRMCExpr::evaluateAsRelocatableImpl(MCValue &Result,
       Spec = AVRMCExpr::VK_PM;
 
     // TODO: don't attach specifier to MCSymbolRefExpr.
-    Sym = MCSymbolRefExpr::create(Value.getAddSym(), Spec, Context);
-    Result = MCValue::get(Sym, nullptr, Value.getConstant());
+    Result =
+        MCValue::get(Value.getAddSym(), nullptr, Value.getConstant(), Spec);
   }
 
   return true;

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp

@@ -236,7 +236,7 @@ bool LoongArchAsmBackend::shouldInsertFixupForCodeAlign(MCAssembler &Asm,
       MCSym = MCSymbolRefExpr::create(Sym, Ctx);
       getSecToAlignSym()[Sec] = MCSym;
     }
-    return MCValue::get(MCSym, nullptr,
+    return MCValue::get(&MCSym->getSymbol(), nullptr,
                         MaxBytesToEmit << 8 | Log2(AF.getAlignment()));
   };
 
@@ -497,11 +497,8 @@ bool LoongArchAsmBackend::handleAddSubRelocations(const MCAssembler &Asm,
   default:
     llvm_unreachable("unsupported fixup size");
   }
-  MCValue A = MCValue::get(
-      MCSymbolRefExpr::create(Target.getAddSym(), Asm.getContext()), nullptr,
-      Target.getConstant());
-  MCValue B = MCValue::get(
-      MCSymbolRefExpr::create(Target.getSubSym(), Asm.getContext()));
+  MCValue A = MCValue::get(Target.getAddSym(), nullptr, Target.getConstant());
+  MCValue B = MCValue::get(Target.getSubSym());
   auto FA = MCFixup::create(Fixup.getOffset(), nullptr, std::get<0>(FK));
   auto FB = MCFixup::create(Fixup.getOffset(), nullptr, std::get<1>(FK));
   auto &Assembler = const_cast<MCAssembler &>(Asm);

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

@@ -656,11 +656,8 @@ bool RISCVAsmBackend::handleAddSubRelocations(const MCAssembler &Asm,
   default:
     llvm_unreachable("unsupported fixup size");
   }
-  MCValue A = MCValue::get(
-      MCSymbolRefExpr::create(Target.getAddSym(), Asm.getContext()), nullptr,
-      Target.getConstant());
-  MCValue B = MCValue::get(
-      MCSymbolRefExpr::create(Target.getSubSym(), Asm.getContext()));
+  MCValue A = MCValue::get(Target.getAddSym(), nullptr, Target.getConstant());
+  MCValue B = MCValue::get(Target.getSubSym());
   auto FA = MCFixup::create(
       Fixup.getOffset(), nullptr,
       static_cast<MCFixupKind>(FirstLiteralRelocationKind + TA));

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

@@ -175,7 +175,7 @@ _g9:
 // ERROBJ: :[[#@LINE+1]]:7: error: Cannot represent a difference across sections
 .quad _g9@AUTH(ia,42) - _g8
 
-// ERROBJ: :[[#@LINE+1]]:7: error: Cannot represent a difference across sections
+// ERROBJ: :[[#@LINE+1]]:7: error: expected relocatable expression
 .quad _g9@AUTH(ia,42) - _g8@AUTH(ia,42)
 .quad 0