[lld-macho] -weak_{library,framework} should always take priority

[int3]

Summary

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.

[smeenai]

LGTM

lld/MachO/Driver.cpp

std::unique_ptr<object::Archive> file = CHECK(
         object::Archive::create(mbref), path + ": failed to parse archive");
 
     if (!file->isEmpty() && !file->hasSymbolTable())
       error(path + ": archive has no index; run ranlib to add one");
 
     if (config->allLoad || forceLoadArchive) {
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
         for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
-          inputFiles.push_back(
-              make<ObjFile>(member.mbref, member.modTime, path));
+          inputFiles.insert(make<ObjFile>(member.mbref, member.modTime, path));
           printArchiveMemberLoad(
               (forceLoadArchive ? "-force_load" : "-all_load"),
               inputFiles.back());
         }
       }
     } else if (config->forceLoadObjC) {
       for (const object::Archive::Symbol &sym : file->symbols())
         if (sym.getName().startswith(objc::klass))
           symtab->addUndefined(sym.getName());
 
       // TODO: no need to look for ObjC sections for a given archive member if
       // we already found that it contains an ObjC symbol. We should also
       // consider creating a LazyObjFile class in order to avoid double-loading
       // these files here and below (as part of the ArchiveFile).
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
         for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
           if (hasObjCSection(member.mbref)) {
-            inputFiles.push_back(
+            inputFiles.insert(
                 make<ObjFile>(member.mbref, member.modTime, path));
             printArchiveMemberLoad("-ObjC", inputFiles.back());
           }
         }
       }
     }
 
     newFile = make<ArchiveFile>(std::move(file));
