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

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–14

", 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

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

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

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/MCExpr.cpp

bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
 
   case Constant:
     Res = MCValue::get(cast<MCConstantExpr>(this)->getValue());
     return true;
 
   case SymbolRef: {
     const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(this);
     const MCSymbol &Sym = SRE->getSymbol();
+    const auto Kind = SRE->getKind();
 
     // Evaluate recursively if this is a variable.
-    if (Sym.isVariable() && SRE->getKind() == MCSymbolRefExpr::VK_None &&
+    if (Sym.isVariable() && (Kind == MCSymbolRefExpr::VK_None || Layout) &&
         canExpand(Sym, InSet)) {
       bool IsMachO = SRE->hasSubsectionsViaSymbols();
       if (Sym.getVariableValue()->evaluateAsRelocatableImpl(
               Res, Asm, Layout, Fixup, Addrs, InSet || IsMachO)) {
+        if (Kind != MCSymbolRefExpr::VK_None) {
+          // 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.getSymB() || Res.getConstant())
+            return false;
+          Res =
+              MCValue::get(MCSymbolRefExpr::create(&Res.getSymA()->getSymbol(),
+                                                   Kind, Asm->getContext()),
+                           Res.getSymB(), Res.getConstant(), Res.getRefKind());
+        }
         if (!IsMachO)
           return true;
 
         const MCSymbolRefExpr *A = Res.getSymA();
         const MCSymbolRefExpr *B = Res.getSymB();
         // FIXME: This is small hack. Given
         // a = b + 4
         // .long a

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

-@ RUN: llvm-mc -triple armv7-linux -filetype obj -o - %s | llvm-objdump --triple=armv7 -D -r - | FileCheck %s
+@ RUN: not llvm-mc -triple armv7-linux -filetype obj %s 2>&1 | FileCheck %s
 
   .text
 
 __aeabi_unwind_cpp_pr0 = 0xdeadbeef
 
 f:
   .fnstart
   bx lr
   .fnend
 
-@ CHECK: R_ARM_NONE __aeabi_unwind_cpp_pr0
-
+@@ Regression test: MC does not crash due to the absolute __aeabi_unwind_cpp_pr0.
+@@ Note, GNU as emits a R_ARM_NONE for this invalid usage.
+@ CHECK: <unknown>:0: error: expected relocatable expression

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
+
+# RUN: not llvm-mc -filetype=obj -triple x86_64 --defsym ERR=1 %s 2>&1 | FileCheck %s --check-prefix=ERR
+
+## If a fixup symbol is equated to an undefined symbol, convert the fixup
+## to be against the target symbol, even if there is a variant (@PLT).
+# 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
+
+# CHECK:      movabsq $0, %rbx
+# CHECK-NEXT:   R_X86_64_64  data_alias
+.globl data_alias
+.set data_alias, data
+movabsq $data_alias, %rbx
+
+## A local alias to a defined symbol still references a section symbol.
+# CHECK:      movabsq $0, %rbx
+# CHECK-NEXT:   R_X86_64_64  .data+0x1
+.set data_alias_l, data
+movabsq $data_alias_l, %rbx
+
+.data
+.byte 0
+.globl data
+data:
+
+.ifdef ERR
+.text
+## Note, GNU as emits a relocation for this erroneous fixup.
+# ERR: {{.*}}.s:[[#@LINE+2]]:1: error: expected relocatable expression
+memcpy_plus_1 = __GI_memcpy+1
+call memcpy_plus_1@PLT
+.endif
+
+## Redirected symbols do not have a symbol table entry.
+# SYM:      NOTYPE  LOCAL  DEFAULT UND
+# SYM-NEXT: NOTYPE  LOCAL  DEFAULT   4 data_alias_l
+# SYM-NEXT: SECTION LOCAL  DEFAULT   4 .data
+# SYM-NEXT: NOTYPE  GLOBAL DEFAULT UND __GI_memcpy
+# SYM-NEXT: NOTYPE  GLOBAL DEFAULT   4 data
+# SYM-NEXT: NOTYPE  GLOBAL DEFAULT   4 data_alias
+# SYM-NOT:  {{.}}

llvm/test/MC/ELF/relocation.s

.long bar                         # R_X86_64_32
         pushq    $bar
         movq  foo(%rip), %rdx
         leaq    foo-bar(%r14),%r14
         addq  $bar,%rax         # R_X86_64_32S
   .quad foo@DTPOFF
         movabsq $baz@TPOFF, %rax
   .word   foo-bar
   .byte   foo-bar
-
-        # this should probably be an error...
-  zed = foo +2
-  call zed@PLT
+  call foo
 
         leaq    -1+foo(%rip), %r11
 
         leaq  _GLOBAL_OFFSET_TABLE_(%rax), %r15
         leaq  _GLOBAL_OFFSET_TABLE_(%rip), %r15
         movl  $_GLOBAL_OFFSET_TABLE_, %eax
         movabs  $_GLOBAL_OFFSET_TABLE_, %rax
 

 // CHECK-NEXT:       0x62 R_X86_64_32S .text 0x0
 // CHECK-NEXT:       0x69 R_X86_64_PC32 foo 0xFFFFFFFFFFFFFFFC
 // CHECK-NEXT:       0x70 R_X86_64_PC32 foo 0x70
 // CHECK-NEXT:       0x77 R_X86_64_32S .text 0x0
 // CHECK-NEXT:       0x7B R_X86_64_DTPOFF64 foo 0x0
 // CHECK-NEXT:       0x85 R_X86_64_TPOFF64 baz 0x0
 // CHECK-NEXT:       0x8D R_X86_64_PC16 foo 0x8D
 // CHECK-NEXT:       0x8F R_X86_64_PC8 foo 0x8F
-// CHECK-NEXT:       0x91 R_X86_64_PLT32 zed 0xFFFFFFFFFFFFFFFC
+// CHECK-NEXT:       0x91 R_X86_64_PLT32 foo 0xFFFFFFFFFFFFFFFC
 // CHECK-NEXT:       0x98 R_X86_64_PC32 foo 0xFFFFFFFFFFFFFFFB
 // CHECK-NEXT:       0x9F R_X86_64_GOTPC32 _GLOBAL_OFFSET_TABLE_ 0x3
 // CHECK-NEXT:       0xA6 R_X86_64_GOTPC32 _GLOBAL_OFFSET_TABLE_ 0xFFFFFFFFFFFFFFFC
 // CHECK-NEXT:       0xAB R_X86_64_GOTPC32 _GLOBAL_OFFSET_TABLE_ 0x1
 // CHECK-NEXT:       0xB1 R_X86_64_GOTPC64 _GLOBAL_OFFSET_TABLE_ 0x2
 // CHECK-NEXT:       0xB9 R_X86_64_SIZE64 blah 0x0
 // CHECK-NEXT:       0xC1 R_X86_64_SIZE64 blah 0x20
 // CHECK-NEXT:       0xC9 R_X86_64_SIZE64 blah 0xFFFFFFFFFFFFFFE0