Skip to content

Commit

Permalink
[lld-macho] Simplify archive loading logic
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@int3
int3 committed Jul 20, 2022
1 parent 09d4dbc commit 87ce7b4
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 48 deletions.
9 changes: 1 addition & 8 deletions lld/MachO/Config.h
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
92 changes: 53 additions & 39 deletions lld/MachO/Driver.cpp
View file
Expand Up @@ -247,16 +247,26 @@ 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;
};

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;
Expand All @@ -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.
Expand All @@ -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)));
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -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");
}
Expand All @@ -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
Expand Down Expand Up @@ -996,38 +1011,38 @@ 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:
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:
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 10 additions & 1 deletion lld/test/MachO/force-load-swift-libs.ll
View file
Expand Up @@ -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
Expand All @@ -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

Expand Down

0 comments on commit 87ce7b4

Please sign in to comment.