[llvm] [NFC] Clean up MCSymbolRefExpr::VariantKind (PR #179363)
Harald van Dijk via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 3 18:54:40 PST 2026
https://github.com/hvdijk updated https://github.com/llvm/llvm-project/pull/179363
>From ae84d8c4e0df1edd57af65d896a0feac518f58c5 Mon Sep 17 00:00:00 2001
From: Harald van Dijk <hdijk at accesssoftek.com>
Date: Wed, 4 Mar 2026 02:53:10 +0000
Subject: [PATCH] [NFC] Clean up MCSpecifierExpr::VariantKind
VK_COFF_IMGREL32 previously had the value 3 because the AArch64 backend
uses bit masking to obtain meaning from its target specifiers, but it
checked this without first making sure it had a target specifier. The
value 3 happened to result in the correct behaviour so this went
unnoticed. Now, the utility functions to perform the masking operations
assert that the specifier is a target specifier, and this no longer
requires VK_COFF_IMGREL32 to have any particular value.
ARM and X86 previously defined S_COFF_SECREL as 1, to match VK_SECREL.
Commit b9ca4c5f removed VK_SECREL and made S_COFF_SECREL
target-specific, but did not update its value to indicate that it was
target-specific. After the renumbering of VK_COFF_IMGREL32, this would
result in a duplicate value.
Fixes #144832.
---
llvm/include/llvm/MC/MCExpr.h | 2 +-
.../MCTargetDesc/AArch64AsmBackend.cpp | 150 +++++++++---------
.../MCTargetDesc/AArch64ELFObjectWriter.cpp | 8 +-
.../AArch64/MCTargetDesc/AArch64MCAsmInfo.h | 7 +-
.../AArch64WinCOFFObjectWriter.cpp | 24 +--
.../Target/ARM/MCTargetDesc/ARMMCAsmInfo.h | 2 +-
.../Target/X86/MCTargetDesc/X86MCAsmInfo.h | 2 +-
llvm/test/MC/ELF/mc-dump.s | 8 +-
8 files changed, 110 insertions(+), 93 deletions(-)
diff --git a/llvm/include/llvm/MC/MCExpr.h b/llvm/include/llvm/MC/MCExpr.h
index d040fa337accf..1ba407681e61a 100644
--- a/llvm/include/llvm/MC/MCExpr.h
+++ b/llvm/include/llvm/MC/MCExpr.h
@@ -195,7 +195,7 @@ class MCSymbolRefExpr : public MCExpr {
// expressions with @). MCSpecifierExpr, as used by AArch64 and RISC-V, offers
// a cleaner approach.
enum VariantKind : uint16_t {
- VK_COFF_IMGREL32 = 3, // symbol at imgrel (image-relative)
+ VK_COFF_IMGREL32 = 1, // symbol at imgrel (image-relative)
FirstTargetSpecifier,
};
diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
index 1f9694cf98fec..ab1b8a6fc3068 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
@@ -212,24 +212,25 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, const MCValue &Target,
case AArch64::fixup_aarch64_movw: {
AArch64::Specifier RefKind =
static_cast<AArch64::Specifier>(Target.getSpecifier());
- if (AArch64::getSymbolLoc(RefKind) != AArch64::S_ABS &&
- AArch64::getSymbolLoc(RefKind) != AArch64::S_SABS) {
- if (!RefKind) {
- // The fixup is an expression
- if (SignedValue > 0xFFFF || SignedValue < -0xFFFF)
- Ctx.reportError(Fixup.getLoc(),
- "fixup value out of range [-0xFFFF, 0xFFFF]");
-
- // Invert the negative immediate because it will feed into a MOVN.
- if (SignedValue < 0)
- SignedValue = ~SignedValue;
- Value = static_cast<uint64_t>(SignedValue);
- } else
- // VK_GOTTPREL, VK_TPREL, VK_DTPREL are movw fixups, but they can't
- // ever be resolved in the assembler.
+ if (!RefKind) {
+ // The fixup is an expression
+ if (SignedValue > 0xFFFF || SignedValue < -0xFFFF)
Ctx.reportError(Fixup.getLoc(),
- "relocation for a thread-local variable points to an "
- "absolute symbol");
+ "fixup value out of range [-0xFFFF, 0xFFFF]");
+
+ // Invert the negative immediate because it will feed into a MOVN.
+ if (SignedValue < 0)
+ SignedValue = ~SignedValue;
+ Value = static_cast<uint64_t>(SignedValue);
+ } else if (RefKind == MCSymbolRefExpr::VK_COFF_IMGREL32) {
+ return Value;
+ } else if (AArch64::getSymbolLoc(RefKind) != AArch64::S_ABS &&
+ AArch64::getSymbolLoc(RefKind) != AArch64::S_SABS) {
+ // VK_GOTTPREL, VK_TPREL, VK_DTPREL are movw fixups, but they can't
+ // ever be resolved in the assembler.
+ Ctx.reportError(Fixup.getLoc(),
+ "relocation for a thread-local variable points to an "
+ "absolute symbol");
return Value;
}
@@ -241,53 +242,55 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, const MCValue &Target,
return Value;
}
- if (AArch64::getSymbolLoc(RefKind) == AArch64::S_SABS) {
- switch (AArch64::getAddressFrag(RefKind)) {
- case AArch64::S_G0:
- break;
- case AArch64::S_G1:
- SignedValue = SignedValue >> 16;
- break;
- case AArch64::S_G2:
- SignedValue = SignedValue >> 32;
- break;
- case AArch64::S_G3:
- SignedValue = SignedValue >> 48;
- break;
- default:
- llvm_unreachable("Variant kind doesn't correspond to fixup");
- }
+ if (RefKind) {
+ if (AArch64::getSymbolLoc(RefKind) == AArch64::S_SABS) {
+ switch (AArch64::getAddressFrag(RefKind)) {
+ case AArch64::S_G0:
+ break;
+ case AArch64::S_G1:
+ SignedValue = SignedValue >> 16;
+ break;
+ case AArch64::S_G2:
+ SignedValue = SignedValue >> 32;
+ break;
+ case AArch64::S_G3:
+ SignedValue = SignedValue >> 48;
+ break;
+ default:
+ llvm_unreachable("Variant kind doesn't correspond to fixup");
+ }
- } else {
- switch (AArch64::getAddressFrag(RefKind)) {
- case AArch64::S_G0:
- break;
- case AArch64::S_G1:
- Value = Value >> 16;
- break;
- case AArch64::S_G2:
- Value = Value >> 32;
- break;
- case AArch64::S_G3:
- Value = Value >> 48;
- break;
- default:
- llvm_unreachable("Variant kind doesn't correspond to fixup");
+ } else {
+ switch (AArch64::getAddressFrag(RefKind)) {
+ case AArch64::S_G0:
+ break;
+ case AArch64::S_G1:
+ Value = Value >> 16;
+ break;
+ case AArch64::S_G2:
+ Value = Value >> 32;
+ break;
+ case AArch64::S_G3:
+ Value = Value >> 48;
+ break;
+ default:
+ llvm_unreachable("Variant kind doesn't correspond to fixup");
+ }
}
- }
- if (RefKind & AArch64::S_NC) {
- Value &= 0xFFFF;
- } else if (AArch64::getSymbolLoc(RefKind) == AArch64::S_SABS) {
- if (SignedValue > 0xFFFF || SignedValue < -0xFFFF)
- Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
+ if (AArch64::isNotChecked(RefKind)) {
+ Value &= 0xFFFF;
+ } else if (AArch64::getSymbolLoc(RefKind) == AArch64::S_SABS) {
+ if (SignedValue > 0xFFFF || SignedValue < -0xFFFF)
+ Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
- // Invert the negative immediate because it will feed into a MOVN.
- if (SignedValue < 0)
- SignedValue = ~SignedValue;
- Value = static_cast<uint64_t>(SignedValue);
- } else if (Value > 0xFFFF) {
- Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
+ // Invert the negative immediate because it will feed into a MOVN.
+ if (SignedValue < 0)
+ SignedValue = ~SignedValue;
+ Value = static_cast<uint64_t>(SignedValue);
+ } else if (Value > 0xFFFF) {
+ Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
+ }
}
return Value;
}
@@ -431,18 +434,20 @@ void AArch64AsmBackend::applyFixup(const MCFragment &F, const MCFixup &Fixup,
if (Fixup.getKind() == FK_Data_8 && TheTriple.isOSBinFormatELF()) {
auto RefKind = static_cast<AArch64::Specifier>(Target.getSpecifier());
- AArch64::Specifier SymLoc = AArch64::getSymbolLoc(RefKind);
- if (SymLoc == AArch64::S_AUTH || SymLoc == AArch64::S_AUTHADDR) {
- const auto *Expr = dyn_cast<AArch64AuthMCExpr>(Fixup.getValue());
- if (!Expr) {
- getContext().reportError(Fixup.getValue()->getLoc(),
- "expected relocatable expression");
- return;
+ if (RefKind >= MCSymbolRefExpr::FirstTargetSpecifier) {
+ AArch64::Specifier SymLoc = AArch64::getSymbolLoc(RefKind);
+ if (SymLoc == AArch64::S_AUTH || SymLoc == AArch64::S_AUTHADDR) {
+ const auto *Expr = dyn_cast<AArch64AuthMCExpr>(Fixup.getValue());
+ if (!Expr) {
+ getContext().reportError(Fixup.getValue()->getLoc(),
+ "expected relocatable expression");
+ return;
+ }
+ assert(Value == 0);
+ Value = (uint64_t(Expr->getDiscriminator()) << 32) |
+ (uint64_t(Expr->getKey()) << 60) |
+ (uint64_t(Expr->hasAddressDiversity()) << 63);
}
- assert(Value == 0);
- Value = (uint64_t(Expr->getDiscriminator()) << 32) |
- (uint64_t(Expr->getKey()) << 60) |
- (uint64_t(Expr->hasAddressDiversity()) << 63);
}
}
@@ -486,7 +491,8 @@ void AArch64AsmBackend::applyFixup(const MCFragment &F, const MCFixup &Fixup,
// handle this more cleanly. This may affect the output of -show-mc-encoding.
AArch64::Specifier RefKind =
static_cast<AArch64::Specifier>(Target.getSpecifier());
- if (AArch64::getSymbolLoc(RefKind) == AArch64::S_SABS ||
+ if ((RefKind >= MCSymbolRefExpr::FirstTargetSpecifier &&
+ AArch64::getSymbolLoc(RefKind) == AArch64::S_SABS) ||
(!RefKind && Fixup.getKind() == AArch64::fixup_aarch64_movw)) {
// If the immediate is negative, generate MOVN else MOVZ.
// (Bit 30 = 0) ==> MOVN, (Bit 30 = 1) ==> MOVZ.
diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
index e33196d4c0144..8d77dc0d7770b 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
@@ -88,8 +88,12 @@ unsigned AArch64ELFObjectWriter::getRelocType(const MCFixup &Fixup,
auto Kind = Fixup.getKind();
AArch64::Specifier RefKind =
static_cast<AArch64::Specifier>(Target.getSpecifier());
- AArch64::Specifier SymLoc = AArch64::getSymbolLoc(RefKind);
- bool IsNC = AArch64::isNotChecked(RefKind);
+ AArch64::Specifier SymLoc = RefKind & AArch64::S_SymLocBits;
+ bool IsNC = RefKind & AArch64::S_NC;
+ if (RefKind >= MCSymbolRefExpr::FirstTargetSpecifier) {
+ SymLoc = AArch64::getSymbolLoc(RefKind);
+ IsNC = AArch64::isNotChecked(RefKind);
+ }
switch (SymLoc) {
case AArch64::S_DTPREL:
diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.h b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.h
index 5bb8ff8599b2a..19b4cbc60f441 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.h
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.h
@@ -187,14 +187,19 @@ StringRef getSpecifierName(Specifier S);
Specifier parsePercentSpecifierName(StringRef);
inline Specifier getSymbolLoc(Specifier S) {
+ assert(S >= MCSymbolRefExpr::FirstTargetSpecifier);
return static_cast<Specifier>(S & AArch64::S_SymLocBits);
}
inline Specifier getAddressFrag(Specifier S) {
+ assert(S >= MCSymbolRefExpr::FirstTargetSpecifier);
return static_cast<Specifier>(S & AArch64::S_AddressFragBits);
}
-inline bool isNotChecked(Specifier S) { return S & AArch64::S_NC; }
+inline bool isNotChecked(Specifier S) {
+ assert(S >= MCSymbolRefExpr::FirstTargetSpecifier);
+ return S & AArch64::S_NC;
+}
} // namespace AArch64
class AArch64AuthMCExpr final : public MCSpecifierExpr {
diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp
index 5fe999389ce79..b4e4920870f1c 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp
@@ -67,17 +67,19 @@ unsigned AArch64WinCOFFObjectWriter::getRelocType(
if (auto *A64E = dyn_cast<MCSpecifierExpr>(Expr)) {
AArch64::Specifier Spec = A64E->getSpecifier();
- switch (AArch64::getSymbolLoc(Spec)) {
- case AArch64::S_ABS:
- case AArch64::S_SECREL:
- // Supported
- break;
- default:
- Ctx.reportError(Fixup.getLoc(),
- "relocation specifier " +
- AArch64::getSpecifierName(A64E->getSpecifier()) +
- " unsupported on COFF targets");
- return COFF::IMAGE_REL_ARM64_ABSOLUTE; // Dummy return value
+ if (Spec >= MCSymbolRefExpr::FirstTargetSpecifier) {
+ switch (AArch64::getSymbolLoc(Spec)) {
+ case AArch64::S_ABS:
+ case AArch64::S_SECREL:
+ // Supported
+ break;
+ default:
+ Ctx.reportError(Fixup.getLoc(),
+ "relocation specifier " +
+ AArch64::getSpecifierName(A64E->getSpecifier()) +
+ " unsupported on COFF targets");
+ return COFF::IMAGE_REL_ARM64_ABSOLUTE; // Dummy return value
+ }
}
}
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.h b/llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.h
index 8b24e7b5d8015..edd3e5748408a 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.h
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.h
@@ -92,7 +92,6 @@ namespace ARM {
using Specifier = uint16_t;
enum {
S_None,
- S_COFF_SECREL,
S_HI16 =
MCSymbolRefExpr::FirstTargetSpecifier, // The R_ARM_MOVT_ABS relocation
@@ -109,6 +108,7 @@ enum {
// .s file)
S_ARM_NONE,
+ S_COFF_SECREL,
S_FUNCDESC,
S_GOT,
S_GOTFUNCDESC,
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCAsmInfo.h b/llvm/lib/Target/X86/MCTargetDesc/X86MCAsmInfo.h
index 9533bfa31cb1f..1da8cdee40caa 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86MCAsmInfo.h
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86MCAsmInfo.h
@@ -69,9 +69,9 @@ using Specifier = uint16_t;
enum {
S_None,
- S_COFF_SECREL,
S_ABS8 = MCSymbolRefExpr::FirstTargetSpecifier,
+ S_COFF_SECREL,
S_DTPOFF,
S_DTPREL,
S_GOT,
diff --git a/llvm/test/MC/ELF/mc-dump.s b/llvm/test/MC/ELF/mc-dump.s
index 51b3ff4774cf3..370f0c1d84432 100644
--- a/llvm/test/MC/ELF/mc-dump.s
+++ b/llvm/test/MC/ELF/mc-dump.s
@@ -16,8 +16,8 @@
# CHECK-NEXT:3 Relaxable Size:0+2 [eb,00] <MCInst #[[#]] <MCOperand Expr:.Ltmp0>>
# CHECK-NEXT: Fixup @1 Value:.Ltmp0 Kind:4001
# CHECK-NEXT:5 Data Size:16 [48,8b,04,25,00,00,00,00,48,8b,04,25,00,00,00,00]
-# CHECK-NEXT: Fixup @4 Value:f0@<variant 11> Kind:4017
-# CHECK-NEXT: Fixup @12 Value:_start@<variant 11> Kind:4017
+# CHECK-NEXT: Fixup @4 Value:f0@<variant 10> Kind:4017
+# CHECK-NEXT: Fixup @12 Value:_start@<variant 10> Kind:4017
# CHECK-NEXT: Symbol @16 .Ltmp0 Temporary
# CHECK-NEXT: Symbol @0 Temporary
# CHECK-NEXT: Symbol @16 Temporary
@@ -36,8 +36,8 @@
# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t -debug-only=mc-dump -save-temp-labels -g 2>&1 | FileCheck %s --check-prefix=CHECK2
# CHECK2:5 Data Size:16 [48,8b,04,25,00,00,00,00,48,8b,04,25,00,00,00,00]
-# CHECK2-NEXT: Fixup @4 Value:f0@<variant 11> Kind:4017
-# CHECK2-NEXT: Fixup @12 Value:_start@<variant 11> Kind:4017
+# CHECK2-NEXT: Fixup @4 Value:f0@<variant 10> Kind:4017
+# CHECK2-NEXT: Fixup @12 Value:_start@<variant 10> Kind:4017
# CHECK2-NEXT: Symbol @16 .Ltmp2
# CHECK2-NEXT: Symbol @0 .Lcfi0 Temporary
# CHECK2:MCSection Name:.eh_frame
More information about the llvm-commits
mailing list