[lld-macho] Use ld64's LC_LINKER_OPTIONS behavior by default

[keith]

Summary

By default ld64 ignores invalid LC_LINKER_OPTIONS unless the link fails,
in which case it prints a warning. Originally lld chose to be strict
about these, but it has uncovered that many of these exist in open
source projects today, since before developers never would have noticed
this issue. In order to make adoption of lld easier, this mirrors ld64's
behavior, while also adding a --strict-auto-link-options flag if
projects want to audit their libraries for these invalid options.

More discussion on https://reviews.llvm.org/D140225
Fixes https://github.com/llvm/llvm-project/issues/59627

[keith]

Alternatively in this patch we could not include --strict-auto-link-options and we could remove --ignore-auto-link-option

[int3]

Alternatively in this patch we could not include --strict-auto-link-options and we could remove --ignore-auto-link-option

Nah I think this current solution is good (:

lld/MachO/Driver.cpp

1903–1904

would be nice to include the file where the error originated from, in the format mentioned here: https://reviews.llvm.org/D140225#inline-1357058

lld/MachO/Options.td

107

how do you feel about just calling this strict-auto-link? I don't think there's any other source of auto-linking, so there should be no potential confusion here

(same for the corresponding variable names)

lld/test/MachO/lc-linker-option.ll

151

nit: usually we use hyphens instead of underscores for these (to match the -NEXT etc suffixes)

[keith]

Rename CLI arg, add file path to warning / error

[int3]

lgtm

lld/MachO/Driver.cpp

412

refLoc seems kind of confusing since we already have a struct Location that represents a file + offset. How about refFile, or originFile?

430

personally I try to defer string building until we know we have to emit the warning. But I guess this code path occurs fairly rarely so there's not much of a perf concern (so this is fine)

lld/test/MachO/lc-linker-option.ll

155–157

could you hyphenate these too?

[keith]

Improve variable naming, require originFile existance

lld/MachO/Config.h

struct Configuration {
   bool deadStrip = false;
   bool errorForArchMismatch = false;
   bool ignoreAutoLink = false;
   // ld64 allows invalid auto link options as long as the link succeeds. LLD
   // does not, but there are cases in the wild where the invalid linker options
   // exist. This allows users to ignore the specific invalid options in the case
   // they can't easily fix them.
   llvm::StringSet<> ignoreAutoLinkOptions;
+  bool strictAutoLink = false;
   PlatformInfo platformInfo;
   std::optional<PlatformInfo> secondaryPlatformInfo;
   NamespaceKind namespaceKind = NamespaceKind::twolevel;
   UndefinedSymbolTreatment undefinedSymbolTreatment =
       UndefinedSymbolTreatment::error;
   ICFLevel icfLevel = ICFLevel::none;
   ObjCStubsMode objcStubsMode = ObjCStubsMode::fast;
   llvm::MachO::HeaderFileType outputType;

lld/MachO/Driver.cpp

if (newFile && !isa<DylibFile>(newFile)) {
     // print the .a name here. Similarly skip lazy files.
     if (config->printEachFile && magic != file_magic::archive && !isLazy)
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
+static std::vector<StringRef> missingAutolinkWarnings;
 static void addLibrary(StringRef name, bool isNeeded, bool isWeak,
                        bool isReexport, bool isHidden, bool isExplicit,
-                       LoadType loadType) {
+                       LoadType loadType, InputFile *originFile = nullptr) {
   if (std::optional<StringRef> path = findLibrary(name)) {
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
             addFile(*path, loadType, /*isLazy=*/false, isExplicit,
                     /*isBundleLoader=*/false, isHidden))) {
       if (isNeeded)
         dylibFile->forceNeeded = true;
       if (isWeak)
         dylibFile->forceWeakImport = true;
       if (isReexport) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
     }
     return;
   }
+  if (loadType == LoadType::LCLinkerOption) {
+    assert(originFile);
+    missingAutolinkWarnings.push_back(
+        saver().save(toString(originFile) +
+                     ": auto-linked library not found for -l" + name));
+    return;
+  }
   error("library not found for -l" + name);
 }
 
 static DenseSet<StringRef> loadedObjectFrameworks;
 static void addFramework(StringRef name, bool isNeeded, bool isWeak,
-                         bool isReexport, bool isExplicit, LoadType loadType) {
+                         bool isReexport, bool isExplicit, LoadType loadType,
+                         InputFile *originFile = nullptr) {
   if (std::optional<StringRef> path = findFramework(name)) {
     if (loadedObjectFrameworks.contains(*path))
       return;
 
     InputFile *file =
         addFile(*path, loadType, /*isLazy=*/false, isExplicit, false);
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(file)) {
       if (isNeeded)
if (auto *dylibFile = dyn_cast_or_null<DylibFile>(file)) {
       // symbols. Frameworks containing static archives are cached separately
       // in addFile() to share caching with libraries, and frameworks
       // containing dylibs should allow overwriting of attributes such as
       // forceNeeded by subsequent loads
       loadedObjectFrameworks.insert(*path);
     }
     return;
   }
+  if (loadType == LoadType::LCLinkerOption) {
+    assert(originFile);
+    missingAutolinkWarnings.push_back(saver().save(
+        toString(originFile) +
+        ": auto-linked framework not found for -framework " + name));
+    return;
+  }
   error("framework not found for -framework " + name);
 }
 
 // Parses LC_LINKER_OPTION contents, which can add additional command line
 // flags. This directly parses the flags instead of using the standard argument
 // parser to improve performance.
 void macho::parseLCLinkerOption(InputFile *f, unsigned argc, StringRef data) {
   if (config->ignoreAutoLink)
void macho::parseLCLinkerOption(InputFile *f, unsigned argc, StringRef data) {
 
   unsigned i = 0;
   StringRef arg = argv[i];
   if (arg.consume_front("-l")) {
     if (config->ignoreAutoLinkOptions.contains(arg))
       return;
     addLibrary(arg, /*isNeeded=*/false, /*isWeak=*/false,
                /*isReexport=*/false, /*isHidden=*/false, /*isExplicit=*/false,
-               LoadType::LCLinkerOption);
+               LoadType::LCLinkerOption, f);
   } else if (arg == "-framework") {
     StringRef name = argv[++i];
     if (config->ignoreAutoLinkOptions.contains(name))
       return;
     addFramework(name, /*isNeeded=*/false, /*isWeak=*/false,
                  /*isReexport=*/false, /*isExplicit=*/false,
-                 LoadType::LCLinkerOption);
+                 LoadType::LCLinkerOption, f);
   } else {
     error(arg + " is not allowed in LC_LINKER_OPTION");
   }
 }
 
 static void addFileList(StringRef path, bool isLazy) {
   std::optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
ctx->e.cleanupCallback = []() {
     resolvedFrameworks.clear();
     resolvedLibraries.clear();
     cachedReads.clear();
     concatOutputSections.clear();
     inputFiles.clear();
     inputSections.clear();
     loadedArchives.clear();
     loadedObjectFrameworks.clear();
+    missingAutolinkWarnings.clear();
     syntheticSections.clear();
     thunkMap.clear();
 
     firstTLVDataSection = nullptr;
     tar = nullptr;
     memset(&in, 0, sizeof(in));
 
     resetLoadedDylibs();
config->callGraphProfileSort = args.hasFlag(
       OPT_call_graph_profile_sort, OPT_no_call_graph_profile_sort, true);
   config->printSymbolOrder = args.getLastArgValue(OPT_print_symbol_order_eq);
   config->forceExactCpuSubtypeMatch =
       getenv("LD_DYLIB_CPU_SUBTYPES_MUST_MATCH");
   config->objcStubsMode = getObjCStubsMode(args);
   config->ignoreAutoLink = args.hasArg(OPT_ignore_auto_link);
   for (const Arg *arg : args.filtered(OPT_ignore_auto_link_option))
     config->ignoreAutoLinkOptions.insert(arg->getValue());
+  config->strictAutoLink = args.hasArg(OPT_strict_auto_link);
 
   for (const Arg *arg : args.filtered(OPT_alias)) {
     config->aliasedSymbols.push_back(
         std::make_pair(arg->getValue(0), arg->getValue(1)));
   }
 
   // FIXME: Add a commandline flag for this too.
   if (const char *zero = getenv("ZERO_AR_DATE"))
#endif
   }
 
   if (config->timeTraceEnabled) {
     checkError(timeTraceProfilerWrite(
         args.getLastArgValue(OPT_time_trace_eq).str(), config->outputFile));
 
     timeTraceProfilerCleanup();
   }
+
+  if (errorCount() != 0 || config->strictAutoLink)
+    for (const auto &warning : missingAutolinkWarnings)
+      warn(warning);
+
   return errorCount() == 0;
 }

lld/MachO/Options.td

def print_symbol_order_eq: Joined<["--"], "print-symbol-order=">,
     HelpText<"Print a symbol order specified by --call-graph-profile-sort into the specified file">,
     Group<grp_lld>;
 def ignore_auto_link_option : Separate<["--"], "ignore-auto-link-option">,
     Group<grp_lld>;
 def ignore_auto_link_option_eq : Joined<["--"], "ignore-auto-link-option=">,
     Alias<!cast<Separate>(ignore_auto_link_option)>,
     HelpText<"Ignore a single auto-linked library or framework. Useful to ignore invalid options that ld64 ignores">,
     Group<grp_lld>;
+def strict_auto_link : Flag<["--"], "strict-auto-link">,
+    HelpText<"Always warn for missing frameworks or libraries if they are loaded via LC_LINKER_OPTIONS">,
+    Group<grp_lld>;
 
 // This is a complete Options.td compiled from Apple's ld(1) manpage
 // dated 2018-03-07 and cross checked with ld64 source code in repo
 // https://github.com/apple-opensource/ld64 at git tag "512.4" dated
 // 2018-03-18.
 
 // Flags<[HelpHidden]> marks options that are not yet ported to lld,
 // and serve as a scoreboard for annotating our progress toward

lld/test/MachO/lc-linker-option.ll

 ;; under ASAN.
 ; RUN: llc %t/foo.ll -o %t/foo.o -filetype=obj
 ; RUN: mkdir -p %t/Foo.framework
 ;; In a proper framework, this is technically supposed to be a symlink to the
 ;; actual archive at Foo.framework/Versions/Current, but we skip that here so
 ;; that this test can run on Windows.
 ; RUN: llvm-ar rcs %t/Foo.framework/Foo %t/foo.o
 ; RUN: llc %t/load-framework-foo.ll -o %t/load-framework-foo.o -filetype=obj
