[lld-macho] Simplify archive loading logic

[int3]

Summary

This is a follow-on to D129556: [lld-macho] Fix loading same libraries from both LC_LINKER_OPTION and command line. I've refactored the code such that
addFile() no longer needs to take an extra parameter. Additionally,
the "do we force-load or not" policy logic is now fully contained within
addFile, instead of being split between addFile and
parseLCLinkerOptions. This also allows us to move the ForceLoad (now
LoadType) enum out of the header file.

Additionally, we can now correctly report loads induced by
LC_LINKER_OPTION in our -why_load output.

I've also added another test to check that CLI library non-force-loads
take precedence over LC_LINKER_OPTION + -force_load_swift_libs. (The
existing logic is correct, just untested.)

[thakis]

lld/MachO/Driver.cpp

299–302

!isCommandLineLoad instead of == false

307

Why && forceLoadSwift && path::filename(path).startswith("libswift")? Does this in LC_LINKER_OPTIONS only apply to swift archives?

[int3]

lld/MachO/Driver.cpp

299–302

d'oh, of course

307

yup

[int3]

update

[thakis]

LG

lld/MachO/Driver.cpp

254

Maybe say "only applies to swift files" in the comment here, or put "Swift" in the constant's name then?

[int3]

lld/MachO/Driver.cpp

254

the swift-force-loading only applies to libraries though, not frameworks, but we pass this enum for both library and framework loads

I can add a comment to Config::forceLoadSwift though

[int3]

lld/MachO/Driver.cpp

254

also this enum is passed regardless of whether we are doing a force-load. LoadType just tells us what induced the load, not *how* the load should be performed (i.e. forced or not). I will add that in a comment

[int3]

more comments and another test

lld/MachO/Config.h

 struct Configuration {
   Symbol *entry = nullptr;
   bool hasReexports = false;
   bool allLoad = false;
   bool applicationExtension = false;
   bool archMultiple = false;
   bool exportDynamic = false;
   bool forceLoadObjC = false;
-  bool forceLoadSwift = false;
+  bool forceLoadSwift = false; // Only applies to LC_LINKER_OPTIONs.
   bool staticLink = false;
   bool implicitDylibs = false;
   bool isPic = false;
   bool headerPadMaxInstallNames = false;
   bool markDeadStrippableDylib = false;
   bool printDylibSearch = false;
   bool printEachFile = false;
   bool printWhyLoad = false;
struct Configuration {
 
   llvm::MachO::Architecture arch() const { return platformInfo.target.Arch; }
 
   llvm::MachO::PlatformType platform() const {
     return platformInfo.target.Platform;
   }
 };
 
-// Whether to force-load an archive.
-enum class ForceLoad {
-  Default, // Apply -all_load or -ObjC behaviors if those flags are enabled
-  Yes,     // Always load the archive, regardless of other flags
-  No,      // Never load the archive, regardless of other flags
-};
-
 extern std::unique_ptr<Configuration> config;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/Driver.cpp

for (const Arg *arg :
     case OPT_max_relative_cache_size_lto:
       add(Twine("cache_size=") + arg->getValue() + "%");
       break;
     }
   }
   return CHECK(parseCachePruningPolicy(ltoPolicy), "invalid LTO cache policy");
 }
 
+// What caused a given library to be loaded. Only relevant for archives.
+// Note that this does not tell us *how* we should load the library, i.e.
+// whether we should do it lazily or eagerly (AKA force loading). The "how" is
+// decided within addFile().
+enum class LoadType {
+  CommandLine,      // Library was passed as a regular CLI argument
+  CommandLineForce, // Library was passed via `-force_load`
+  LCLinkerOption,   // Library was passed via LC_LINKER_OPTIONS
+};
+
 struct ArchiveFileInfo {
   ArchiveFile *file;
   bool isCommandLineLoad;
 };
 
 static DenseMap<StringRef, ArchiveFileInfo> loadedArchives;
 
