From 87ce7b41d82a1b19ba5131d9c1068f1be0b77025 Mon Sep 17 00:00:00 2001 From: Jez Ng Date: Tue, 19 Jul 2022 21:54:58 -0400 Subject: [PATCH] [lld-macho] Simplify archive loading logic This is a follow-on to {D129556}. 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.) Reviewed By: #lld-macho, thakis Differential Revision: https://reviews.llvm.org/D130137 --- lld/MachO/Config.h | 9 +--- lld/MachO/Driver.cpp | 92 +++++++++++++++++++-------------- lld/test/MachO/force-load-swift-libs.ll | 11 +++- 3 files changed, 64 insertions(+), 48 deletions(-) diff --git a/lld/MachO/Config.h b/lld/MachO/Config.h index 90fae41..c7e4b4f 100644 --- a/lld/MachO/Config.h +++ b/lld/MachO/Config.h @@ -109,7 +109,7 @@ struct Configuration { 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; @@ -204,13 +204,6 @@ struct Configuration { } }; -// 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 config; } // namespace macho diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp index d9e92a9..63a0e7f 100644 --- a/lld/MachO/Driver.cpp +++ b/lld/MachO/Driver.cpp @@ -247,6 +247,16 @@ static llvm::CachePruningPolicy getLTOCachePolicy(InputArgList &args) { 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; @@ -254,9 +264,9 @@ struct ArchiveFileInfo { static DenseMap 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 buffer = readFile(path); if (!buffer) return nullptr; @@ -266,6 +276,7 @@ static InputFile *addFile(StringRef path, ForceLoad forceLoadArchive, 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. @@ -285,21 +296,33 @@ static InputFile *addFile(StringRef path, ForceLoad forceLoadArchive, file = make(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 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))); @@ -383,13 +406,10 @@ static InputFile *addFile(StringRef path, ForceLoad forceLoadArchive, } 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 path = findLibrary(name)) { if (auto *dylibFile = dyn_cast_or_null( - addFile(*path, forceLoadArchive, isCommandLineLoad, - /*isLazy=*/false, isExplicit))) { + addFile(*path, loadType, /*isLazy=*/false, isExplicit))) { if (isNeeded) dylibFile->forceNeeded = true; if (isWeak) @@ -406,15 +426,13 @@ static void addLibrary(StringRef name, bool isNeeded, bool isWeak, static DenseSet 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 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(file)) { if (isNeeded) dylibFile->forceNeeded = true; @@ -454,17 +472,14 @@ void macho::parseLCLinkerOption(InputFile *f, unsigned argc, StringRef data) { 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"); } @@ -476,7 +491,7 @@ static void addFileList(StringRef path, bool isLazy) { 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 @@ -996,30 +1011,30 @@ static void createFiles(const InputArgList &args) { 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( - 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( - 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( - 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: @@ -1027,7 +1042,7 @@ static void createFiles(const InputArgList &args) { 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: @@ -1036,7 +1051,7 @@ static void createFiles(const InputArgList &args) { 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) @@ -1284,9 +1299,8 @@ bool macho::link(ArrayRef argsArr, llvm::raw_ostream &stdoutOS, 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) diff --git a/lld/test/MachO/force-load-swift-libs.ll b/lld/test/MachO/force-load-swift-libs.ll index ed8cf20..70324e1 100644 --- a/lld/test/MachO/force-load-swift-libs.ll +++ b/lld/test/MachO/force-load-swift-libs.ll @@ -6,7 +6,8 @@ ; 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 @@ -16,6 +17,14 @@ ; 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 -- 2.7.4