[lld] 24979e1 - [lld/mac] Don't load DylibFiles from the DylibFile constructor
Nico Weber via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 1 12:31:44 PDT 2021
Author: Nico Weber
Date: 2021-06-01T15:31:02-04:00
New Revision: 24979e1113adf077572fcc1022ee4e3a534a8a4d
URL: https://github.com/llvm/llvm-project/commit/24979e1113adf077572fcc1022ee4e3a534a8a4d
DIFF: https://github.com/llvm/llvm-project/commit/24979e1113adf077572fcc1022ee4e3a534a8a4d.diff
LOG: [lld/mac] Don't load DylibFiles from the DylibFile constructor
loadDylib() keeps a name->DylibFile cache, but it only writes
to the cache once the DylibFile constructor has completed.
So dylib loads done recursively from the DylibFile constructor
wouldn't use the cache.
Now, we load additional dylibs after writing to the cache,
which means the cache now gets used for dylibs loaded because
they're referenced from other dylibs.
Related to PR49514 and PR50101, but no dramatic behavior change in itself.
(Technically we no longer crash when a tbd file reexports itself,
but that doesn't happen in practice. We now accept it silently instead
of crashing; ld64 has a diag for the reexport cycle.)
Differential Revision: https://reviews.llvm.org/D103423
Added:
Modified:
lld/MachO/DriverUtils.cpp
lld/MachO/InputFiles.cpp
lld/MachO/InputFiles.h
Removed:
################################################################################
diff --git a/lld/MachO/DriverUtils.cpp b/lld/MachO/DriverUtils.cpp
index 2c045c57bdde..d7ab5583f744 100644
--- a/lld/MachO/DriverUtils.cpp
+++ b/lld/MachO/DriverUtils.cpp
@@ -201,10 +201,11 @@ Optional<DylibFile *> macho::loadDylib(MemoryBufferRef mbref,
DylibFile *umbrella,
bool isBundleLoader) {
CachedHashStringRef path(mbref.getBufferIdentifier());
- DylibFile *file = loadedDylibs[path];
+ DylibFile *&file = loadedDylibs[path];
if (file)
return file;
+ DylibFile *newFile;
file_magic magic = identify_magic(mbref.getBuffer());
if (magic == file_magic::tapi_file) {
Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
@@ -214,19 +215,27 @@ Optional<DylibFile *> macho::loadDylib(MemoryBufferRef mbref,
return {};
}
file = make<DylibFile>(**result, umbrella, isBundleLoader);
+
+ // parseReexports() can recursively call loadDylib(). That's fine since
+ // we wrote DylibFile we just loaded to the loadDylib cache via the `file`
+ // reference. But the recursive load can grow loadDylibs, so the `file`
+ // reference might become invalid after parseReexports() -- so copy the
+ // pointer it refers to before going on.
+ newFile = file;
+ newFile->parseReexports(**result);
} else {
assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
magic == file_magic::macho_dynamically_linked_shared_lib_stub ||
magic == file_magic::macho_executable ||
magic == file_magic::macho_bundle);
file = make<DylibFile>(mbref, umbrella, isBundleLoader);
+
+ // parseLoadCommands() can also recursively call loadDylib(). See comment
+ // in previous block for why this means we must copy `file` here.
+ newFile = file;
+ newFile->parseLoadCommands(mbref, umbrella);
}
- // Note that DylibFile's ctor may recursively invoke loadDylib(), which can
- // cause loadedDylibs to get resized and its iterators invalidated. As such,
- // we redo the key lookup here instead of caching an iterator from our earlier
- // lookup at the start of the function.
- loadedDylibs[path] = file;
- return file;
+ return newFile;
}
Optional<StringRef>
diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index 4d3b889586e2..d55776611ca9 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -832,7 +832,7 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
return;
// Initialize symbols.
- DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
+ exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
if (const load_command *cmd = findCommand(hdr, LC_DYLD_INFO_ONLY)) {
auto *c = reinterpret_cast<const dyld_info_command *>(cmd);
parseTrie(buf + c->export_off, c->export_size,
@@ -846,9 +846,12 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
error("LC_DYLD_INFO_ONLY not found in " + toString(this));
return;
}
+}
- const uint8_t *p =
- reinterpret_cast<const uint8_t *>(hdr) + target->headerSize;
+void DylibFile::parseLoadCommands(MemoryBufferRef mb, DylibFile *umbrella) {
+ auto *hdr = reinterpret_cast<const mach_header *>(mb.getBufferStart());
+ const uint8_t *p = reinterpret_cast<const uint8_t *>(mb.getBufferStart()) +
+ target->headerSize;
for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
auto *cmd = reinterpret_cast<const load_command *>(p);
p += cmd->cmdsize;
@@ -877,6 +880,13 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
}
}
+// Some versions of XCode ship with .tbd files that don't have the right
+// platform settings.
+static constexpr std::array<StringRef, 3> skipPlatformChecks{
+ "/usr/lib/system/libsystem_kernel.dylib",
+ "/usr/lib/system/libsystem_platform.dylib",
+ "/usr/lib/system/libsystem_pthread.dylib"};
+
DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
bool isBundleLoader)
: InputFile(DylibKind, interface), refState(RefState::Unreferenced),
@@ -890,13 +900,6 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
compatibilityVersion = interface.getCompatibilityVersion().rawValue();
currentVersion = interface.getCurrentVersion().rawValue();
- // Some versions of XCode ship with .tbd files that don't have the right
- // platform settings.
- static constexpr std::array<StringRef, 3> skipPlatformChecks{
- "/usr/lib/system/libsystem_kernel.dylib",
- "/usr/lib/system/libsystem_platform.dylib",
- "/usr/lib/system/libsystem_pthread.dylib"};
-
if (!is_contained(skipPlatformChecks, dylibName) &&
!is_contained(interface.targets(), config->platformInfo.target)) {
error(toString(this) + " is incompatible with " +
@@ -904,7 +907,7 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
return;
}
- DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
+ exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
auto addSymbol = [&](const Twine &name) -> void {
symbols.push_back(symtab->addDylib(saver.save(name), exportingFile,
/*isWeakDef=*/false,
@@ -934,10 +937,11 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
break;
}
}
+}
+void DylibFile::parseReexports(const llvm::MachO::InterfaceFile &interface) {
const InterfaceFile *topLevel =
interface.getParent() == nullptr ? &interface : interface.getParent();
-
for (InterfaceFileRef intfRef : interface.reexportedLibraries()) {
InterfaceFile::const_target_range targets = intfRef.targets();
if (is_contained(skipPlatformChecks, intfRef.getInstallName()) ||
diff --git a/lld/MachO/InputFiles.h b/lld/MachO/InputFiles.h
index 6e58601ae56a..cc57591e0758 100644
--- a/lld/MachO/InputFiles.h
+++ b/lld/MachO/InputFiles.h
@@ -130,6 +130,13 @@ class OpaqueFile : public InputFile {
// .dylib file
class DylibFile : public InputFile {
public:
+ explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
+ bool isBundleLoader = false);
+
+ explicit DylibFile(const llvm::MachO::InterfaceFile &interface,
+ DylibFile *umbrella = nullptr,
+ bool isBundleLoader = false);
+
// Mach-O dylibs can re-export other dylibs as sub-libraries, meaning that the
// symbols in those sub-libraries will be available under the umbrella
// library's namespace. Those sub-libraries can also have their own
@@ -137,16 +144,13 @@ class DylibFile : public InputFile {
// the root dylib to ensure symbols in the child library are correctly bound
// to the root. On the other hand, if a dylib is being directly loaded
// (through an -lfoo flag), then `umbrella` should be a nullptr.
- explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
- bool isBundleLoader = false);
-
- explicit DylibFile(const llvm::MachO::InterfaceFile &interface,
- DylibFile *umbrella = nullptr,
- bool isBundleLoader = false);
+ void parseLoadCommands(MemoryBufferRef mb, DylibFile *umbrella);
+ void parseReexports(const llvm::MachO::InterfaceFile &interface);
static bool classof(const InputFile *f) { return f->kind() == DylibKind; }
StringRef dylibName;
+ DylibFile *exportingFile;
uint32_t compatibilityVersion = 0;
uint32_t currentVersion = 0;
int64_t ordinal = 0; // Ordinal numbering starts from 1, so 0 is a sentinel
More information about the llvm-commits
mailing list