[ELF] - Fix missing relocation when linking executable with --unresolved-symbols=ignore-all

[grimar]

Summary

This is PR32112. Previously when we linked executable with
--unresolved-symbols=ignore-all and undefined symbols, like:

_start:
callq und@PLT

we did not create relocations, though it looks in that case
we should delegate handling of such symbols to runtime linker,
hence should emit them. Patch fixes that.

[grimar]

• Removed too agressive change about --noinhibit-exec behavior.

[ruiu]

Looks like what you described in the commit message is different from what this patch actually does. This patch is to handle the --noinhibit_exec option which wasn't handled before.

[grimar]

In D35724#817642, @ruiu wrote:

Looks like what you described in the commit message is different from what this patch actually does.

Logic supporting what is desctibed in commit message was implemented in includeInDynsym(),
but I had to change how we handle --noinhibit_exec because few out testcase started to fail.

This patch is to handle the --noinhibit_exec option which wasn't handled before.

Actually we already handled it, but in a local way, it affected undefined symbols policy:

static UnresolvedPolicy getUnresolvedSymbolPolicy(opt::InputArgList &Args) {
  // -noinhibit-exec or -r imply some default values.
  if (Args.hasArg(OPT_noinhibit_exec))
    return UnresolvedPolicy::WarnAll;

I'll try to split --noinhibit exec change to a different patch.

[grimar]

Splitted --noinhibit_exec change to D35793.

[ruiu]

Please remove the code for -noinhibit-exec from this patch.

[grimar]

• Removed --noinhibit-exec relative code. It was splitted to D35793.

Notes: without D35793 this fails 2 testcases currently.

[ruiu]

ELF/Symbols.cpp

152 ↗

(On Diff #108026)

Why do you have to move this code?

365–373 ↗

(On Diff #108026)

Does this order matter?

[grimar]

• Addressed review comments.

ELF/Symbols.cpp

152 ↗

(On Diff #108026)

Previously for !Config->Shared we always returned false and it was early exit,
but if we want to handle undefined symbols case we need to move it and we can not exit
early anymore.

For example we should not return that undefined symbol is preemtible if it is hidden
(in that case includeInDynsym() should filter it out, that is required for hidden-vis-shared.s).

There are many other tests with other cases which would reasonable fail if I do not move this piece.

365–373 ↗

(On Diff #108026)

A kind of. What I tried to do here is preserve original logic, but add code that is equal to:

if (!Config->Shared && canBeExternal() && !body()->symbol()->isWeak())
  return true;

I just separated conditions and tried to exit early when possible. I reduced it by 2 lines in fresh diff.

[grimar]

• Addressed review comments.
• Added testcase with protected symbol. In that case we fail to link it with --unresolved-symbols=ignore-all. That behavior is natural and matches both bfd and gold. See no reasons to change it atm.

[ruiu]

LGTM with this comment.

ELF/Symbols.cpp

152–154 ↗

(On Diff #108331)

Undefined symbols in non-DSOs are usually just an error, so it usually doesn't matter whether we return true or false here. However, if -unresolved-symbols=ignore-all is specified, undefined symbols in executables are automatically exported so that the runtime linker can try to resolve them. In that case, they is preemptible. So, we return true for an undefined symbol in case the option is specified.

[grimar]

Was reverted in rL309258 because broke bots:

• http://green.lab.llvm.org/green//job/clang-stage2-configure-Rlto_check/17762/consoleFull#-1417328700a1ca8a51-895e-46c6-af87-ce24fa4cd561
• http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/3081/steps/check-cfi-and-supported%20in%20gcc%20build/logs/stdio
• http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/3948

[grimar]

• Updated to support testcase from r309329 (weak-undef-export.s)

[ruiu]

test/ELF/no-inhibit-exec.s

12–13 ↗

(On Diff #108628)

This doesn't seem to make much sense. Did you forgot to add -NEXT?

[grimar]

• Addressed comment: fixed testcase.

test/ELF/no-inhibit-exec.s

12–13 ↗

(On Diff #108628)

Thats true, thanks !

[grimar]

• Rebased, addressed comments.

lld/trunk/ELF/Symbols.cpp

if (isLocal())
     return false;
 
   // Shared symbols resolve to the definition in the DSO. The exceptions are
   // symbols with copy relocations (which resolve to .bss) or preempt plt
   // entries (which resolve to that plt entry).
   if (isShared())
     return !NeedsCopy && !NeedsPltAddr;
 
-  // That's all that can be preempted in a non-DSO.
-  if (!Config->Shared)
-    return false;
-
   // Only symbols that appear in dynsym can be preempted.
   if (!symbol()->includeInDynsym())
     return false;
 
   // Only default visibility symbols can be preempted.
   if (symbol()->Visibility != STV_DEFAULT)
     return false;
 
+  // Undefined symbols in non-DSOs are usually just an error, so it
+  // doesn't matter whether we return true or false here. However, if
+  // -unresolved-symbols=ignore-all is specified, undefined symbols in
+  // executables are automatically exported so that the runtime linker
+  // can try to resolve them. In that case, they is preemptible. So, we
+  // return true for an undefined symbol in case the option is specified.
+  if (!Config->Shared)
+    return isUndefined();
+
   // -Bsymbolic means that definitions are not preempted.
   if (Config->Bsymbolic || (Config->BsymbolicFunctions && isFunc()))
     return !isDefined();
   return true;
 }
 
 // Overwrites all attributes with Other's so that this symbol becomes
 // an alias to Other. This is useful for handling some options such as
if (Config->NoGnuUnique && Binding == STB_GNU_UNIQUE)
     return STB_GLOBAL;
   return Binding;
 }
 
 bool Symbol::includeInDynsym() const {
   if (computeBinding() == STB_LOCAL)
     return false;
   if (body()->isUndefined())
-    return Config->Shared;
+    return Config->Shared || !body()->symbol()->isWeak();
   return ExportDynamic || body()->isShared();
 }
 
 // Print out a log message for --trace-symbol.
 void elf::printTraceSymbol(Symbol *Sym) {
   SymbolBody *B = Sym->body();
   std::string S;
   if (B->isUndefined())

lld/trunk/test/ELF/executable-undefined-ignoreall.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: ld.lld %t -o %tout --unresolved-symbols=ignore-all -pie
+# RUN: llvm-readobj -r %tout | FileCheck %s
+
+# CHECK:      Relocations [
+# CHECK-NEXT:   Section ({{.*}}) .rela.plt {
+# CHECK-NEXT:     0x2018 R_X86_64_JUMP_SLOT foo 0x0
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+_start:
+callq foo@PLT

lld/trunk/test/ELF/executable-undefined-protected-ignoreall.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: not ld.lld %t -o %tout --unresolved-symbols=ignore-all -pie 2>&1 | FileCheck %s
+# CHECK: error: undefined symbol: foo
+
+.protected foo
+_start:
+callq foo@PLT

lld/trunk/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 -2101253
+# CHECK-NEXT: 201000: {{.*}} callq 0
+
+# RELOC:      Relocations [
+# RELOC-NEXT: ]
 
 # next code will not link without noinhibit-exec flag
 # because of undefined symbol _bar
 .globl _start
 _start:
   call _bar