[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

@@ -274,8 +274,7 @@ static InputFile *addFile(StringRef path, bool forceLoadArchive) {
     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());
@@ -293,7 +292,7 @@ static InputFile *addFile(StringRef path, bool forceLoadArchive) {
       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());
           }
@@ -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<const char *> argsArr, bool canExitEarly,
     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.

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<std::string> macho::resolveDylibPath(StringRef path) {
   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);
-
-  assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
-         magic == file_magic::macho_dynamically_linked_shared_lib_stub);
-  return make<DylibFile>(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);
+    file = make<DylibFile>(mbref, umbrella);
+  }
+  return file;
 }
 
 uint32_t macho::getModTime(StringRef path) {

lld/MachO/InputFiles.cpp

@@ -85,7 +85,7 @@ std::string lld::toString(const InputFile *f) {
       .str();
 }
 
-std::vector<InputFile *> macho::inputFiles;
+SetVector<InputFile *> macho::inputFiles;
 std::unique_ptr<TarWriter> macho::tar;
 int InputFile::idCount = 0;
 
@@ -516,7 +516,7 @@ static bool isImplicitlyLinked(StringRef path) {
 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)
@@ -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.

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<llvm::lto::InputFile> obj;
 };
 
-extern std::vector<InputFile *> inputFiles;
+extern llvm::SetVector<InputFile *> inputFiles;
 
 llvm::Optional<MemoryBufferRef> readFile(StringRef path);
 

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
 

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