[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 ↗

(On Diff #198221)

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

[grimar]

ELF/Relocations.cpp

678

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 ↗

(On Diff #198221)

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

1034

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

1042

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

1042

@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

1035

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))
+  // Error if the target symbol is undefined. Symbol index 0 may be used by
+  // marker relocations, e.g. R_*_NONE and R_ARM_V4BX. 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/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