-static InputFile *addFile(StringRef path, ForceLoad forceLoadArchive,
-                          bool isCommandLineLoad, bool isLazy = false,
-                          bool isExplicit = true, bool isBundleLoader = false) {
+static InputFile *addFile(StringRef path, LoadType loadType,
+                          bool isLazy = false, bool isExplicit = true,
+                          bool isBundleLoader = false) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return nullptr;
   MemoryBufferRef mbref = *buffer;
   InputFile *newFile = nullptr;
 
   file_magic magic = identify_magic(mbref.getBuffer());
   switch (magic) {
   case file_magic::archive: {
+    bool isCommandLineLoad = loadType != LoadType::LCLinkerOption;
     // Avoid loading archives twice. If the archives are being force-loaded,
     // loading them twice would create duplicate symbol errors. In the
     // non-force-loading case, this is just a minor performance optimization.
     // We don't take a reference to cachedFile here because the
     // loadArchiveMember() call below may recursively call addFile() and
     // invalidate this reference.
     auto entry = loadedArchives.find(path);
 
     ArchiveFile *file;
     if (entry == loadedArchives.end()) {
       // No cached archive, we need to create a new one
       std::unique_ptr<object::Archive> archive = CHECK(
           object::Archive::create(mbref), path + ": failed to parse archive");
 
       if (!archive->isEmpty() && !archive->hasSymbolTable())
         error(path + ": archive has no index; run ranlib to add one");
       file = make<ArchiveFile>(std::move(archive));
     } else {
       file = entry->second.file;
-      // If file is previously loaded via command line, or is loaded via
-      // LC_LINKER_OPTION and being loaded via LC_LINKER_OPTION again,
-      // using the cached archive should be enough
-      if (entry->second.isCommandLineLoad ||
-          entry->second.isCommandLineLoad == isCommandLineLoad)
+      // Command-line loads take precedence. If file is previously loaded via
+      // command line, or is loaded via LC_LINKER_OPTION and being loaded via
+      // LC_LINKER_OPTION again, using the cached archive is enough.
+      if (entry->second.isCommandLineLoad || !isCommandLineLoad)
         return file;
     }
 
+    bool isLCLinkerForceLoad = loadType == LoadType::LCLinkerOption &&
+                               config->forceLoadSwift &&
+                               path::filename(path).startswith("libswift");
     if ((isCommandLineLoad && config->allLoad) ||
-        forceLoadArchive == ForceLoad::Yes) {
+        loadType == LoadType::CommandLineForce || isLCLinkerForceLoad) {
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
         Error e = Error::success();
         for (const object::Archive::Child &c : file->getArchive().children(e)) {
-          StringRef reason =
-              forceLoadArchive == ForceLoad::Yes ? "-force_load" : "-all_load";
+          StringRef reason;
+          switch (loadType) {
+            case LoadType::LCLinkerOption:
+              reason = "LC_LINKER_OPTION";
+              break;
+            case LoadType::CommandLineForce:
+              reason = "-force_load";
+              break;
+            case LoadType::CommandLine:
+              reason = "-all_load";
+              break;
+          }
           if (Error e = file->fetch(c, reason))
             error(toString(file) + ": " + reason +
                   " failed to load archive member: " + toString(std::move(e)));
         }
         if (e)
           error(toString(file) +
                 ": Archive::children failed: " + toString(std::move(e)));
       }
if (newFile && !isa<DylibFile>(newFile)) {
     if (config->printEachFile && magic != file_magic::archive && !isLazy)
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
 static void addLibrary(StringRef name, bool isNeeded, bool isWeak,
-                       bool isReexport, bool isExplicit,
-                       ForceLoad forceLoadArchive,
-                       bool isCommandLineLoad = true) {
+                       bool isReexport, bool isExplicit, LoadType loadType) {
   if (Optional<StringRef> path = findLibrary(name)) {
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-            addFile(*path, forceLoadArchive, isCommandLineLoad,
-                    /*isLazy=*/false, isExplicit))) {
+            addFile(*path, loadType, /*isLazy=*/false, isExplicit))) {
       if (isNeeded)
         dylibFile->forceNeeded = true;
       if (isWeak)
         dylibFile->forceWeakImport = true;
       if (isReexport) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
     }
     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,
-                         ForceLoad forceLoadArchive,
-                         bool isCommandLineLoad = true) {
+                         bool isReexport, bool isExplicit, LoadType loadType) {
   if (Optional<StringRef> path = findFramework(name)) {
     if (loadedObjectFrameworks.contains(*path))
       return;
 
-    InputFile *file = addFile(*path, forceLoadArchive, isCommandLineLoad,
-                              /*isLazy=*/false, isExplicit, false);
+    InputFile *file =
+        addFile(*path, loadType, /*isLazy=*/false, isExplicit, false);
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(file)) {
       if (isNeeded)
         dylibFile->forceNeeded = true;
       if (isWeak)
         dylibFile->forceWeakImport = true;
       if (isReexport) {
         config->hasReexports = true;
         dylibFile->reexport = true;
for (unsigned i = 0; i < argc && offset < data.size(); ++i) {
     offset += strlen(data.data() + offset) + 1;
   }
   if (argv.size() != argc || offset > data.size())
     fatal(toString(f) + ": invalid LC_LINKER_OPTION");
 
   unsigned i = 0;
   StringRef arg = argv[i];
   if (arg.consume_front("-l")) {
-    ForceLoad forceLoadArchive =
-        config->forceLoadSwift && arg.startswith("swift") ? ForceLoad::Yes
-                                                          : ForceLoad::No;
     addLibrary(arg, /*isNeeded=*/false, /*isWeak=*/false,
-               /*isReexport=*/false, /*isExplicit=*/false, forceLoadArchive,
-               /*isCommandLineLoad=*/false);
+               /*isReexport=*/false, /*isExplicit=*/false,
+               LoadType::LCLinkerOption);
   } else if (arg == "-framework") {
     StringRef name = argv[++i];
     addFramework(name, /*isNeeded=*/false, /*isWeak=*/false,
-                 /*isReexport=*/false, /*isExplicit=*/false, ForceLoad::No,
-                 /*isCommandLineLoad=*/false);
+                 /*isReexport=*/false, /*isExplicit=*/false,
+                 LoadType::LCLinkerOption);
   } else {
     error(arg + " is not allowed in LC_LINKER_OPTION");
   }
 }
 
 static void addFileList(StringRef path, bool isLazy) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
   for (StringRef path : args::getLines(mbref))
-    addFile(rerootPath(path), ForceLoad::Default, true, isLazy);
+    addFile(rerootPath(path), LoadType::CommandLine, isLazy);
 }
 
 // We expect sub-library names of the form "libfoo", which will match a dylib
 // with a path of .*/libfoo.{dylib, tbd}.
 // XXX ld64 seems to ignore the extension entirely when matching sub-libraries;
 // I'm not sure what the use case for that is.
 static bool markReexport(StringRef searchName, ArrayRef<StringRef> extensions) {
   for (InputFile *file : inputFiles) {
static void createFiles(const InputArgList &args) {
   bool isLazy = false;
   for (const Arg *arg : args) {
     const Option &opt = arg->getOption();
     warnIfDeprecatedOption(opt);
     warnIfUnimplementedOption(opt);
 
     switch (opt.getID()) {
     case OPT_INPUT:
-      addFile(rerootPath(arg->getValue()), ForceLoad::Default, true, isLazy);
+      addFile(rerootPath(arg->getValue()), LoadType::CommandLine, isLazy);
       break;
     case OPT_needed_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), ForceLoad::Default, true)))
+              addFile(rerootPath(arg->getValue()), LoadType::CommandLine)))
         dylibFile->forceNeeded = true;
       break;
     case OPT_reexport_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), ForceLoad::Default, true))) {
+              addFile(rerootPath(arg->getValue()), LoadType::CommandLine))) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
       break;
     case OPT_weak_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), ForceLoad::Default, true)))
+              addFile(rerootPath(arg->getValue()), LoadType::CommandLine)))
         dylibFile->forceWeakImport = true;
       break;
     case OPT_filelist:
       addFileList(arg->getValue(), isLazy);
       break;
     case OPT_force_load:
-      addFile(rerootPath(arg->getValue()), ForceLoad::Yes, true);
+      addFile(rerootPath(arg->getValue()), LoadType::CommandLineForce);
       break;
     case OPT_l:
     case OPT_needed_l:
     case OPT_reexport_l:
     case OPT_weak_l:
       addLibrary(arg->getValue(), opt.getID() == OPT_needed_l,
                  opt.getID() == OPT_weak_l, opt.getID() == OPT_reexport_l,
-                 /*isExplicit=*/true, ForceLoad::Default);
+                 /*isExplicit=*/true, LoadType::CommandLine);
       break;
     case OPT_framework:
     case OPT_needed_framework:
     case OPT_reexport_framework:
     case OPT_weak_framework:
       addFramework(arg->getValue(), opt.getID() == OPT_needed_framework,
                    opt.getID() == OPT_weak_framework,
                    opt.getID() == OPT_reexport_framework, /*isExplicit=*/true,
-                   ForceLoad::Default);
+                   LoadType::CommandLine);
       break;
     case OPT_start_lib:
       if (isLazy)
         error("nested --start-lib");
       isLazy = true;
       break;
     case OPT_end_lib:
       if (!isLazy)
