[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

Why do you have to move this code?

365–374

Does this order matter?

[grimar]

• Addressed review comments.

ELF/Symbols.cpp

152

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

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

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

11–12

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

11–12

Thats true, thanks !

[grimar]

• Rebased, addressed comments.

ELF/Config.h

struct Configuration {
   bool ExportDynamic;
   bool FatalWarnings;
   bool GcSections;
   bool GdbIndex;
   bool GnuHash;
   bool ICF;
   bool MipsN32Abi = false;
   bool NoGnuUnique;
+  bool NoinhibitExec;
   bool NoUndefinedVersion;
   bool Nostdlib;
   bool OFormatBinary;
   bool Omagic;
   bool OptRemarksWithHotness;
   bool Pie;
   bool PrintGcSections;
   bool Relocatable;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->ICF = Args.hasArg(OPT_icf);
   Config->Init = Args.getLastArgValue(OPT_init, "_init");
   Config->LTOAAPipeline = Args.getLastArgValue(OPT_lto_aa_pipeline);
   Config->LTONewPmPasses = Args.getLastArgValue(OPT_lto_newpm_passes);
   Config->LTOO = getInteger(Args, OPT_lto_O, 2);
   Config->LTOPartitions = getInteger(Args, OPT_lto_partitions, 1);
   Config->MapFile = Args.getLastArgValue(OPT_Map);
   Config->NoGnuUnique = Args.hasArg(OPT_no_gnu_unique);
+  Config->NoinhibitExec = Args.hasArg(OPT_noinhibit_exec);
   Config->NoUndefinedVersion = Args.hasArg(OPT_no_undefined_version);
   Config->Nostdlib = Args.hasArg(OPT_nostdlib);
   Config->OFormatBinary = isOutputFormatBinary(Args);
   Config->Omagic = Args.hasArg(OPT_omagic);
   Config->OptRemarksFilename = Args.getLastArgValue(OPT_opt_remarks_filename);
   Config->OptRemarksWithHotness = Args.hasArg(OPT_opt_remarks_with_hotness);
   Config->Optimize = getInteger(Args, OPT_O, 1);
   Config->OutputFile = Args.getLastArgValue(OPT_o);

ELF/Relocations.cpp

for (SharedSymbol *Sym : getSymbolsAt<ELFT>(SS)) {
     Sym->CopyRelSec = Sec;
     Sym->CopyRelSecOff = Off;
     Sym->symbol()->IsUsedInRegularObj = true;
   }
 
   In<ELFT>::RelaDyn->addReloc({Target->CopyRel, Sec, Off, false, SS, 0});
 }
 
+static void errorOrWarn(const Twine &Msg) {
+  if (!Config->NoinhibitExec)
+    error(Msg);
+  else
+    warn(Msg);
+}
+
 template <class ELFT>
 static RelExpr adjustExpr(SymbolBody &Body, RelExpr Expr, uint32_t Type,
                           const uint8_t *Data, InputSectionBase &S,
                           typename ELFT::uint RelOff) {
   if (Body.isGnuIFunc()) {
     Expr = toPlt(Expr);
   } else if (!isPreemptible(Body, Type)) {
     if (needsPlt(Expr))
if (Body.isFunc()) {
     // pointer equality is maintained. To avoid an infinite loop, the only entry
     // that points to the real function is a dedicated got entry used by the
     // plt. That is identified by special relocation types (R_X86_64_JUMP_SLOT,
     // R_386_JMP_SLOT, etc).
     Body.NeedsPltAddr = true;
     return toPlt(Expr);
   }
 
-  error("symbol '" + toString(Body) + "' defined in " + toString(Body.File) +
-        " has no type");
+  errorOrWarn("symbol '" + toString(Body) + "' defined in " +
+              toString(Body.File) + " has no type");
   return Expr;
 }
 
 // Returns an addend of a given relocation. If it is RELA, an addend
 // is in a relocation itself. If it is REL, we need to read it from an
 // input section.
 template <class ELFT, class RelTy>
 static int64_t computeAddend(const RelTy &Rel, const uint8_t *Buf) {

 }
 
 template <class ELFT>
 static void reportUndefined(SymbolBody &Sym, InputSectionBase &S,
                             uint64_t Offset) {
   if (Config->UnresolvedSymbols == UnresolvedPolicy::IgnoreAll)
     return;
 
-  bool CanBeExternal = Sym.symbol()->computeBinding() != STB_LOCAL &&
-                       Sym.getVisibility() == STV_DEFAULT;
+  bool CanBeExternal = Sym.symbol()->canBeExternal();
   if (Config->UnresolvedSymbols == UnresolvedPolicy::Ignore && CanBeExternal)
     return;
 
   std::string Msg =
       "undefined symbol: " + toString(Sym) + "\n>>> referenced by ";
 
   std::string Src = S.getSrcMsg<ELFT>(Offset);
   if (!Src.empty())
if (needsGot(Expr)) {
         addGotEntry<ELFT>(Body, Preemptible);
       }
     }
 
     if (!needsPlt(Expr) && !needsGot(Expr) && isPreemptible(Body, Type)) {
       // We don't know anything about the finaly symbol. Just ask the dynamic
       // linker to handle the relocation for us.
       if (!Target->isPicRel(Type))
-        error("relocation " + toString(Type) +
-              " cannot be used against shared object; recompile with -fPIC" +
-              getLocation<ELFT>(Sec, Body, Offset));
+        errorOrWarn(
+            "relocation " + toString(Type) +
 
       In<ELFT>::RelaDyn->addReloc(
           {Target->getDynRel(Type), &Sec, Offset, false, &Body, Addend});
 
       // MIPS ABI turns using of GOT and dynamic relocations inside out.
       // While regular ABI uses dynamic relocations to fill up GOT entries
       // MIPS ABI requires dynamic linker to fills up GOT entries using
       // specially sorted dynamic symbol table. This affects even dynamic

ELF/Symbols.h

struct Symbol {
 
   // True if this symbol is specified by --trace-symbol option.
   unsigned Traced : 1;
 
   // This symbol version was found in a version script.
   unsigned InVersionScript : 1;
 
   bool includeInDynsym() const;
+  bool canBeExternal() const;
   uint8_t computeBinding() const;
   bool isWeak() const { return Binding == llvm::ELF::STB_WEAK; }
 
   // This field is used to store the Symbol's SymbolBody. This instantiation of
   // AlignedCharArrayUnion gives us a struct with a char array field that is
   // large and aligned enough to store any derived class of SymbolBody.
   llvm::AlignedCharArrayUnion<DefinedCommon, DefinedRegular, Undefined,
                               SharedSymbol, LazyArchive, LazyObject>

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 external symbols in a non-DSO usually reported by linker and link
+  // fails, but together with --unresolved-symbols=ignore-all link succeeds and
+  // runtime linker should take care about them during execution.
+  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 (Visibility != STV_DEFAULT && Visibility != STV_PROTECTED)
     return STB_LOCAL;
   if (VersionId == VER_NDX_LOCAL && body()->isInCurrentDSO())
     return STB_LOCAL;
   if (Config->NoGnuUnique && Binding == STB_GNU_UNIQUE)
     return STB_GLOBAL;
   return Binding;
 }
 
+bool Symbol::canBeExternal() const {
+  return computeBinding() != STB_LOCAL &&
+         body()->getVisibility() == STV_DEFAULT;
+}
+
 bool Symbol::includeInDynsym() const {
   if (computeBinding() == STB_LOCAL)
     return false;
-  return ExportDynamic || body()->isShared() ||
-         (body()->isUndefined() && Config->Shared);
+  if (ExportDynamic || body()->isShared())
+    return true;
+  if (!body()->isUndefined())
+    return false;
+  if (Config->Shared)
+    return true;
+  return canBeExternal() && !body()->symbol()->isWeak();
 }
 
 // Print out a log message for --trace-symbol.
 void elf::printTraceSymbol(Symbol *Sym) {
   SymbolBody *B = Sym->body();
   std::string S;
   if (B->isUndefined())
     S = ": reference to ";
   else if (B->isCommon())

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

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
 # REQUIRES: x86
 
 # CHECK: Disassembly of section .text:
 # CHECK-NEXT: _start
-# CHECK-NEXT: 201000: {{.*}} callq -2101253
+# CHECK-NEXT: 201000: {{.*}} callq 0
 
 # next code will not link without noinhibit-exec flag
 # because of undefined symbol _bar
 .globl _start
 _start:
   call _bar