[lld] 87ce7b4 - [lld-macho] Simplify archive loading logic
Jez Ng via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 19 18:56:43 PDT 2022
Author: Jez Ng
Date: 2022-07-19T21:56:24-04:00
New Revision: 87ce7b41d82a1b19ba5131d9c1068f1be0b77025
URL: https://github.com/llvm/llvm-project/commit/87ce7b41d82a1b19ba5131d9c1068f1be0b77025
DIFF: https://github.com/llvm/llvm-project/commit/87ce7b41d82a1b19ba5131d9c1068f1be0b77025.diff
LOG: [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
Added:
Modified:
lld/MachO/Config.h
lld/MachO/Driver.cpp
lld/test/MachO/force-load-swift-libs.ll
Removed:
################################################################################
diff --git a/lld/MachO/Config.h b/lld/MachO/Config.h
index 90fae41e42ae2..c7e4b4f967829 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<Configuration> config;
} // namespace macho
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index d9e92a9a933ac..63a0e7f3a8434 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<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;
@@ -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<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)));
@@ -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<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)
@@ -406,15 +426,13 @@ static void addLibrary(StringRef name, bool isNeeded, bool isWeak,
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;
@@ -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<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:
@@ -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<const char *> 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 ed8cf20b53a84..70324e10e9ce7 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
More information about the llvm-commits
mailing list