config->printDylibSearch =
       args.hasArg(OPT_print_dylib_search) || getenv("RC_TRACE_DYLIB_SEARCHING");
   config->printEachFile = args.hasArg(OPT_t);
   config->printWhyLoad = args.hasArg(OPT_why_load);
   config->omitDebugInfo = args.hasArg(OPT_S);
   config->errorForArchMismatch = args.hasArg(OPT_arch_errors_fatal);
   if (const Arg *arg = args.getLastArg(OPT_bundle_loader)) {
     if (config->outputType != MH_BUNDLE)
       error("-bundle_loader can only be used with MachO bundle output");
-    addFile(arg->getValue(), ForceLoad::Default, true, /*isLazy=*/false,
-            /*isExplicit=*/false,
-            /*isBundleLoader=*/true);
+    addFile(arg->getValue(), LoadType::CommandLine, /*isLazy=*/false,
+            /*isExplicit=*/false, /*isBundleLoader=*/true);
   }
   if (const Arg *arg = args.getLastArg(OPT_umbrella)) {
     if (config->outputType != MH_DYLIB)
       warn("-umbrella used, but not creating dylib");
     config->umbrella = arg->getValue();
   }
   config->ltoObjPath = args.getLastArgValue(OPT_object_path_lto);
   config->ltoo = args::getInteger(args, OPT_lto_O, 2);

lld/test/MachO/force-load-swift-libs.ll

 ; REQUIRES: x86
 ; RUN: rm -rf %t; split-file %s %t
 
 ; RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/swift-foo.s -o %t/swift-foo.o
 ; RUN: llvm-ar rcs %t/libswiftFoo.a %t/swift-foo.o
 ; RUN: llvm-as %t/lc-linker-opt.ll -o %t/lc-linker-opt.o
 ; RUN: llvm-as %t/no-lc-linker-opt.ll -o %t/no-lc-linker-opt.o
 
-; RUN: %lld -lSystem -force_load_swift_libs -L%t %t/lc-linker-opt.o -o %t/lc-linker-opt
+; RUN: %lld -lSystem -force_load_swift_libs -L%t %t/lc-linker-opt.o -o \
+; RUN:   %t/lc-linker-opt -why_load 2>&1 | FileCheck %s --check-prefix=WHY-LOAD
 ; RUN: llvm-objdump --macho --syms %t/lc-linker-opt | FileCheck %s --check-prefix=HAS-SWIFT
 
 ; RUN: %lld -lSystem -L%t %t/lc-linker-opt.o -o %t/lc-linker-opt-no-force
 ; RUN: llvm-objdump --macho --syms %t/lc-linker-opt-no-force | FileCheck %s --check-prefix=NO-SWIFT
 
 ;; Swift libraries passed on the CLI don't get force-loaded!
 ; RUN: %lld -lSystem -force_load_swift_libs -lswiftFoo -L%t %t/no-lc-linker-opt.o -o %t/no-lc-linker-opt
 ; RUN: llvm-objdump --macho --syms %t/no-lc-linker-opt | FileCheck %s --check-prefix=NO-SWIFT
 
+;; Moreover, if a Swift library is passed on the CLI, that supersedes any
+;; LC_LINKER_OPTIONs that reference it.
+; RUN: %lld -lSystem -force_load_swift_libs -lswiftFoo -L%t %t/lc-linker-opt.o -o %t/both-cli-and-lc-linker-opt
+; RUN: llvm-objdump --macho --syms  %t/both-cli-and-lc-linker-opt | FileCheck %s --check-prefix=NO-SWIFT
+; RUN: %lld -lSystem -force_load_swift_libs -L%t %t/lc-linker-opt.o -lswiftFoo -o %t/both-cli-and-lc-linker-opt
+; RUN: llvm-objdump --macho --syms  %t/both-cli-and-lc-linker-opt | FileCheck %s --check-prefix=NO-SWIFT
+
+; WHY-LOAD: LC_LINKER_OPTION forced load of {{.*}}libswiftFoo.a(swift-foo.o)
 ; HAS-SWIFT: _swift_foo
 ; NO-SWIFT-NOT: _swift_foo
 
 ;--- lc-linker-opt.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 = !{!"-lswiftFoo"}