From 544148ae702ac3e94aa41b656e9f6d11b382f9f6 Mon Sep 17 00:00:00 2001 From: Jez Ng Date: Mon, 14 Dec 2020 17:59:22 -0500 Subject: [PATCH] [lld-macho] -weak_{library,framework} should always take priority We were not setting forceWeakImport for file paths given by `-weak_library` if we had already loaded the file. This diff fixes that by having `loadDylib` return a cached DylibFile instance even if we have already loaded that file. We still avoid emitting multiple LC_LOAD_DYLIBs, but we achieve this by making inputFiles a SetVector instead of relying on the `loadedDylibs` cache. Reviewed By: #lld-macho, smeenai Differential Revision: https://reviews.llvm.org/D93255 --- lld/MachO/Driver.cpp | 11 +++--- lld/MachO/DriverUtils.cpp | 43 +++++++++++------------ lld/MachO/InputFiles.cpp | 6 ++-- lld/MachO/InputFiles.h | 3 +- lld/test/MachO/invalid/duplicate-symbol.s | 1 + lld/test/MachO/weak-import.s | 4 +++ 6 files changed, 36 insertions(+), 32 deletions(-) diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp index c456d2fb1fd5be..96972cb17e6fa5 100644 --- a/lld/MachO/Driver.cpp +++ b/lld/MachO/Driver.cpp @@ -274,8 +274,7 @@ static InputFile *addFile(StringRef path, bool forceLoadArchive) { if (config->allLoad || forceLoadArchive) { if (Optional buffer = readFile(path)) { for (const ArchiveMember &member : getArchiveMembers(*buffer)) { - inputFiles.push_back( - make(member.mbref, member.modTime, path)); + inputFiles.insert(make(member.mbref, member.modTime, path)); printArchiveMemberLoad( (forceLoadArchive ? "-force_load" : "-all_load"), inputFiles.back()); @@ -293,7 +292,7 @@ static InputFile *addFile(StringRef path, bool forceLoadArchive) { if (Optional buffer = readFile(path)) { for (const ArchiveMember &member : getArchiveMembers(*buffer)) { if (hasObjCSection(member.mbref)) { - inputFiles.push_back( + inputFiles.insert( make(member.mbref, member.modTime, path)); printArchiveMemberLoad("-ObjC", inputFiles.back()); } @@ -325,7 +324,7 @@ static InputFile *addFile(StringRef path, bool forceLoadArchive) { // print the .a name here. if (config->printEachFile && magic != file_magic::archive) lld::outs() << toString(newFile) << '\n'; - inputFiles.push_back(newFile); + inputFiles.insert(newFile); } return newFile; } @@ -521,7 +520,7 @@ static void compileBitcodeFiles() { lto->add(*bitcodeFile); for (ObjFile *file : lto->compile()) - inputFiles.push_back(file); + inputFiles.insert(file); } // Replaces common symbols with defined symbols residing in __common sections. @@ -873,7 +872,7 @@ bool macho::link(llvm::ArrayRef argsArr, bool canExitEarly, StringRef fileName = arg->getValue(2); Optional buffer = readFile(fileName); if (buffer) - inputFiles.push_back(make(*buffer, segName, sectName)); + inputFiles.insert(make(*buffer, segName, sectName)); } // Initialize InputSections. diff --git a/lld/MachO/DriverUtils.cpp b/lld/MachO/DriverUtils.cpp index 05677a9df78d1e..5040f634e181a8 100644 --- a/lld/MachO/DriverUtils.cpp +++ b/lld/MachO/DriverUtils.cpp @@ -15,7 +15,7 @@ #include "lld/Common/Memory.h" #include "lld/Common/Reproduce.h" #include "llvm/ADT/CachedHashString.h" -#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/Option/Arg.h" #include "llvm/Option/ArgList.h" #include "llvm/Option/Option.h" @@ -168,33 +168,32 @@ Optional macho::resolveDylibPath(StringRef path) { return {}; } -static Optional makeDylibFromTapi(MemoryBufferRef mbref, - DylibFile *umbrella) { - Expected> result = TextAPIReader::get(mbref); - if (!result) { - error("could not load TAPI file at " + mbref.getBufferIdentifier() + ": " + - toString(result.takeError())); - return {}; - } - return make(**result, umbrella); -} - -static DenseSet loadedDylibs; +// It's not uncommon to have multiple attempts to load a single dylib, +// especially if it's a commonly re-exported core library. +static DenseMap loadedDylibs; Optional macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella) { StringRef path = mbref.getBufferIdentifier(); - if (loadedDylibs.contains(CachedHashStringRef(path))) - return {}; - loadedDylibs.insert(CachedHashStringRef(path)); + DylibFile *&file = loadedDylibs[CachedHashStringRef(path)]; + if (file) + return file; file_magic magic = identify_magic(mbref.getBuffer()); - if (magic == file_magic::tapi_file) - return makeDylibFromTapi(mbref, umbrella); - - assert(magic == file_magic::macho_dynamically_linked_shared_lib || - magic == file_magic::macho_dynamically_linked_shared_lib_stub); - return make(mbref, umbrella); + if (magic == file_magic::tapi_file) { + Expected> result = TextAPIReader::get(mbref); + if (!result) { + error("could not load TAPI file at " + mbref.getBufferIdentifier() + + ": " + toString(result.takeError())); + return {}; + } + file = make(**result, umbrella); + } else { + assert(magic == file_magic::macho_dynamically_linked_shared_lib || + magic == file_magic::macho_dynamically_linked_shared_lib_stub); + file = make(mbref, umbrella); + } + return file; } uint32_t macho::getModTime(StringRef path) { diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp index 92fc7d10ebb082..4bf568284c54ce 100644 --- a/lld/MachO/InputFiles.cpp +++ b/lld/MachO/InputFiles.cpp @@ -85,7 +85,7 @@ std::string lld::toString(const InputFile *f) { .str(); } -std::vector macho::inputFiles; +SetVector macho::inputFiles; std::unique_ptr macho::tar; int InputFile::idCount = 0; @@ -516,7 +516,7 @@ static bool isImplicitlyLinked(StringRef path) { void loadReexport(StringRef path, DylibFile *umbrella) { Optional reexport = loadReexportHelper(path, umbrella); if (reexport && isImplicitlyLinked(path)) - inputFiles.push_back(*reexport); + inputFiles.insert(*reexport); } DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella) @@ -670,7 +670,7 @@ void ArchiveFile::fetch(const object::Archive::Symbol &sym) { " has unhandled file type"); return; } - inputFiles.push_back(file); + inputFiles.insert(file); // ld64 doesn't demangle sym here even with -demangle. Match that, so // intentionally no call to toMachOString() here. diff --git a/lld/MachO/InputFiles.h b/lld/MachO/InputFiles.h index e5a1566dda202e..e52905e75d5674 100644 --- a/lld/MachO/InputFiles.h +++ b/lld/MachO/InputFiles.h @@ -14,6 +14,7 @@ #include "lld/Common/LLVM.h" #include "lld/Common/Memory.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SetVector.h" #include "llvm/BinaryFormat/MachO.h" #include "llvm/DebugInfo/DWARF/DWARFUnit.h" #include "llvm/Object/Archive.h" @@ -158,7 +159,7 @@ class BitcodeFile : public InputFile { std::unique_ptr obj; }; -extern std::vector inputFiles; +extern llvm::SetVector inputFiles; llvm::Optional readFile(StringRef path); diff --git a/lld/test/MachO/invalid/duplicate-symbol.s b/lld/test/MachO/invalid/duplicate-symbol.s index ce662b6141d0f7..392630a7f07566 100644 --- a/lld/test/MachO/invalid/duplicate-symbol.s +++ b/lld/test/MachO/invalid/duplicate-symbol.s @@ -2,6 +2,7 @@ # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t-dup.o # RUN: not %lld -o /dev/null %t-dup.o %t.o 2>&1 | FileCheck %s +# RUN: not %lld -o /dev/null %t.o %t.o 2>&1 | FileCheck %s # CHECK: error: duplicate symbol: _main diff --git a/lld/test/MachO/weak-import.s b/lld/test/MachO/weak-import.s index 81e0a9e7e3ecda..51736edd54998a 100644 --- a/lld/test/MachO/weak-import.s +++ b/lld/test/MachO/weak-import.s @@ -6,6 +6,10 @@ # RUN: %lld -weak-lSystem %t/test.o -weak_framework CoreFoundation -weak_library %t/libfoo.dylib -o %t/test # RUN: llvm-objdump --macho --all-headers %t/test | FileCheck %s -DDIR=%t +# RUN: %lld -weak-lSystem %t/test.o \ +# RUN: -framework CoreFoundation -weak_framework CoreFoundation -framework CoreFoundation \ +# RUN: %t/libfoo.dylib -weak_library %t/libfoo.dylib %t/libfoo.dylib -o %t/test +# RUN: llvm-objdump --macho --all-headers %t/test | FileCheck %s -DDIR=%t # CHECK: cmd LC_LOAD_WEAK_DYLIB # CHECK-NEXT: cmdsize