MCExpr::evaluateAsRelocatableImpl : allow evaluation of non-VK_None MCSymbolRefExpr when MCAsmLayout is available

[MaskRay]

Summary

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4acf8c78e659833be8be047ba2f8561386a11d4b
(1994) introduced this behavior:
if a fixup symbol is equated to an expression with an undefined symbol, convert
the fixup to be against the target symbol. glibc relies on this behavior to perform
assembly level indirection

asm("memcpy = __GI_memcpy"); // from sysdeps/generic/symbol-hacks.h

...
  // call memcpy@PLT
  // The relocation references __GI_memcpy in GNU as, but memcpy in MC (without the patch)
  memcpy (...);

(1) It complements extern __typeof(memcpy) memcpy asm("__GI_memcpy"); The frontend asm label does not redirect synthesized memcpy in the middle-end. (See D88712 for details)
(2) asm("memcpy = __GI_memcpy"); is in every translation unit, but the memcpy declaration may not be visible in the translation unit where memcpy is synthesized.

MC already redirects memcpy = __GI_memcpy; call memcpy but not memcpy = __GI_memcpy; call memcpy@plt.
This patch fixes the latter by allowing MCExpr::evaluateAsRelocatableImpl to
evaluate a non-VK_None MCSymbolRefExpr, which is only done after the layout is available.

GNU as allows memcpy = __GI_memcpy+1; call memcpy@PLT which seems nonsensical, so we don't allow it.

MC/PowerPC/pr38945.s NUMBER = 0x6ffffff9; cmpwi 8,NUMBER@l requires the
symbol@l form in AsmMatcher, so evaluation needs to be deferred. This is the
place whether future simplification may be possible.