+; RUN: llc %t/load-framework-undefined-symbol.ll -o %t/load-framework-undefined-symbol.o -filetype=obj
+; RUN: llc %t/load-missing.ll -o %t/load-missing.o -filetype=obj
 ; RUN: llc %t/main.ll -o %t/main.o -filetype=obj
 ; RUN: %lld %t/load-framework-foo.o %t/main.o -o %t/main -F%t
 ; RUN: llvm-objdump --macho --syms %t/main | FileCheck %s --check-prefix=SYMS
 
 ;; Make sure -all_load and -ObjC have no effect on libraries loaded via
 ;; LC_LINKER_OPTION flags.
 ; RUN: llc %t/load-library-foo.ll -o %t/load-library-foo.o -filetype=obj
 ; RUN: llvm-ar rcs %t/libfoo.a %t/foo.o

 ; RUN: %lld -F %t -framework Foo -framework Foo %t/main -o /dev/null
 
 ; RUN: llvm-as %t/foo.ll -o %t/Foo.framework/Foo
 ; RUN: llvm-as %t/load-framework-twice.ll -o %t/main
 ;; Order of the object with the LC_LINKER_OPTION vs -framework arg is important.
 ; RUN: %lld %t/main -F %t -framework Foo -framework Foo -o /dev/null
 ; RUN: %lld -F %t -framework Foo -framework Foo %t/main -o /dev/null
 
+;; Checks that "framework not found" errors from LC_LINKER_OPTIONS are not
+;; emitted unless the link fails or --strict-auto-link is passed.
+; RUN: %lld -ObjC %t/load-framework-foo.o %t/main.o -o %t/main-no-foo.out
+; RUN: llvm-objdump --macho --syms %t/main-no-foo.out | FileCheck %s --check-prefix=SYMS-NO-FOO
+; RUN: not %lld --strict-auto-link -ObjC %t/load-missing.o %t/main.o -o %t/main-no-foo.out 2>&1 \
+; RUN:   | FileCheck %s --check-prefix=MISSING-AUTO-LINK
+; RUN: %no-fatal-warnings-lld --strict-auto-link -ObjC %t/load-missing.o %t/main.o -o %t/main-no-foo.out 2>&1 \
+; RUN:   | FileCheck %s --check-prefix=MISSING-AUTO-LINK
+; RUN: not %lld -ObjC %t/load-framework-undefined-symbol.o %t/load-missing.o %t/main.o -o %t/main-no-foo.out 2>&1 \
+; RUN:   | FileCheck %s --check-prefixes=UNDEFINED-SYMBOL,MISSING-AUTO-LINK
+
+;; Verify that nothing from the framework is included.
+; SYMS-NO-FOO:       SYMBOL TABLE:
+; SYMS-NO-FOO-NEXT:  g     F __TEXT,__text _main
+; SYMS-NO-FOO-NOT:   g     O __DATA,__objc_data _OBJC_CLASS_$_TestClass
+
+; UNDEFINED-SYMBOL: undefined symbol: __SomeUndefinedSymbol
+; MISSING-AUTO-LINK: {{.+}}load-missing.o: auto-linked framework not found for -framework Foo
+; MISSING-AUTO-LINK: {{.+}}load-missing.o: auto-linked library not found for -lBar
+
 ;--- framework.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"CoreFoundation"}
 !llvm.linker.options = !{!0}
 
 declare void @_CFBigNumGetInt128(...)

 
 ;--- load-framework-foo.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"Foo"}
 !llvm.linker.options = !{!0}
 
+;--- load-missing.ll
+target triple = "x86_64-apple-macosx10.15.0"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+!0 = !{!"-framework", !"Foo"}
+!1 = !{!"-lBar"}
+!llvm.linker.options = !{!0, !1}
+
+;--- load-framework-undefined-symbol.ll
+target triple = "x86_64-apple-macosx10.15.0"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+declare void @_SomeUndefinedSymbol(...)
+define void @foo() {
+  call void @_SomeUndefinedSymbol()
+  ret void
+}
+
 ;--- load-framework-twice.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"Foo"}
 !llvm.linker.options = !{!0, !0}
 
 define void @main() {