[ELF] Error on relocations to local undefined symbols

[MaskRay]

Summary

For a reference to a local symbol, ld.bfd and gold error if the symbol
is defined in a discarded section but accept it if the symbol is
undefined. This inconsistent behavior seems unnecessary for us (it
probably makes sense for them as they differentiate local/global
symbols, the error would mean more code).

Weaken the condition to getSymbol(Config->IsMips64EL) == 0 to catch such
errors. The symbol index can be 0 (e.g. R_*_NONE) and we shouldn't error on them.

[MaskRay]

Update description

This patch works on our internal huge code base.

[grimar]

test/ELF/amdgpu-relocs.s

121

Do you know what was the point to check '.rodata'/'nonalloc' content initially?

[grimar]

ELF/Relocations.cpp

676

I am also not sure it is a good idea to template this function.
Perhaps would be better to check this condition outside/or to pass the getSymbol value?

[MaskRay]

Delete template parameter RelTy

[MaskRay]

Move the amdgpu test change to D61594

[MaskRay]

This + D61583 will give an error on the case provided in PR41693, but the error message isn't so good, so we also need D59649 :)

test/ELF/amdgpu-relocs.s

121

I've moved the amdgpu-relocs.s change to D61594

[ruiu]

I may be missing something, but is an undefined local symbol a real thing? It can never be resolved, and thus no one would produce such symbol, no?

[MaskRay]

I think compilers don't produce local undefined symbol. However, users can write such invalid assembly files (see our amdgpu-relocs.s test)

Also, the "undefined" semantic can be brought up by a symbol defined in a discarded section (COMDAT or SHF_EXCLUDE). The latest report can be found at https://bugs.llvm.org/show_bug.cgi?id=41693 Such (relocations to STT_SECTION symbols whose sections are discarded) errors occur occasionally.

[grimar]

I also do not know if undefined local symbol can be a real thing.
So my comment is about implementation.

ELF/Relocations.cpp

1032

Should we just never report error for R_*_NONE maybe? I.e use Type == Target->NoneRel here?
(should look a bit better than comparing symbol index it seems).

Do we have a test for that btw?

[MaskRay]

In D61563#1493055, @grimar wrote:

I also do not know if undefined local symbol can be a real thing.
So my comment is about implementation.

See my previous comment. It is for the error when a symbol (defined in discarded section) is used in a relocation expression.

If I remove SymIndex != 0 the following will break.

arm-v4bx.test
icf10.test
icf11.test
mips-abs-got.s
relocation-dtrace.test
relocation-none-aarch64.test
relocation-none-i686.test

The relocation-dtrace.test looks quite reasonable. R_ARM_NONE (and R_X86_64_NONE etc) can be used to create references among sections to prevent garbage collection (https://lwn.net/Articles/741494/)
mips-abs-got.s .reloc 0, R_MIPS_GOT_PAGE, 0 seems a reasonable relocation type other than R_*_NONE (hope @atanasyan can confirm)

So I have to keep SymIndex != 0 (I did try to delete the check all together before I realized some are legitimate)

[MaskRay]

Ping :)

[grimar]

ELF/Relocations.cpp

1040

I tried to reorder/simplify the code slightly to avoid symbol index check:

const uint8_t *RelocatedAddr = Sec.data().begin() + Rel.r_offset;
RelExpr Expr = Target->getRelExpr(Type, Sym, RelocatedAddr);

// Ignore "hint" relocations because they are only markers for relaxation.
if (oneof<R_HINT, R_NONE>(Expr))
  return;

// Skip if the target symbol is an erroneous undefined symbol.
if (maybeReportUndefined(Sym, Sec, Rel.r_offset))
  return;

With that the only test case that fails is mips-abs-got.s.
I am not sure if we can do something with it, I am not a MIPS expert.

[MaskRay]

ELF/Relocations.cpp

1040

@atanasyan Can R_MIPS_GOT_PAGE be (reasonably) used with symbol index 0? If it is theoretically possible but unlikely emitted by compiles/humans in reality, mips-abs-got.s should probably be updated.

[atanasyan]

I do not remember a reason of adding the .reloc 0, R_MIPS_GOT_PAGE, 0 statement to the test. I think now we can change mips-abs-got.s like that:

--- a/lld/test/ELF/mips-abs-got.s
+++ b/lld/test/ELF/mips-abs-got.s
@@ -4,7 +4,7 @@
 
 # RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux -o %t.o %s
 # RUN: echo "SECTIONS { \
-# RUN:          zero = 0; foo = 0x11004; bar = 0x22000; }" > %t.script
+# RUN:          zero1 = 0; zero2 = 0; foo = 0x11004; bar = 0x22000; }" > %t.script
 # RUN: ld.lld --script %t.script -o %t.exe %t.o
 # RUN: llvm-readobj --mips-plt-got %t.exe | FileCheck %s
 
@@ -30,7 +30,7 @@
 
   .text
   nop
-  .reloc 0, R_MIPS_GOT_PAGE, 0
-  ld      $v0, %got_page(zero)($gp)
+  ld      $v0, %got_page(zero1)($gp)
+  ld      $v0, %got_page(zero2)($gp)
   ld      $v0, %got_page(foo)($gp)
   ld      $v0, %got_page(bar+0x10008)($gp)

[ruiu]

LGTM

[MaskRay]

R_ARM_V4BX uses symbol index 0. Clarify why we check SymIndex != 0 instead of Type != Target->NoneRel

[grimar]

ELF/Relocations.cpp

1033

I.e. this is only for ARM? Should we use a helper there then maybe, like:

(pseudocode)

static bool isMarkerSym(Type) {
 if (!Arm)
  return Type == Target->NoneRel;
return SymIndex != 0;
}

ELF/Relocations.cpp