Note, if we suppress the VM_None evaluation when MCAsmLayout is nullptr, we may
lose the invalid reassignment of non-absolute variable diagnostic
(ARM/thumb_set-diagnostics.s and MC/AsmParser/variables-invalid.s).
We know that this diagnostic is troublesome in some cases
(https://github.com/ClangBuiltLinux/linux/issues/1008), so we can consider
making simplification in the future.

[MaskRay]

I have tried adding the expression folding stuff to`MCExpr::evaluateAsRelocatableImpl` but I abandoned that idea because evaluateAsRelocatableImpl does not like a modifier different from VK_None (@PLT). (What does a@plt + 1 do? Relaxing the condition (== VK_None) may work... but the semantics are not very clear.

[MaskRay]

Fix clang-tidy issues

[MaskRay]

🙄

[echristo]

Oh, you're waiting on me. Uh, I'll get to this tomorrow. :)

[jyknight]

I have tried adding the expression folding stuff to`MCExpr::evaluateAsRelocatableImpl` but I abandoned that idea because evaluateAsRelocatableImpl does not like a modifier different from VK_None (@PLT). (What does a@plt + 1 do? Relaxing the condition (== VK_None) may work... but the semantics are not very clear.

a@plt + 1 is perfectly fine -- that turns into a R_X86_64_PLT32 relocation with symbol "a" and addend 1.

The interesting question is what this should do:

b=a+1 # no @plt!
... b@plt

It's certainly impossible to create a PLT referring to a function at address "a+1", in current object formats. So, probably this should be invalid. However, GAS seems to consider the modifier as more of an overall-modifier, resulting in the same thing as a@plt + 1. I agree that's probably not something we ought to support, since it seems wrong.

But, it does still seem reasonable to put the !VK_None support into evaluateAsRelocatableImpl -- as long as the resolved MCValue has only a single symbol, no offset and no kind.

[MaskRay]

In D88625#2328335, @jyknight wrote:

I have tried adding the expression folding stuff to`MCExpr::evaluateAsRelocatableImpl` but I abandoned that idea because evaluateAsRelocatableImpl does not like a modifier different from VK_None (@PLT). (What does a@plt + 1 do? Relaxing the condition (== VK_None) may work... but the semantics are not very clear.

a@plt + 1 is perfectly fine -- that turns into a R_X86_64_PLT32 relocation with symbol "a" and addend 1.

The interesting question is what this should do:

b=a+1 # no @plt!
... b@plt

It's certainly impossible to create a PLT referring to a function at address "a+1", in current object formats. So, probably this should be invalid. However, GAS seems to consider the modifier as more of an overall-modifier, resulting in the same thing as a@plt + 1. I agree that's probably not something we ought to support, since it seems wrong.

But, it does still seem reasonable to put the !VK_None support into evaluateAsRelocatableImpl -- as long as the resolved MCValue has only a single symbol, no offset and no kind.

The current way the patch implements the "if a fixup symbol is equated to an expression with an undefined symbol" logic is consistent with the step GNU as implements the check.
GNU as implements the logic when it is about to emit a relocation, not when folding expressions.

One major problem by moving the condition to MCExpr::evaluateAsRelocatableImpl is that we may do isUndefined() check too early (this special case does not kick in when the symbol is defined (memcpy = __GI_memcpy; call memcpy ... __GI_memcpy: ... should emit a relocation referencing memcpy, instead of __GI_memcpy)).
The second problem is indeed the cooperation with the VK_None special rule on MCExpr.cpp:805. Another minor problem is that this will add another rule to the existing if (!IsMachO) hack on 810.

[MaskRay]

😋

[MaskRay]

@jyknight Ping😹

This is not so important as D88712 when used for glibc, but still it'd be good to have this patch:)

I know that not supporting MCBinaryExpr may make you feel a bit nervous: binary expr on an undefined symbol is very rarely used.

[jyknight]

In D88625#2328351, @MaskRay wrote:

In D88625#2328335, @jyknight wrote:

I have tried adding the expression folding stuff to`MCExpr::evaluateAsRelocatableImpl` but I abandoned that idea because evaluateAsRelocatableImpl does not like a modifier different from VK_None (@PLT). (What does a@plt + 1 do? Relaxing the condition (== VK_None) may work... but the semantics are not very clear.

a@plt + 1 is perfectly fine -- that turns into a R_X86_64_PLT32 relocation with symbol "a" and addend 1.

The interesting question is what this should do:

b=a+1 # no @plt!
... b@plt

It's certainly impossible to create a PLT referring to a function at address "a+1", in current object formats. So, probably this should be invalid. However, GAS seems to consider the modifier as more of an overall-modifier, resulting in the same thing as a@plt + 1. I agree that's probably not something we ought to support, since it seems wrong.

But, it does still seem reasonable to put the !VK_None support into evaluateAsRelocatableImpl -- as long as the resolved MCValue has only a single symbol, no offset and no kind.

The current way the patch implements the "if a fixup symbol is equated to an expression with an undefined symbol" logic is consistent with the step GNU as implements the check.
GNU as implements the logic when it is about to emit a relocation, not when folding expressions.

All of this is pretty confusing. We do need to expand variable references, including those to undefined symbols, during expression evaluation in certain contexts, such as .if. We have some code in there to do this now (conditioning expansion based on "InSet"), but e.g.

x = a - 1
.if x + 1 == a
nop
.endif

doesn't work with llvm, today, while it does in gas.

OTOH, for something like the following, we definitely need to preserve the reference to b, and NOT try to expand it to a+1 too early (because we need to not emit the PLT relocation to global symbol a). Binutils' as compiles this as a PC32 relocation off of .text.a, while we generate a PLT32 relocation off of b; both of those seem probably fine.

b = a+1

.section .text.foo
foo:
jmp b@PLT


.section .text.a
.globl a
a:
nop
nop

One major problem by moving the condition to MCExpr::evaluateAsRelocatableImpl is that we may do isUndefined() check too early (this special case does not kick in when the symbol is defined (memcpy = __GI_memcpy; call memcpy ... __GI_memcpy: ... should emit a relocation referencing memcpy, instead of __GI_memcpy)).

I think we can ensure that we only do it at the right point, by checking whether we have the optional Layout object passed in. evaluateFixup calls evaluateAsRelocatable with a Layout object, but other uses that occur pre-layout will not.

[MaskRay]

Change MCExpr::evaluateAsRelocatableImpl instead. This works for MCBinaryExpr automatically

[MaskRay]

Changed to patch MCExpr::evaluateAsRelocatableImpl instead

In D88625#2369263, @jyknight wrote:

In D88625#2328351, @MaskRay wrote:

In D88625#2328335, @jyknight wrote:

I have tried adding the expression folding stuff to`MCExpr::evaluateAsRelocatableImpl` but I abandoned that idea because evaluateAsRelocatableImpl does not like a modifier different from VK_None (@PLT). (What does a@plt + 1 do? Relaxing the condition (== VK_None) may work... but the semantics are not very clear.

a@plt + 1 is perfectly fine -- that turns into a R_X86_64_PLT32 relocation with symbol "a" and addend 1.

The interesting question is what this should do:

b=a+1 # no @plt!
... b@plt

It's certainly impossible to create a PLT referring to a function at address "a+1", in current object formats. So, probably this should be invalid. However, GAS seems to consider the modifier as more of an overall-modifier, resulting in the same thing as a@plt + 1. I agree that's probably not something we ought to support, since it seems wrong.

But, it does still seem reasonable to put the !VK_None support into evaluateAsRelocatableImpl -- as long as the resolved MCValue has only a single symbol, no offset and no kind.

The current way the patch implements the "if a fixup symbol is equated to an expression with an undefined symbol" logic is consistent with the step GNU as implements the check.
GNU as implements the logic when it is about to emit a relocation, not when folding expressions.

All of this is pretty confusing. We do need to expand variable references, including those to undefined symbols, during expression evaluation in certain contexts, such as .if. We have some code in there to do this now (conditioning expansion based on "InSet"), but e.g.

x = a - 1
.if x + 1 == a
nop
.endif

doesn't work with llvm, today, while it does in gas.

OTOH, for something like the following, we definitely need to preserve the reference to b, and NOT try to expand it to a+1 too early (because we need to not emit the PLT relocation to global symbol a). Binutils' as compiles this as a PC32 relocation off of .text.a, while we generate a PLT32 relocation off of b; both of those seem probably fine.

b = a+1

.section .text.foo
foo:
jmp b@PLT


.section .text.a
.globl a
a:
nop
nop

b is local (a symbol assignment does not copy the binding), so GNU as emits a relocation referencing the section symbol.
This patch does not change the behavior.

One major problem by moving the condition to MCExpr::evaluateAsRelocatableImpl is that we may do isUndefined() check too early (this special case does not kick in when the symbol is defined (memcpy = __GI_memcpy; call memcpy ... __GI_memcpy: ... should emit a relocation referencing memcpy, instead of __GI_memcpy)).

I think we can ensure that we only do it at the right point, by checking whether we have the optional Layout object passed in. evaluateFixup calls evaluateAsRelocatable with a Layout object, but other uses that occur pre-layout will not.

It seems that the parse-time MCExpr::evaluateAsRelocatableImpl only takes actions if the value is absolute (not isInSection()),,,,, so MCExpr::evaluateAsRelocatableImpl actually works.
The whole infrastructure does give me an impression that it can have subtle behaviors in a few places....

[MaskRay]

@jyknight Ping:)

[jyknight]

llvm/lib/MC/MCExpr.cpp

811 ↗

(On Diff #302429)

Maybe to prevent the nonsensical result in the test below, have a condition, like:

// If the reference has a variant kind, we can only handle expressions
// which evaluate exactly to a single unadorned symbol.
if (Kind != MCSymbolRefExpr::VK_None) {
  if (Res.getRefKind() != MCSymbolRefExpr::VK_None || !Res.getSymA() || Res.getSymB() || Res.getConstant())
    return false;
  Res = ...
}

llvm/test/MC/ARM/ehabi-personality-abs.s

12 ↗

(On Diff #302429)

", because the target is an absolute symbol."

llvm/test/MC/ELF/relocation-alias.s

19

This is compatible with what GNU as does, but is nonsensical behavior. Do you think we really want to support it?

[MaskRay]

Comments

[MaskRay]

llvm/lib/MC/MCExpr.cpp

816 ↗

(On Diff #304993)

I cannot test Res.getSymB in the conditions because (1) foo - bar can result into an early error (2) foo + foo will cause a report_fatal_error crash.

[jyknight]

llvm/lib/MC/MCExpr.cpp

816 ↗

(On Diff #304993)

Are you saying that the code, as it is now, has this problem? (If so, I don't understand what you mean).

But, looking at the test failure shown for lld/test/ELF/x86-64-relax-got-abs.s shows that "return false" was wrong here -- we shouldn't be _failing_ to evaluate, rather, we should be returning the symbol ref expr as-is. (Res = MCValue::get(SRE, nullptr, 0); return true; -- the fallthrough case below.

[MaskRay]

Special case the Res.isAbsolute() case

[MaskRay]

llvm/lib/MC/MCExpr.cpp

816 ↗

(On Diff #304993)

I mean I cannot construct a test case to exercise the "Res.getSymB()" condition.

Thanks for spotting the lld/test/ELF/x86-64-relax-got-abs.s issue. I'll special case Res.isAbsolute and add a test for abs@gotpcrelx... abs = 42

[MaskRay]

Forgot to resolve a comment.

Does it look good now:)

[jyknight]

AFAICT, this looks good now. I guess we'll see if anything breaks after it's submitted. =)

[MaskRay]

Thanks for taking time to perform the thorough reviews:)

I have done two Linux kernel builds, make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- O=/tmp/out/arm64 LLVM=1 LLVM_IAS=1 defconfig vmlinux and make O=/tmp/out/x86_64 LLVM=1 LLVM_IAS=1 defconfig vmlinux -> passed.

llvm/lib/MC/MCAssembler.cpp

bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout,
   }
   if (const MCSymbolRefExpr *RefB = Target.getSymB()) {
     if (RefB->getKind() != MCSymbolRefExpr::VK_None) {
       Ctx.reportError(Fixup.getLoc(),
                       "unsupported subtraction of qualified symbol");
       return true;
     }
   }
+  if (const MCSymbolRefExpr *RefA = Target.getSymA()) {
+    // If SymA is equated to an expression with an undefined symbol, GNU as
+    // converts the fixup to be against the target symbol. We only support a
+    // simple form `alias = aliasee` (MCSymbolRefExpr).
+    auto &SymA = RefA->getSymbol();
+    if (SymA.isVariable()) {
+      if (auto *SRE = dyn_cast<MCSymbolRefExpr>(SymA.getVariableValue()))
+        if (SRE->getSymbol().isUndefined() &&
+            SRE->getKind() != MCSymbolRefExpr::VK_WEAKREF) {
+          Target = MCValue::get(
+              MCSymbolRefExpr::create(&SRE->getSymbol(), RefA->getKind(), Ctx),
+              Target.getSymB(), Target.getConstant(), Target.getRefKind());
+        }
+    }
+  }
 
   assert(getBackendPtr() && "Expected assembler backend");
   bool IsTarget = getBackendPtr()->getFixupKindInfo(Fixup.getKind()).Flags &
                   MCFixupKindInfo::FKF_IsTarget;
 
   if (IsTarget)
     return getBackend().evaluateTargetFixup(*this, Layout, Fixup, DF, Target,
                                             Value, WasForced);

llvm/test/MC/ELF/relocation-alias.s

+# RUN: llvm-mc -filetype=obj -triple x86_64 %s -o %t
+# RUN: llvm-objdump -dr %t | FileCheck %s
+# RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYM
+
+## If a fixup symbol is equated to an undefined symbol, convert the fixup
+## to be against the target symbol.
+# CHECK:      callq {{.*}}
+# CHECK-NEXT:   R_X86_64_PLT32  __GI_memcpy-0x4
+# CHECK:      movabsq $0, %rax
+# CHECK-NEXT:   R_X86_64_64  __GI_memcpy+0x2
+memcpy = __GI_memcpy
+call memcpy@PLT
+movabsq $memcpy+2, %rax
+
+## NOTE: GNU as redirects memcpy_plus_1.
+# CHECK:      callq {{.*}}
+# CHECK-NEXT:   R_X86_64_PLT32  memcpy_plus_1
+memcpy_plus_1 = __GI_memcpy+1
+call memcpy_plus_1@PLT
+
+# CHECK:      movabsq $0, %rbx
+# CHECK-NEXT:   R_X86_64_64  data_alias
+.globl data_alias
+.set data_alias, data
+movabsq $data_alias, %rbx
+
+.data
+.byte 0
+.globl data
+data:
+
+## Redirected symbols do not have a symbol table entry.
+# SYM:      NOTYPE LOCAL  DEFAULT UND
+# SYM-NEXT: NOTYPE GLOBAL DEFAULT UND __GI_memcpy
+# SYM-NEXT: NOTYPE GLOBAL DEFAULT   4 data
+# SYM-NEXT: NOTYPE GLOBAL DEFAULT   4 data_alias
+# SYM-NEXT: NOTYPE GLOBAL DEFAULT UND memcpy_plus_1
+# SYM-NOT:  {{.}}