[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

 
 // For --discard-{all,locals,none}.
 enum class DiscardPolicy { Default, All, Locals, None };
 
 // For --strip-{all,debug}.
 enum class StripPolicy { None, All, Debug };
 
 // For --unresolved-symbols.
-enum class UnresolvedPolicy { ReportError, Warn, WarnAll, Ignore, IgnoreAll };
+enum class UnresolvedPolicy { ReportError, Warn, Ignore, IgnoreAll };
 
 // For --sort-section and linkerscript sorting rules.
 enum class SortSectionPolicy { Default, None, Alignment, Name, Priority };
 
 // For --target2
 enum class Target2Policy { Abs, Rel, GotRel };
 
 struct SymbolVersion {
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

 static std::string getRpath(opt::InputArgList &Args) {
   std::vector<StringRef> V = getArgs(Args, OPT_rpath);
   return llvm::join(V.begin(), V.end(), ":");
 }
 
 // Determines what we should do if there are remaining unresolved
 // symbols after the name resolution.
 static UnresolvedPolicy getUnresolvedSymbolPolicy(opt::InputArgList &Args) {
-  // -noinhibit-exec or -r imply some default values.
-  if (Args.hasArg(OPT_noinhibit_exec))
-    return UnresolvedPolicy::WarnAll;
   if (Args.hasArg(OPT_relocatable))
     return UnresolvedPolicy::IgnoreAll;
 
   UnresolvedPolicy ErrorOrWarn = getArg(Args, OPT_error_unresolved_symbols,
                                         OPT_warn_unresolved_symbols, true)
                                      ? UnresolvedPolicy::ReportError
                                      : UnresolvedPolicy::Warn;
 
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/Error.cpp

void elf::warn(const Twine &Msg) {
 
   std::lock_guard<std::mutex> Lock(Mu);
   newline(Msg);
   print("warning: ", raw_ostream::MAGENTA);
   *ErrorOS << Msg << "\n";
 }
 
 void elf::error(const Twine &Msg) {
+  if (Config->NoinhibitExec) {
+    warn(Msg);
+    return;
+  }
+
   std::lock_guard<std::mutex> Lock(Mu);
   newline(Msg);
 
   if (Config->ErrorLimit == 0 || ErrorCount < Config->ErrorLimit) {
     print("error: ", raw_ostream::RED);
     *ErrorOS << Msg << "\n";
   } else if (ErrorCount == Config->ErrorLimit) {
     print("error: ", raw_ostream::RED);

ELF/Relocations.cpp

 }
 
 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())
     Msg += Src + "\n>>>               ";
   Msg += S.getObjMsg<ELFT>(Offset);
 
-  if (Config->UnresolvedSymbols == UnresolvedPolicy::WarnAll ||
-      (Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal)) {
+  if (Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal)
     warn(Msg);
-  } else {
+  else
     error(Msg);
-  }
-}
 
 template <class RelTy>
 static std::pair<uint32_t, uint32_t>
 mergeMipsN32RelTypes(uint32_t Type, uint32_t Offset, RelTy *I, RelTy *E) {
   // MIPS N32 ABI treats series of successive relocations with the same offset
   // as a single relocation. The similar approach used by N64 ABI, but this ABI
   // packs all relocations into the single relocation record. Here we emulate
   // this for the N32 ABI. Iterate over relocation with the same offset and put

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