static InputFile *addFile(StringRef path, bool forceLoadArchive) {
   default:
     error(path + ": unhandled file type");
   }
   if (newFile) {
     // printArchiveMemberLoad() prints both .a and .o names, so no need to
     // 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;
 }
 
 static void addLibrary(StringRef name, bool isWeak) {
   if (Optional<std::string> path = findLibrary(name)) {
     auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path, false));
     if (isWeak && dylibFile)

 
 static void compileBitcodeFiles() {
   auto lto = make<BitcodeCompiler>();
   for (InputFile *file : inputFiles)
     if (auto *bitcodeFile = dyn_cast<BitcodeFile>(file))
       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.
 // This function must be called after all symbol names are resolved (i.e. after
 // all InputFiles have been loaded.) As a result, later operations won't see
 // any CommonSymbols.
 static void replaceCommonSymbols() {
   for (macho::Symbol *sym : symtab->getSymbols()) {
bool macho::link(llvm::ArrayRef<const char *> argsArr, bool canExitEarly,
   symtab->addDSOHandle(in.header);
 
   for (opt::Arg *arg : args.filtered(OPT_sectcreate)) {
     StringRef segName = arg->getValue(0);
     StringRef sectName = arg->getValue(1);
     StringRef fileName = arg->getValue(2);
     Optional<MemoryBufferRef> buffer = readFile(fileName);
     if (buffer)
-      inputFiles.push_back(make<OpaqueFile>(*buffer, segName, sectName));
+      inputFiles.insert(make<OpaqueFile>(*buffer, segName, sectName));
   }
 
   // Initialize InputSections.
   for (InputFile *file : inputFiles) {
     for (SubsectionMap &map : file->subsections) {
       for (auto &p : map) {
         InputSection *isec = p.second;
         inputSections.push_back(isec);

lld/MachO/DriverUtils.cpp

 #include "Config.h"
 #include "InputFiles.h"
 
 #include "lld/Common/Args.h"
 #include "lld/Common/ErrorHandler.h"
 #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"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/TextAPI/MachO/TextAPIReader.h"
 
 using namespace llvm;
Optional<std::string> macho::resolveDylibPath(StringRef path) {
   SmallString<261> location = path;
   path::replace_extension(location, ".tbd");
   if (fs::exists(location))
     return std::string(location);
 
   return {};
 }
 
-static Optional<DylibFile *> makeDylibFromTapi(MemoryBufferRef mbref,
-                                               DylibFile *umbrella) {
-  Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
-  if (!result) {
-    error("could not load TAPI file at " + mbref.getBufferIdentifier() + ": " +
-          toString(result.takeError()));
-    return {};
-  }
-  return make<DylibFile>(**result, umbrella);
-}
-
-static DenseSet<CachedHashStringRef> 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<CachedHashStringRef, DylibFile *> loadedDylibs;
 
 Optional<DylibFile *> 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);
-
+  if (magic == file_magic::tapi_file) {
+    Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
+    if (!result) {
+      error("could not load TAPI file at " + mbref.getBufferIdentifier() +
+            ": " + toString(result.takeError()));
+      return {};
+    }
+    file = make<DylibFile>(**result, umbrella);
+  } else {
     assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
            magic == file_magic::macho_dynamically_linked_shared_lib_stub);
-  return make<DylibFile>(mbref, umbrella);
+    file = make<DylibFile>(mbref, umbrella);
+  }
+  return file;
 }
 
 uint32_t macho::getModTime(StringRef path) {
   fs::file_status stat;
   if (!fs::status(path, stat))
     if (fs::exists(stat))
       return toTimeT(stat.getLastModificationTime());
 

lld/MachO/InputFiles.h

 //===- InputFiles.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_MACHO_INPUT_FILES_H
 #define LLD_MACHO_INPUT_FILES_H
 
 #include "MachOStructs.h"
 
 #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"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/TextAPI/MachO/InterfaceFile.h"
 #include "llvm/TextAPI/MachO/TextAPIReader.h"
 
 #include <map>

 class BitcodeFile : public InputFile {
 public:
   explicit BitcodeFile(MemoryBufferRef mb);
   static bool classof(const InputFile *f) { return f->kind() == BitcodeKind; }
 
   std::unique_ptr<llvm::lto::InputFile> obj;
 };
 
-extern std::vector<InputFile *> inputFiles;
+extern llvm::SetVector<InputFile *> inputFiles;
 
 llvm::Optional<MemoryBufferRef> readFile(StringRef path);
 
 const llvm::MachO::load_command *
 findCommand(const llvm::MachO::mach_header_64 *, uint32_t type);
 
 } // namespace macho
 
 std::string toString(const macho::InputFile *file);
 } // namespace lld
 
 #endif

lld/MachO/InputFiles.cpp

if (!f)
     return "<internal>";
   if (f->archiveName.empty())
     return std::string(f->getName());
   return (path::filename(f->archiveName) + "(" + path::filename(f->getName()) +
           ")")
       .str();
 }
 
-std::vector<InputFile *> macho::inputFiles;
+SetVector<InputFile *> macho::inputFiles;
 std::unique_ptr<TarWriter> macho::tar;
 int InputFile::idCount = 0;
 
 // Open a given file path and return it as a memory-mapped file.
 Optional<MemoryBufferRef> macho::readFile(StringRef path) {
   // Open a file.
   auto mbOrErr = MemoryBuffer::getFile(path);
   if (auto ec = mbOrErr.getError()) {
static bool isImplicitlyLinked(StringRef path) {
   return path::parent_path(path) == "/usr/lib";
   // TODO: check for public frameworks too. We'll need to implement
   // -sub_umbrella first to write a test case.
 }
 
 void loadReexport(StringRef path, DylibFile *umbrella) {
   Optional<DylibFile *> reexport = loadReexportHelper(path, umbrella);
   if (reexport && isImplicitlyLinked(path))
-    inputFiles.push_back(*reexport);
+    inputFiles.insert(*reexport);
 }
 
 DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella)
     : InputFile(DylibKind, mb) {
   if (umbrella == nullptr)
     umbrella = this;
 
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
case file_magic::bitcode:
     break;
   default:
     StringRef bufname =
         CHECK(c.getName(), toString(this) + ": could not get buffer name");
     error(toString(this) + ": archive member " + bufname +
           " 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.
   printArchiveMemberLoad(sym_copy.getName(), file);
 }
 
 BitcodeFile::BitcodeFile(MemoryBufferRef mbref)
     : InputFile(BitcodeKind, mbref) {
   obj = check(lto::InputFile::create(mbref));
 }

lld/test/MachO/invalid/duplicate-symbol.s

 # REQUIRES: x86
 # 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
 
 .text
 .global _main
 _main:
   mov $0, %rax
   ret

lld/test/MachO/weak-import.s

 # REQUIRES: x86
 # RUN: split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
 # RUN: %lld -lSystem -dylib %t/foo.o -o %t/libfoo.dylib
 
 # 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
 # CHECK-NEXT:    name /usr/lib/libSystem.B.dylib
 
 # CHECK:          cmd LC_LOAD_WEAK_DYLIB
 # CHECK-NEXT: cmdsize
 # CHECK-NEXT:    name /System/Library/Frameworks/CoreFoundation.framework/CoreFoundation