static int64_t computeAddend(const RelTy &Rel, const RelTy *End,
 
   return Addend;
 }
 
 // Report an undefined symbol if necessary.
 // Returns true if this function printed out an error message.
 static bool maybeReportUndefined(Symbol &Sym, InputSectionBase &Sec,
                                  uint64_t Offset) {
-  if (Sym.isLocal() || !Sym.isUndefined() || Sym.isWeak())
+  if (!Sym.isUndefined() || Sym.isWeak())
     return false;
 
   bool CanBeExternal =
       Sym.computeBinding() != STB_LOCAL && Sym.Visibility == STV_DEFAULT;
   if (Config->UnresolvedSymbols == UnresolvedPolicy::Ignore && CanBeExternal)
     return false;
 
   std::string Msg = "undefined ";

 };
 
 static std::vector<IRelativeReloc> IRelativeRelocs;
 
 template <class ELFT, class RelTy>
 static void scanReloc(InputSectionBase &Sec, OffsetGetter &GetOffset, RelTy *&I,
                       RelTy *End) {
   const RelTy &Rel = *I;
-  Symbol &Sym = Sec.getFile<ELFT>()->getRelocTargetSym(Rel);
+  uint32_t SymIndex = Rel.getSymbol(Config->IsMips64EL);
+  Symbol &Sym = Sec.getFile<ELFT>()->getSymbol(SymIndex);
   RelType Type;
 
   // Deal with MIPS oddity.
   if (Config->MipsN32Abi) {
     Type = getMipsN32RelType(I, End);
   } else {
     Type = Rel.getType(Config->IsMips64EL);
     ++I;
   }
 
   // Get an offset in an output section this relocation is applied to.
   uint64_t Offset = GetOffset.get(Rel.r_offset);
   if (Offset == uint64_t(-1))
     return;
 
-  // Skip if the target symbol is an erroneous undefined symbol.
-  if (maybeReportUndefined(Sym, Sec, Rel.r_offset))
+  // Skip if the target symbol is an erroneous undefined symbol. SymIndex 0 may
+  // be used by R_*_NONE. Don't error on them.
+  if (SymIndex != 0 && maybeReportUndefined(Sym, Sec, Rel.r_offset))
     return;
 
   const uint8_t *RelocatedAddr = Sec.data().begin() + Rel.r_offset;
   RelExpr Expr = Target->getRelExpr(Type, Sym, RelocatedAddr);
 
   // Ignore "hint" relocations because they are only markers for relaxation.
   if (oneof<R_HINT, R_NONE>(Expr))
     return;
 
   // We can separate the small code model relocations into 2 categories:
   // 1) Those that access the compiler generated .toc sections.
   // 2) Those that access the linker allocated got entries.
   // lld allocates got entries to symbols on demand. Since we don't try to sort
   // the got entries in any way, we don't have to track which objects have
   // got-based small code model relocs. The .toc sections get placed after the
   // end of the linker allocated .got section and we do sort those so sections

test/ELF/amdgpu-relocs.s

 # REQUIRES: amdgpu
 # RUN: llvm-mc -filetype=obj -triple=amdgcn--amdhsa -mcpu=fiji %s -o %t.o
 # RUN: ld.lld --hash-style=sysv -shared %t.o -o %t.so
 # RUN: llvm-readobj -r %t.so | FileCheck %s
-# RUN: llvm-objdump -s %t.so | FileCheck %s --check-prefix=OBJDUMP
+# RUN: llvm-readelf -x .rodata -x nonalloc %t.so | FileCheck %s --check-prefix=HEX
 
 .text
 
 kernel0:
   s_mov_b32 s0, common_var0@GOTPCREL+4
   s_mov_b32 s0, common_var1@gotpcrel32@lo+4
   s_mov_b32 s0, common_var2@gotpcrel32@hi+4
 
kernel0:
   .weak    weak_var1
   .weak    weak_var2
   .weakref weakref_var0, weakref_alias_var0
   .weakref weakref_var1, weakref_alias_var1
   .weakref weakref_var2, weakref_alias_var2
   .local   local_var0
   .local   local_var1
   .local   local_var2
+  local_var0:
+  local_var1:
+  local_var2:
 
 # R_AMDGPU_ABS32:
 .section nonalloc, "w", @progbits
   .long var0, common_var2+4
   .long var1, common_var1+8
   .long var2, common_var0+12
 
 # R_AMDGPU_ABS64:
 .type ptr, @object
 .data
   .globl ptr
   .p2align 3
 ptr:
   .quad temp
   .size ptr, 8
 
 # R_AMDGPU_RELATIVE64:
   .type temp2, @object
   .local temp2
   .size temp2, 4
+  temp2:
 
   .type ptr2, @object
   .globl ptr2
   .size ptr2, 8
   .p2align 3
 ptr2:
   .quad temp2
 
 # R_AMDGPU_REL64:
 .type foo, @object
 .rodata
   .globl foo
   .p2align 3
 foo:
   .quad temp2@rel64
   .size foo, 8
 
 # The relocation for local_var{0, 1, 2} and var should be resolved by the
 # linker.
 # CHECK: Relocations [
 # CHECK: .rela.dyn {
-# CHECK-NEXT: R_AMDGPU_RELATIVE64 - 0x0
+# CHECK-NEXT: R_AMDGPU_RELATIVE64 - 0x3008
 # CHECK-NEXT: R_AMDGPU_ABS64 common_var0 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 common_var1 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 common_var2 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 extern_var0 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 extern_var1 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 extern_var2 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 global_var0 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 global_var1 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 global_var2 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 temp 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 weak_var0 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 weak_var1 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 weak_var2 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 weakref_alias_var0 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 weakref_alias_var1 0x0
 # CHECK-NEXT: R_AMDGPU_ABS64 weakref_alias_var2 0x0
 # CHECK-NEXT: }
 # CHECK-NEXT: ]
 
-# OBJDUMP: Contents of section .rodata:
-# OBJDUMP: 98f8ffff ffffffff
+# HEX:      section '.rodata':
+# HEX-NEXT: 0x00000768 a0280000 00000000
 
-# OBJDUMP: Contents of section nonalloc:
-# OBJDUMP-NEXT: 0000 00000000 14380000 00000000 18340000
-# OBJDUMP-NEXT: 00000000 1c300000
+# HEX:      section 'nonalloc':
+# HEX-NEXT: 0x00000000 00000000 14380000 00000000 18340000
+# HEX-NEXT: 0x00000010 00000000 1c300000

test/ELF/local-undefined-symbol.s

 # REQUIRES: x86
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
-# RUN: ld.lld %t -o %t1
-# RUN: llvm-readobj --symbols %t1 | FileCheck %s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: not ld.lld %t.o -o %t 2>&1 | FileCheck %s
 
-# CHECK:     Symbols [
-# CHECK-NOT:  Name: foo
+# CHECK: error: undefined symbol: foo
 
 .global _start
 _start:
  jmp foo
 
 .local foo