Keep some relocations with undefined weak symbols

[• rafael]

Summary

This is pr34301.

As the bug points out, we want to keep some relocations with undefined weak symbols. This means that we cannot always claim that these symbols are not preemptible as we do now.

Unfortunately, we cannot also just always claim that they are preemptible. Doing so would, for example, cause us to try to create a plt entry when we don't even have a dynamic symbol table.

What almost works is to say that weak undefined symbols are preemptible if and only if we have a dynamic symbol table. Almost because we don't want to fail the build trying to create a copy relocation to a weak undefined.

[davide]

Thanks for fixing this.
I have no objections to the way you fixed it, some comments inline. Maybe Rui wants to take a look at the style.

ELF/Config.h

130

you seem to assign this unconditionally, so do you need to initialize this to false?

ELF/Driver.cpp

1012–1013

Maybe you can add a comment here? I remember the rule now, but I won't in a bit :)

ELF/Relocations.cpp

571–581

I see what you mean here, but, do we have a better way of saying "degrade it to zero?"
Something like, force its value to be zero? (Sorry, I don't have a better way of expressing it)

[ruiu]

ELF/Driver.cpp

1012–1013

Since this is computed based on other configurations, I'd move this to setConfigs because that's where other non-command-line parameters are set.

[• rafael]

Address review comments.

[davide]

LGTM

[ruiu]

Unfortunately it also depends on there being a .so in the link or not,
so it has to be set after the others.

SetConfigs is called after createFiles, and all .so files are added by createFiles, it is safe to initialize the variable in SetConfigs, no?

[ruiu]

LGTM

[davide]

r313372

ELF/Config.h

struct Configuration {
   bool EmitRelocs;
   bool EnableNewDtags;
   bool ExportDynamic;
   bool FatalWarnings;
   bool GcSections;
   bool GdbIndex;
   bool GnuHash;
   bool HasDynamicList = false;
+  bool HasDynSymTab;
   bool ICF;
   bool MipsN32Abi = false;
   bool NoGnuUnique;
   bool NoUndefinedVersion;
   bool NoinhibitExec;
   bool Nostdlib;
   bool OFormatBinary;
   bool Omagic;

ELF/Driver.cpp

template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   for (auto *Arg : Args.filtered(OPT_trace_symbol))
     Symtab->trace(Arg->getValue());
 
   // Add all files to the symbol table. This will add almost all
   // symbols that we need to the symbol table.
   for (InputFile *F : Files)
     Symtab->addFile<ELFT>(F);
 
+  // Now that we have every file, we can decide if we will need a
+  // dynamic symbol table.
+  // We need one if we were asked to export dynamic symbols or if we are
+  // producing a shared library.
+  // We also need one if any shared libraries are used and for pie executables
+  // (probably because the dynamic linker needs it).
+  Config->HasDynSymTab = !SharedFile<ELFT>::Instances.empty() || Config->Pic ||
+                         Config->ExportDynamic;
+
   // Some symbols (such as __ehdr_start) are defined lazily only when there
   // are undefined symbols for them, so we add these to trigger that logic.
   for (StringRef Sym : Script->Opt.ReferencedSymbols)
     Symtab->addUndefined<ELFT>(Sym);
 
   // If an entry symbol is in a static archive, pull out that file now
   // to complete the symbol table. After this, no new names except a
   // few linker-synthesized ones will be added to the symbol table.

ELF/Relocations.cpp

if (Body.isGnuIFunc()) {
     if (Expr == R_GOT_PC && !isAbsoluteValue(Body))
       Expr = Target->adjustRelaxExpr(Type, Data, Expr);
   }
 
   bool IsWrite = !Config->ZText || (S.Flags & SHF_WRITE);
   if (IsWrite || isStaticLinkTimeConstant<ELFT>(Expr, Type, Body, S, RelOff))
     return Expr;
 
-  // This relocation would require the dynamic linker to write a value to read
-  // only memory. We can hack around it if we are producing an executable and
+  // If we got here we know that this relocation would require the dynamic
+  // linker to write a value to read only memory.
+
+  // If the relocation is to a weak undef, give up on it and produce a
+  // non preemptible 0.
+  if (Body.isUndefWeak()) {
+    Body.IsPreemptible = false;
+    return Expr;
+  }
+
+  // We can hack around it if we are producing an executable and
   // the refered symbol can be preemepted to refer to the executable.
   if (Config->Shared || (Config->Pic && !isRelExpr(Expr))) {
     error("can't create dynamic relocation " + toString(Type) + " against " +
           (Body.getName().empty() ? "local symbol"
                                   : "symbol: " + toString(Body)) +
           " in readonly segment; recompile object files with -fPIC" +
           getLocation<ELFT>(S, Body, RelOff));
     return Expr;

ELF/Symbols.cpp

uint8_t Symbol::computeBinding() const {
   if (VersionId == VER_NDX_LOCAL && body()->isInCurrentDSO())
     return STB_LOCAL;
   if (Config->NoGnuUnique && Binding == STB_GNU_UNIQUE)
     return STB_GLOBAL;
   return Binding;
 }
 
 bool Symbol::includeInDynsym() const {
+  if (!Config->HasDynSymTab)
+    return false;
   if (computeBinding() == STB_LOCAL)
     return false;
-  if (body()->isUndefWeak())
-    return Config->Shared;
   if (!body()->isInCurrentDSO())
     return true;
   return ExportDynamic;
 }
 
 // Print out a log message for --trace-symbol.
 void elf::printTraceSymbol(Symbol *Sym) {
   SymbolBody *B = Sym->body();

ELF/Writer.cpp

for (InputSection *S : createCommonSections())
     Add(S);
 
   InX::Bss = make<BssSection>(".bss");
   Add(InX::Bss);
   InX::BssRelRo = make<BssSection>(".bss.rel.ro");
   Add(InX::BssRelRo);
 
   // Add MIPS-specific sections.
-  bool HasDynSymTab = !SharedFile<ELFT>::Instances.empty() || Config->Pic ||
-                      Config->ExportDynamic;
   if (Config->EMachine == EM_MIPS) {
-    if (!Config->Shared && HasDynSymTab) {
+    if (!Config->Shared && Config->HasDynSymTab) {
       InX::MipsRldMap = make<MipsRldMapSection>();
       Add(InX::MipsRldMap);
     }
     if (auto *Sec = MipsAbiFlagsSection<ELFT>::create())
       Add(Sec);
     if (auto *Sec = MipsOptionsSection<ELFT>::create())
       Add(Sec);
     if (auto *Sec = MipsReginfoSection<ELFT>::create())
       Add(Sec);
   }
 
-  if (HasDynSymTab) {
+  if (Config->HasDynSymTab) {
     InX::DynSymTab = make<SymbolTableSection<ELFT>>(*InX::DynStrTab);
     Add(InX::DynSymTab);
 
     In<ELFT>::VerSym = make<VersionTableSection<ELFT>>();
     Add(In<ELFT>::VerSym);
 
     if (!Config->VersionDefinitions.empty()) {
       In<ELFT>::VerDef = make<VersionDefinitionSection<ELFT>>();

test/ELF/no-inhibit-exec.s

 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 # RUN: not ld.lld %t -o %t2
 # RUN: ld.lld %t --noinhibit-exec -o %t2
 # RUN: llvm-objdump -d %t2 | FileCheck %s
 # RUN: llvm-readobj -r %t2 | FileCheck %s --check-prefix=RELOC
 # REQUIRES: x86
 
 # CHECK: Disassembly of section .text:
 # CHECK-NEXT: _start
-# CHECK-NEXT: 201000: {{.*}} callq 0
+# CHECK-NEXT: 201000: {{.*}} callq -2101253
 
 # RELOC:      Relocations [
 # RELOC-NEXT: ]
 
 # next code will not link without noinhibit-exec flag
 # because of undefined symbol _bar
 .globl _start
 _start:
   call _bar

test/ELF/pie-weak.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -relax-relocations=false -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: ld.lld -pie %t.o -o %t
 # RUN: llvm-readobj -r %t | FileCheck --check-prefix=RELOCS %s
 # RUN: llvm-objdump -d %t | FileCheck --check-prefix=DISASM %s
 
 # RELOCS:      Relocations [
+# RELOCS-NEXT:   Section ({{.*}}) .rela.dyn {
+# RELOCS-NEXT:     R_X86_64_GLOB_DAT foo 0x0
+# RELOCS-NEXT:   }
 # RELOCS-NEXT: ]
 
 .weak foo
 
 .globl _start
 _start:
 # DISASM: _start:
-# DISASM-NEXT: 1000: 48 8b 05 69 10 00 00 movq 4201(%rip), %rax
+# DISASM-NEXT: 1000: 48 8b 05 99 10 00 00 movq 4249(%rip), %rax
 #                                              ^ .got - (.text + 7)
 mov foo@gotpcrel(%rip), %rax

test/ELF/relocation-relative-weak.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: ld.lld %t.o -o %t -pie
 # RUN: llvm-readobj -dyn-relocations %t | FileCheck %s
 
 # CHECK:      Dynamic Relocations {
+# CHECK-NEXT:   0x2018 R_X86_64_JUMP_SLOT w 0x0
 # CHECK-NEXT: }
 
 .globl _start
 _start:
 
 .globl w
 .weak w
 call w@PLT

test/ELF/weak-undef-export.s

 # CHECK-NEXT:     Name: @ (0)
 # CHECK-NEXT:     Value: 0x0
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Local (0x0)
 # CHECK-NEXT:     Type: None (0x0)
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: Undefined (0x0)
 # CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: foo@ (1)
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Weak (0x2)
+# CHECK-NEXT:     Type: None (0x0)
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: Undefined (0x0)
+# CHECK-NEXT:  }
 # CHECK-NEXT: ]
 
         .weak foo
         .quad foo

test/ELF/weak-undef-val.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld %t.o -o %t --export-dynamic
+# RUN: llvm-readobj -s -section-data %t | FileCheck %s
+
+# CHECK:      Name: .text
+# CHECK-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT:   SHF_EXECINSTR
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address: 0x201000
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size:
+# CHECK-NEXT: Link:
+# CHECK-NEXT: Info:
+# CHECK-NEXT: AddressAlignment:
+# CHECK-NEXT: EntrySize:
+# CHECK-NEXT: SectionData (
+# CHECK-NEXT:   0000: 00F0DFFF                             |
+# CHECK-NEXT: )
+
+        .global _start
+_start:
+        .weak foobar
+        .long foobar - .

test/ELF/weak-undef.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: ld.lld %t.o -o %t -pie
 # RUN: llvm-readobj -dyn-symbols %t | FileCheck %s
 
 # CHECK:      DynamicSymbols [
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: @
 # CHECK-NEXT:     Value: 0x0
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Local (0x0)
 # CHECK-NEXT:     Type: None (0x0)
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: Undefined (0x0)
 # CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: foo@
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Weak
+# CHECK-NEXT:     Type: None
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: Undefined
+# CHECK-NEXT:   }
 # CHECK-NEXT: ]
 
 .weak foo
 
 .globl _start
 _start: