[ELF] Fix interaction between --unresolved-symbols= and --[no-]allow-shlib-undefined

[MaskRay]

Summary

As mentioned in https://reviews.llvm.org/D67479#1667256 ,

• --[no-]allow-shlib-undefined control the diagnostic for an unresolved symbol in a shared object
• -z defs/-z undefs control the diagnostic for an unresolved symbol in a regular object file
• --unresolved-symbols= controls both bits.

In addition, make --warn-unresolved-symbols affect --no-allow-shlib-undefined.

This patch makes the behavior match GNU ld.

[psmith]

LGTM, with one suggestion of a name change. I've checked the GNU ld behaviour against the green test changes and LLD matches.

lld/ELF/Driver.cpp

587

Perhaps call this setUnresolvedSymbolPolicy as it isn't returning anything and is modifying config as a side effect.

[MaskRay]

Rename to setUnresolvedSymbolPolicy

[MaskRay]

Fix test (%t6 does not exist)

lld/ELF/Config.h

struct Configuration {
   std::vector<llvm::StringRef> thinLTOModulesToCompile;
   std::vector<llvm::StringRef> undefined;
   std::vector<SymbolVersion> dynamicList;
   std::vector<uint8_t> buildIdVector;
   llvm::MapVector<std::pair<const InputSectionBase *, const InputSectionBase *>,
                   uint64_t>
       callGraphProfile;
   bool allowMultipleDefinition;
-  bool allowShlibUndefined;
   bool androidPackDynRelocs;
   bool armHasBlx = false;
   bool armHasMovtMovw = false;
   bool armJ1J2BranchEncoding = false;
   bool asNeeded = false;
   bool bsymbolic;
   bool bsymbolicFunctions;
   bool callGraphProfileSort;
struct Configuration {
   bool zWxneeded;
   DiscardPolicy discard;
   GnuStackKind zGnustack;
   ICFLevel icf;
   OrphanHandlingPolicy orphanHandling;
   SortSectionPolicy sortSection;
   StripPolicy strip;
   UnresolvedPolicy unresolvedSymbols;
+  UnresolvedPolicy unresolvedSymbolsInShlib;
   Target2Policy target2;
   ARMVFPArgKind armVFPArgs = ARMVFPArgKind::Default;
   BuildIdKind buildId = BuildIdKind::None;
   SeparateSegmentKind zSeparate;
   ELFKind ekind = ELFNoneKind;
   uint16_t emachine = llvm::ELF::EM_NONE;
   llvm::Optional<uint64_t> imageBase;
   uint64_t commonPageSize;

lld/ELF/Driver.cpp

 
 static std::string getRpath(opt::InputArgList &args) {
   std::vector<StringRef> v = args::getStrings(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) {
+static void setUnresolvedSymbolPolicy(opt::InputArgList &args) {
   UnresolvedPolicy errorOrWarn = args.hasFlag(OPT_error_unresolved_symbols,
                                               OPT_warn_unresolved_symbols, true)
                                      ? UnresolvedPolicy::ReportError
                                      : UnresolvedPolicy::Warn;
+  // -shared implies -unresolved-symbols=ignore-all because missing
+  // symbols are likely to be resolved at runtime.
+  bool diagRegular = !config->shared, diagShlib = !config->shared;
 
-  // Process the last of -unresolved-symbols, -no-undefined or -z defs.
-  for (auto *arg : llvm::reverse(args)) {
+  for (const opt::Arg *arg : args) {
     switch (arg->getOption().getID()) {
     case OPT_unresolved_symbols: {
       StringRef s = arg->getValue();
-      if (s == "ignore-all" || s == "ignore-in-object-files")
-        return UnresolvedPolicy::Ignore;
-      if (s == "ignore-in-shared-libs" || s == "report-all")
-        return errorOrWarn;
+      if (s == "ignore-all") {
+        diagRegular = false;
+        diagShlib = false;
+      } else if (s == "ignore-in-object-files") {
+        diagRegular = false;
+        diagShlib = true;
+      } else if (s == "ignore-in-shared-libs") {
+        diagRegular = true;
+        diagShlib = false;
+      } else if (s == "report-all") {
+        diagRegular = true;
+        diagShlib = true;
+      } else {
         error("unknown --unresolved-symbols value: " + s);
-      continue;
+      }
+      break;
     }
     case OPT_no_undefined:
-      return errorOrWarn;
+      diagRegular = true;
+      break;
     case OPT_z:
       if (StringRef(arg->getValue()) == "defs")
-        return errorOrWarn;
-      if (StringRef(arg->getValue()) == "undefs")
-        return UnresolvedPolicy::Ignore;
-      continue;
+        diagRegular = true;
+      else if (StringRef(arg->getValue()) == "undefs")
+        diagRegular = false;
+      break;
+    case OPT_allow_shlib_undefined:
+      diagShlib = false;
+      break;
+    case OPT_no_allow_shlib_undefined:
+      diagShlib = true;
+      break;
     }
   }
 
-  // -shared implies -unresolved-symbols=ignore-all because missing
-  // symbols are likely to be resolved at runtime using other DSOs.
-  if (config->shared)
-    return UnresolvedPolicy::Ignore;
-  return errorOrWarn;
+  config->unresolvedSymbols =
+      diagRegular ? errorOrWarn : UnresolvedPolicy::Ignore;
+  config->unresolvedSymbolsInShlib =
+      diagShlib ? errorOrWarn : UnresolvedPolicy::Ignore;
 }
 
 static Target2Policy getTarget2(opt::InputArgList &args) {
   StringRef s = args.getLastArgValue(OPT_target2, "got-rel");
   if (s == "rel")
     return Target2Policy::Rel;
   if (s == "abs")
     return Target2Policy::Abs;
errorHandler().fatalWarnings =
       args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false);
   errorHandler().vsDiagnostics =
       args.hasArg(OPT_visual_studio_diagnostics_format, false);
 
   config->allowMultipleDefinition =
       args.hasFlag(OPT_allow_multiple_definition,
                    OPT_no_allow_multiple_definition, false) ||
       hasZOption(args, "muldefs");
-  config->allowShlibUndefined =
-      args.hasFlag(OPT_allow_shlib_undefined, OPT_no_allow_shlib_undefined,
-                   args.hasArg(OPT_shared));
   config->auxiliaryList = args::getStrings(args, OPT_auxiliary);
   config->bsymbolic = args.hasArg(OPT_Bsymbolic);
   config->bsymbolicFunctions = args.hasArg(OPT_Bsymbolic_functions);
   config->checkSections =
       args.hasFlag(OPT_check_sections, OPT_no_check_sections, true);
   config->chroot = args.getLastArgValue(OPT_chroot);
   config->compressDebugSections = getCompressDebugSections(args);
   config->cref = args.hasFlag(OPT_cref, OPT_no_cref, false);
config->timeTraceGranularity =
       args::getInteger(args, OPT_time_trace_granularity, 500);
   config->trace = args.hasArg(OPT_trace);
   config->undefined = args::getStrings(args, OPT_undefined);
   config->undefinedVersion =
       args.hasFlag(OPT_undefined_version, OPT_no_undefined_version, true);
   config->unique = args.hasArg(OPT_unique);
   config->useAndroidRelrTags = args.hasFlag(
       OPT_use_android_relr_tags, OPT_no_use_android_relr_tags, false);
-  config->unresolvedSymbols = getUnresolvedSymbolPolicy(args);
   config->warnBackrefs =
       args.hasFlag(OPT_warn_backrefs, OPT_no_warn_backrefs, false);
   config->warnCommon = args.hasFlag(OPT_warn_common, OPT_no_warn_common, false);
   config->warnIfuncTextrel =
       args.hasFlag(OPT_warn_ifunc_textrel, OPT_no_warn_ifunc_textrel, false);
   config->warnSymbolOrdering =
       args.hasFlag(OPT_warn_symbol_ordering, OPT_no_warn_symbol_ordering, true);
   config->zCombreloc = getZFlag(args, "combreloc", "nocombreloc", true);
static void readConfigs(opt::InputArgList &args) {
   config->zRetpolineplt = hasZOption(args, "retpolineplt");
   config->zRodynamic = hasZOption(args, "rodynamic");
   config->zSeparate = getZSeparate(args);
   config->zShstk = hasZOption(args, "shstk");
   config->zStackSize = args::getZOptionValue(args, OPT_z, "stack-size", 0);
   config->zStartStopVisibility = getZStartStopVisibility(args);
   config->zText = getZFlag(args, "text", "notext", true);
   config->zWxneeded = hasZOption(args, "wxneeded");
+  setUnresolvedSymbolPolicy(args);
 
   for (opt::Arg *arg : args.filtered(OPT_z)) {
     std::pair<StringRef, StringRef> option =
         StringRef(arg->getValue()).split('=');
     if (option.first != "dead-reloc-in-nonalloc")
       continue;
     constexpr StringRef errPrefix = "-z dead-reloc-in-nonalloc=: ";
     std::pair<StringRef, StringRef> kv = option.second.split('=');

lld/ELF/Writer.cpp

script->processSymbolAssignments();
     }
   }
 
   if (in.plt && in.plt->isNeeded())
     in.plt->addSymbols();
   if (in.iplt && in.iplt->isNeeded())
     in.iplt->addSymbols();
 
-  if (!config->allowShlibUndefined) {
+  if (config->unresolvedSymbolsInShlib != UnresolvedPolicy::Ignore) {
     // Error on undefined symbols in a shared object, if all of its DT_NEEDED
     // entries are seen. These cases would otherwise lead to runtime errors
     // reported by the dynamic linker.
     //
     // ld.bfd traces all DT_NEEDED to emulate the logic of the dynamic linker to
     // catch more cases. That is too much for us. Our approach resembles the one
     // used in ld.gold, achieves a good balance to be useful but not too smart.
     for (SharedFile *file : sharedFiles)
       file->allNeededIsKnown =
           llvm::all_of(file->dtNeeded, [&](StringRef needed) {
             return symtab->soNames.count(needed);
           });
 
     for (Symbol *sym : symtab->symbols())
       if (sym->isUndefined() && !sym->isWeak())
         if (auto *f = dyn_cast_or_null<SharedFile>(sym->file))
-          if (f->allNeededIsKnown)
-            errorOrWarn(toString(f) + ": undefined reference to " +
-                        toString(*sym) + " [--no-allow-shlib-undefined]");
+          if (f->allNeededIsKnown) {
+            auto diagnose = config->unresolvedSymbolsInShlib ==
+                                    UnresolvedPolicy::ReportError
+                                ? errorOrWarn
+                                : warn;
+            diagnose(toString(f) + ": undefined reference to " +
           }
+  }
 
   {
     llvm::TimeTraceScope timeScope("Add symbols to symtabs");
     // Now that we have defined all possible global symbols including linker-
     // synthesized ones. Visit all symbols to give the finishing touches.
     for (Symbol *sym : symtab->symbols()) {
       if (!includeInSymtab(*sym))
         continue;

lld/test/ELF/allow-shlib-undefined.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
 # RUN:   %p/Inputs/allow-shlib-undefined.s -o %t1.o
 # RUN: ld.lld -shared %t1.o -o %t.so
 
 # RUN: ld.lld --allow-shlib-undefined %t.o %t.so -o /dev/null
 # RUN: not ld.lld --no-allow-shlib-undefined %t.o %t.so -o /dev/null 2>&1 | FileCheck %s
 # Executable defaults to --no-allow-shlib-undefined
 # RUN: not ld.lld %t.o %t.so -o /dev/null 2>&1 | FileCheck %s
-# RUN: ld.lld %t.o %t.so --noinhibit-exec -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld %t.o %t.so --noinhibit-exec -o /dev/null 2>&1 | FileCheck %s --check-prefix=WARN
+# RUN: ld.lld %t.o %t.so --warn-unresolved-symbols -o /dev/null 2>&1 | FileCheck %s --check-prefix=WARN
 # -shared defaults to --allow-shlib-undefined
 # RUN: ld.lld -shared %t.o %t.so -o /dev/null
 
 # RUN: echo | llvm-mc -filetype=obj -triple=x86_64-unknown-linux -o %tempty.o
 # RUN: ld.lld -shared %tempty.o -o %tempty.so
 # RUN: ld.lld -shared %t1.o %tempty.so -o %t2.so
 # RUN: ld.lld --no-allow-shlib-undefined %t.o %t2.so -o /dev/null
 
 # DSO with undefines:
 # should link with or without any of these options.
 # RUN: ld.lld -shared %t1.o -o /dev/null
 # RUN: ld.lld -shared --allow-shlib-undefined %t1.o -o /dev/null
 # RUN: ld.lld -shared --no-allow-shlib-undefined %t1.o -o /dev/null
 
 .globl _start
 _start:
   callq _shared@PLT
 
-# CHECK: {{.*}}.so: undefined reference to _unresolved [--no-allow-shlib-undefined]
+# CHECK: error: {{.*}}.so: undefined reference to _unresolved [--no-allow-shlib-undefined]
+# WARN: warning: {{.*}}.so: undefined reference to _unresolved [--no-allow-shlib-undefined]

lld/test/ELF/unresolved-symbols.s

 
 ## Also ignore all should not produce error for symbols from DSOs.
 # RUN: ld.lld %t1.o %t.so -o %t1_3 --allow-shlib-undefined --unresolved-symbols=ignore-all
 # RUN: llvm-readobj %t1_3 > /dev/null 2>&1
 
 ## Ignoring undefines in objects should not produce error for symbol from object.
 # RUN: ld.lld %t1.o %t2.o -o %t2 --unresolved-symbols=ignore-in-object-files
 # RUN: llvm-readobj %t2 > /dev/null 2>&1
-## And still should not should produce for undefines from DSOs.
-# RUN: ld.lld %t1.o %t.so -o /dev/null --allow-shlib-undefined --unresolved-symbols=ignore-in-object-files
-# RUN: llvm-readobj %t2 > /dev/null 2>&1
+## --unresolved-symbols overrides a previous --allow-shlib-undefined.
+# RUN: not ld.lld %t1.o %t.so -o /dev/null --allow-shlib-undefined --unresolved-symbols=ignore-in-object-files 2>&1 | FileCheck %s --check-prefix=SHLIB
+
+# SHLIB: error: {{.*}}.so: undefined reference to undef [--no-allow-shlib-undefined]
 
 ## Ignoring undefines in shared should produce error for symbol from object.
 # RUN: not ld.lld %t2.o -o /dev/null --unresolved-symbols=ignore-in-shared-libs 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERRUND %s
 ## And should not produce errors for symbols from DSO.
 # RUN: ld.lld %t1.o %t.so -o %t3_1 --allow-shlib-undefined --unresolved-symbols=ignore-in-shared-libs
 # RUN: llvm-readobj %t3_1 > /dev/null 2>&1
 
 ## Ignoring undefines in shared libs should not produce error for symbol from object
 ## if we are linking DSO.
 # RUN: ld.lld -shared %t1.o -o %t4 --unresolved-symbols=ignore-in-shared-libs
 # RUN: llvm-readobj %t4 > /dev/null 2>&1
 
 ## Do not report undefines if linking relocatable.
 # RUN: ld.lld -r %t1.o %t2.o -o %t5 --unresolved-symbols=report-all
 # RUN: llvm-readobj %t5 > /dev/null 2>&1
 
-## report-all is the default one. Check that we do not report
-## undefines from DSO and do report undefines from object. With
-## report-all specified and without.
-# RUN: ld.lld -shared %t1.o %t.so -o %t6 --unresolved-symbols=report-all
-# RUN: llvm-readobj %t6 > /dev/null 2>&1
+## report-all is the default when linking an executable. Check that we report
+## unresolved undefines from both DSO and regular object files.
+# RUN: not ld.lld -shared %t1.o %t.so -o /dev/null --unresolved-symbols=report-all 2>&1 | FileCheck %s --check-prefix=SHLIB
 # RUN: ld.lld -shared %t1.o %t.so -o %t6_1
 # RUN: llvm-readobj %t6_1 > /dev/null 2>&1
 # RUN: not ld.lld %t2.o -o /dev/null --unresolved-symbols=report-all 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERRUND %s
 # RUN: not ld.lld %t2.o -o /dev/null 2>&1 | FileCheck -check-prefix=ERRUND %s
 
 .globl _start
 _start: