[lld-macho] Don't load dylibs more than once

[int3]

Summary

Also remove DylibFile::reexported since it's unused.

Fixes llvm.org/PR48393.

[int3]

update

[thakis]

Nice! Fixes PR48393 :)

[thakis]

I guess you could add a test to test/MachO/lc-linker-command.s too. It'll pass, it uses the same codepath, but it's a different way to add -framework flags so maybe it's worth testing.

[smeenai]

lld/MachO/DriverUtils.cpp

182

StringSet copies all the strings you add to it. I don't think that's a huge deal for this use case, but just something to keep in mind if this ever starts showing up in profiles.

(A SmallSet of StringRef would avoid the copying, but you'd rehash all the strings whenever you grew the set. A SmallSet of CachedHashStringRef would avoid both the copying and the rehashing, but it feels kinda overkill?)

[int3]

test LC_LINKER_OPTION, use CachedHashStringRef

lld/MachO/DriverUtils.cpp

182

I don't know if the set will be small... libSystem alone re-exports 67 dylibs. But a DenseSet of CachedHashStringRef seems easy enough :)

[thakis]

Now that I look at PR48393 again, this gets -framework CoreFoundation -weak_framework CoreFoundation -framework CoreFoundation wrong: CoreFoundation should be linked weakly in that case, but with this patch I think it doesn't. (Sorry for only noticing after you committed.)

[int3]

Ah good catch. I'll put up a diff for that later

lld/MachO/Driver.h

 
 void parseLCLinkerOption(InputFile*, unsigned argc, StringRef data);
 
 std::string createResponseFile(const llvm::opt::InputArgList &args);
 
 // Check for both libfoo.dylib and libfoo.tbd (in that order).
 llvm::Optional<std::string> resolveDylibPath(llvm::StringRef path);
 
-llvm::Optional<DylibFile *> makeDylibFromTAPI(llvm::MemoryBufferRef mbref,
-                                              DylibFile *umbrella = nullptr);
+llvm::Optional<DylibFile *> loadDylib(llvm::MemoryBufferRef mbref,
 
 uint32_t getModTime(llvm::StringRef path);
 
 void printArchiveMemberLoad(StringRef reason, const InputFile *);
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/Driver.cpp

case file_magic::archive: {
     newFile = make<ArchiveFile>(std::move(file));
     break;
   }
   case file_magic::macho_object:
     newFile = make<ObjFile>(mbref, getModTime(path), "");
     break;
   case file_magic::macho_dynamically_linked_shared_lib:
   case file_magic::macho_dynamically_linked_shared_lib_stub:
-    newFile = make<DylibFile>(mbref);
-    break;
   case file_magic::tapi_file: {
-    if (Optional<DylibFile *> dylibFile = makeDylibFromTAPI(mbref))
+    if (Optional<DylibFile *> dylibFile = loadDylib(mbref))
       newFile = *dylibFile;
     break;
   }
   case file_magic::bitcode:
     newFile = make<BitcodeFile>(mbref);
     break;
   default:
     error(path + ": unhandled file type");

lld/MachO/DriverUtils.cpp

 //===- DriverUtils.cpp ----------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "Driver.h"
 #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/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 {};
 }
 
-Optional<DylibFile *> macho::makeDylibFromTAPI(MemoryBufferRef mbref,
+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;
+
+Optional<DylibFile *> macho::loadDylib(MemoryBufferRef mbref,
+                                       DylibFile *umbrella) {
+  StringRef path = mbref.getBufferIdentifier();
+  if (loadedDylibs.contains(CachedHashStringRef(path)))
+    return {};
+  loadedDylibs.insert(CachedHashStringRef(path));
+
+  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);
+}
+
 uint32_t macho::getModTime(StringRef path) {
   fs::file_status stat;
   if (!fs::status(path, stat))
     if (fs::exists(stat))
       return toTimeT(stat.getLastModificationTime());
 
   warn("failed to get modification time of " + path);
   return 0;
 }
 
 void macho::printArchiveMemberLoad(StringRef reason, const InputFile *f) {
   if (config->printEachFile)
     lld::outs() << toString(f) << '\n';
   if (config->printWhyLoad)
     lld::outs() << reason << " forced load of " << toString(f) << '\n';
 }

lld/MachO/InputFiles.h

explicit DylibFile(const llvm::MachO::InterfaceFile &interface,
                      DylibFile *umbrella = nullptr);
 
   static bool classof(const InputFile *f) { return f->kind() == DylibKind; }
 
   StringRef dylibName;
   uint64_t ordinal = 0; // Ordinal numbering starts from 1, so 0 is a sentinel
   bool reexport = false;
   bool forceWeakImport = false;
-  std::vector<DylibFile *> reexported;
 };
 
 // .a file
 class ArchiveFile : public InputFile {
 public:
   explicit ArchiveFile(std::unique_ptr<llvm::object::Archive> &&file);
   static bool classof(const InputFile *f) { return f->kind() == ArchiveKind; }
   void fetch(const llvm::object::Archive::Symbol &sym);

lld/MachO/InputFiles.cpp

 
 // The path can point to either a dylib or a .tbd file.
 static Optional<DylibFile *> loadDylib(StringRef path, DylibFile *umbrella) {
   Optional<MemoryBufferRef> mbref = readFile(path);
   if (!mbref) {
     error("could not read dylib file at " + path);
     return {};
   }
-
-  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);
-  return make<DylibFile>(*mbref, umbrella);
+  return loadDylib(*mbref, umbrella);
 }
 
 // TBD files are parsed into a series of TAPI documents (InterfaceFiles), with
 // the first document storing child pointers to the rest of them. When we are
 // processing a given TBD file, we store that top-level document here. When
 // processing re-exports, we search its children for potentially matching
 // documents in the same TBD file. Note that the children themselves don't
 // point to further documents, i.e. this is a two-level tree.
static bool isImplicitlyLinked(StringRef path) {
   if (!config->implicitDylibs)
     return false;
 
   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.
 }
 
-static Optional<DylibFile *> loadReexport(StringRef path, DylibFile *umbrella) {
+void loadReexport(StringRef path, DylibFile *umbrella) {
   Optional<DylibFile *> reexport = loadReexportHelper(path, umbrella);
   if (reexport && isImplicitlyLinked(path))
     inputFiles.push_back(*reexport);
-  return reexport;
 }
 
 DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella)
     : InputFile(DylibKind, mb) {
   if (umbrella == nullptr)
     umbrella = this;
 
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
     auto *cmd = reinterpret_cast<const load_command *>(p);
     p += cmd->cmdsize;
     if (cmd->cmd != LC_REEXPORT_DYLIB)
       continue;
 
     auto *c = reinterpret_cast<const dylib_command *>(cmd);
     StringRef reexportPath =
         reinterpret_cast<const char *>(c) + read32le(&c->dylib.name);
-    if (Optional<DylibFile *> reexport = loadReexport(reexportPath, umbrella))
-      reexported.push_back(*reexport);
+    loadReexport(reexportPath, umbrella);
   }
 }
 
 DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella)
     : InputFile(DylibKind, interface) {
   if (umbrella == nullptr)
     umbrella = this;
 
DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella)
 
   bool isTopLevelTapi = false;
   if (currentTopLevelTapi == nullptr) {
     currentTopLevelTapi = &interface;
     isTopLevelTapi = true;
   }
 
   for (InterfaceFileRef intfRef : interface.reexportedLibraries())
-    if (Optional<DylibFile *> reexport =
-            loadReexport(intfRef.getInstallName(), umbrella))
-      reexported.push_back(*reexport);
+    loadReexport(intfRef.getInstallName(), umbrella);
 
   if (isTopLevelTapi)
     currentTopLevelTapi = nullptr;
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<llvm::object::Archive> &&f)
     : InputFile(ArchiveKind, f->getMemoryBufferRef()), file(std::move(f)) {
   for (const object::Archive::Symbol &sym : file->symbols())

lld/test/MachO/dylink.s

 # CHECK-LABEL: Bind table:
 # CHECK-DAG: __DATA_CONST __got  0x{{0*}}[[#%x, HELLO_RIP + HELLO_OFF]]               pointer 0   libhello   _hello_world
 # CHECK-DAG: __DATA_CONST __got  0x{{0*}}[[#%x, HELLO_ITS_ME_RIP + HELLO_ITS_ME_OFF]] pointer 0   libhello   _hello_its_me
 # CHECK-DAG: __DATA_CONST __got  0x{{0*}}[[#%x, GOODBYE_RIP + GOODBYE_OFF]]           pointer 0   libgoodbye _goodbye_world
 # CHECK-DAG: __DATA       __data 0x[[#%x, DATA_ADDR:]]                                pointer 0   libhello   _hello_world
 # CHECK-DAG: __DATA       __data 0x{{0*}}[[#%x, DATA_ADDR + 8]]                       pointer 8   libhello   _hello_its_me
 # CHECK-DAG: __DATA       __data 0x{{0*}}[[#%x, DATA_ADDR + 16]]                      pointer -15 libgoodbye _goodbye_world
 
+# RUN: llvm-objdump --macho --all-headers %t/dylink | FileCheck %s \
+# RUN:   --check-prefix=LOAD --implicit-check-not LC_LOAD_DYLIB
+## Check that we don't create duplicate LC_LOAD_DYLIBs.
+# RUN: %lld -o %t/dylink -L%t -lhello -lhello -lgoodbye -lgoodbye %t/dylink.o
+# RUN: llvm-objdump --macho --all-headers %t/dylink | FileCheck %s \
+# RUN:   --check-prefix=LOAD --implicit-check-not LC_LOAD_DYLIB
+
+# LOAD:              cmd LC_LOAD_DYLIB
+# LOAD-NEXT:     cmdsize
+# LOAD-NEXT:        name @executable_path/libhello.dylib
+# LOAD:              cmd LC_LOAD_DYLIB
+# LOAD-NEXT:     cmdsize
+# LOAD-NEXT:        name @executable_path/libgoodbye.dylib
+
 .section __TEXT,__text
 .globl _main
 
 _main:
   movl $0x2000004, %eax # write() syscall
   mov $1, %rdi # stdout
   movq _hello_world@GOTPCREL(%rip), %rsi
   mov $13, %rdx # length of str

lld/test/MachO/implicit-dylibs.s

 # CHECK:     Bind table:
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter _foo
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 libtoplevel   _toplevel
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter _sublevel
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 libc++abi     ___gxx_personality_v0
 
 # RUN: llvm-objdump --macho --all-headers %t/test | FileCheck %s \
 # RUN:   --check-prefix=LOAD -DDIR=%t --implicit-check-not LC_LOAD_DYLIB
+## Check that we don't create duplicate LC_LOAD_DYLIBs.
+# RUN: %lld -syslibroot %t -o %t/test -lSystem -L%t -lreexporter -ltoplevel %t/test.o
+# RUN: llvm-objdump --macho --all-headers %t/test | FileCheck %s \
+# RUN:   --check-prefix=LOAD -DDIR=%t --implicit-check-not LC_LOAD_DYLIB
+
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT: cmdsize
 # LOAD-NEXT:    name /usr/lib/libSystem.B.dylib
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT: cmdsize
 # LOAD-NEXT:    name /usr/lib/libc++abi.dylib
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT: cmdsize

lld/test/MachO/lc-linker-option.ll

 # REQUIRES: x86
 # RUN: rm -rf %t
 # RUN: split-file %s %t
 
 # RUN: llvm-as %t/framework.ll -o %t/framework.o
 # RUN: %lld %t/framework.o -o %t/frame
-# RUN: llvm-objdump --macho --all-headers %t/frame | FileCheck --check-prefix=FRAME %s
+# RUN: llvm-objdump --macho --all-headers %t/frame | FileCheck --check-prefix=FRAME %s \
+# RUN:  --implicit-check-not LC_LOAD_DYLIB
 # FRAME:          cmd LC_LOAD_DYLIB
 # FRAME-NEXT: cmdsize
 # FRAME-NEXT:    name /System/Library/Frameworks/CoreFoundation.framework/CoreFoundation
 
 # RUN: llvm-as %t/l.ll -o %t/l.o
 # RUN: %lld %t/l.o -o %t/l
-# RUN: llvm-objdump --macho --all-headers %t/l | FileCheck --check-prefix=LIB %s
+# RUN: llvm-objdump --macho --all-headers %t/l | FileCheck --check-prefix=LIB %s \
+# RUN:  --implicit-check-not LC_LOAD_DYLIB
+
+## Check that we don't create duplicate LC_LOAD_DYLIBs.
+# RUN: %lld -lSystem %t/l.o -o %t/l
+# RUN: llvm-objdump --macho --all-headers %t/l | FileCheck --check-prefix=LIB %s \
+# RUN:  --implicit-check-not LC_LOAD_DYLIB
 # LIB:          cmd LC_LOAD_DYLIB
 # LIB-NEXT: cmdsize
 # LIB-NEXT:    name /usr/lib/libSystem.B.dylib
 
 # RUN: llvm-as %t/invalid.ll -o %t/invalid.o
 # RUN: not %lld %t/invalid.o -o /dev/null 2>&1 | FileCheck --check-prefix=INVALID %s
 # INVALID: error: -why_load is not allowed in LC_LINKER_OPTION
 
 #--- framework.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"CoreFoundation"}
 !llvm.linker.options = !{!0}
 
 define void @main() {
   ret void
 }
 
 #--- l.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-lSystem"}
-!llvm.linker.options = !{!0}
+!llvm.linker.options = !{!0, !0}
 
 define void @main() {
   ret void
 }
 
 #--- invalid.ll
 
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-why_load"}
 !llvm.linker.options = !{!0}
 
 define void @main() {
   ret void
 }