[lld-macho] Refactor archive loading

[int3]

Summary

The previous logic was duplicated between symbol-initiated
archive loads versus flag-initiated loads (i.e. -force_load and
-ObjC). This resulted in code duplication as well as redundant work --
we would create Archive instances twice whenever we had one of those
flags; once in getArchiveMembers and again when we constructed the
ArchiveFile.

This was motivated by an upcoming diff where we load archive members
containing ObjC-related symbols before loading those containing
ObjC-related sections, as well as before performing symbol resolution.
Without this refactor, it would be difficult to do that while avoiding
loading the same archive member twice.

[oontvoo]

LGTM thanks!!!

lld/MachO/Driver.cpp

255

clang-tidy: use auto *file pls.

261–267

We've already done error() on line 262, why do we need to do it again on 266?
(Wouldn't this error on the last error twice?)

283–289

same question here w.r.t erroring twice.

(also the code looks alsmot identical - maybe future clean up could factor them out somehow)

lld/MachO/InputFiles.cpp

1257–1258

could you include the unhandled type in the err msg? (makes it easier to debug when we do get this error)

lld/MachO/InputFiles.h

194

Why not? Just curious

197

Does this want to return a const ref instead?

[int3]

lld/MachO/Driver.cpp

261–267

They're different errors -- one error is from fetching, the other one from trying to get the children

283–289

I guess we could have a helper function that just extracts the children and prints the error, but in general I think this code is ugly because the underlying Archive API is ugly

lld/MachO/InputFiles.h

194

we usually just dump the error to stderr directly via error() or fatal(), so there's no need to return it to the caller

197

yeah we can do that

[oontvoo]

LGTM

lld/MachO/Driver.cpp

261–267

D'oh - different errors, same name. Sorry missed it!

[thevinster]

lld/MachO/InputFiles.cpp

1303

nit: I'm not sure what the standard practice is, but for readability, I feel like it would be more helpful to have this error message be different than L1292. This makes it easier to debug since toMachOString can contain hidden a :.

lld/MachO/Driver.h

 findPathCombination(const llvm::Twine &name,
                     const std::vector<llvm::StringRef> &roots,
                     ArrayRef<llvm::StringRef> extensions = {""});
 
 // If -syslibroot is specified, absolute paths to non-object files may be
 // rerooted.
 llvm::StringRef rerootPath(llvm::StringRef path);
 
-llvm::Optional<InputFile *> loadArchiveMember(MemoryBufferRef, uint32_t modTime,
-                                              StringRef archiveName,
-                                              bool objCOnly,
-                                              uint64_t offsetInArchive);
-
 uint32_t getModTime(llvm::StringRef path);
 
 void printArchiveMemberLoad(StringRef reason, const InputFile *);
 
 // Map simulator platforms to their underlying device platform.
 llvm::MachO::PlatformKind removeSimulator(llvm::MachO::PlatformKind platform);
 
 // Helper class to export dependency info.

lld/MachO/Driver.cpp

for (const Arg *arg :
     case OPT_max_relative_cache_size_lto:
       add(Twine("cache_size=") + arg->getValue() + "%");
       break;
     }
   }
   return CHECK(parseCachePruningPolicy(ltoPolicy), "invalid LTO cache policy");
 }
 
-namespace {
-struct ArchiveMember {
-  MemoryBufferRef mbref;
-  uint32_t modTime;
-  uint64_t offsetInArchive;
-};
-} // namespace
-
-// Returns slices of MB by parsing MB as an archive file.
-// Each slice consists of a member file in the archive.
-static std::vector<ArchiveMember> getArchiveMembers(MemoryBufferRef mb) {
-  std::unique_ptr<Archive> file =
-      CHECK(Archive::create(mb),
-            mb.getBufferIdentifier() + ": failed to parse archive");
-  Archive *archive = file.get();
-  make<std::unique_ptr<Archive>>(std::move(file)); // take ownership
-
-  std::vector<ArchiveMember> v;
-  Error err = Error::success();
-
-  // Thin archives refer to .o files, so --reproduce needs the .o files too.
-  bool addToTar = archive->isThin() && tar;
-
-  for (const Archive::Child &c : archive->children(err)) {
-    MemoryBufferRef mbref =
-        CHECK(c.getMemoryBufferRef(),
-              mb.getBufferIdentifier() +
-                  ": could not get the buffer for a child of the archive");
-    if (addToTar)
-      tar->append(relativeToRoot(check(c.getFullName())), mbref.getBuffer());
-    uint32_t modTime = toTimeT(
-        CHECK(c.getLastModified(), mb.getBufferIdentifier() +
-                                       ": could not get the modification "
-                                       "time for a child of the archive"));
-    v.push_back({mbref, modTime, c.getChildOffset()});
-  }
-  if (err)
-    fatal(mb.getBufferIdentifier() +
-          ": Archive::children failed: " + toString(std::move(err)));
-
-  return v;
-}
-
 static DenseMap<StringRef, ArchiveFile *> loadedArchives;
 
 static InputFile *addFile(StringRef path, bool forceLoadArchive,
                           bool isExplicit = true, bool isBundleLoader = false) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return nullptr;
   MemoryBufferRef mbref = *buffer;
   InputFile *newFile = nullptr;
 
   file_magic magic = identify_magic(mbref.getBuffer());
   switch (magic) {
   case file_magic::archive: {
     // Avoid loading archives twice. If the archives are being force-loaded,
     // loading them twice would create duplicate symbol errors. In the
     // non-force-loading case, this is just a minor performance optimization.
     // We don't take a reference to cachedFile here because the
     // loadArchiveMember() call below may recursively call addFile() and
     // invalidate this reference.
     if (ArchiveFile *cachedFile = loadedArchives[path])
       return cachedFile;
 
-    std::unique_ptr<object::Archive> file = CHECK(
+    std::unique_ptr<object::Archive> archive = CHECK(
         object::Archive::create(mbref), path + ": failed to parse archive");
 
-    if (!file->isEmpty() && !file->hasSymbolTable())
+    if (!archive->isEmpty() && !archive->hasSymbolTable())
       error(path + ": archive has no index; run ranlib to add one");
 
+    auto *file = make<ArchiveFile>(std::move(archive));
     if (config->allLoad || forceLoadArchive) {
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
-        for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
-          if (Optional<InputFile *> file = loadArchiveMember(
-                  member.mbref, member.modTime, path, /*objCOnly=*/false,
-                  member.offsetInArchive)) {
-            inputFiles.insert(*file);
-            printArchiveMemberLoad(
-                (forceLoadArchive ? "-force_load" : "-all_load"),
-                inputFiles.back());
-          }
-        }
+        Error e = Error::success();
+        for (const object::Archive::Child &c : file->getArchive().children(e)) {
+          StringRef reason = forceLoadArchive ? "-force_load" : "-all_load";
+          if (Error e = file->fetch(c, reason))
+            error(toString(file) + ": " + reason +
+                  " failed to load archive member: " + toString(std::move(e)));
+        }
+        if (e)
+          error(toString(file) +
+                ": Archive::children failed: " + toString(std::move(e)));
       }
     } else if (config->forceLoadObjC) {
-      for (const object::Archive::Symbol &sym : file->symbols())
+      for (const object::Archive::Symbol &sym : file->getArchive().symbols())
         if (sym.getName().startswith(objc::klass))
           symtab->addUndefined(sym.getName(), /*file=*/nullptr,
                                /*isWeakRef=*/false);
 
       // 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).
+      // we already found that it contains an ObjC symbol.
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
-        for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
-          if (Optional<InputFile *> file = loadArchiveMember(
-                  member.mbref, member.modTime, path, /*objCOnly=*/true,
-                  member.offsetInArchive)) {
-            inputFiles.insert(*file);
-            printArchiveMemberLoad("-ObjC", inputFiles.back());
-          }
+        Error e = Error::success();
+        for (const object::Archive::Child &c : file->getArchive().children(e)) {
+          Expected<MemoryBufferRef> mb = c.getMemoryBufferRef();
+          if (!mb || !hasObjCSection(*mb))
+            continue;
+          if (Error e = file->fetch(c, "-ObjC"))
+            error(toString(file) + ": -ObjC failed to load archive member: " +
+                  toString(std::move(e)));
         }
+        if (e)
+          error(toString(file) +
+                ": Archive::children failed: " + toString(std::move(e)));
       }
     }
 
-    newFile = loadedArchives[path] = make<ArchiveFile>(std::move(file));
+    file->addLazySymbols();
+    newFile = loadedArchives[path] = 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:
   case file_magic::tapi_file:

lld/MachO/DriverUtils.cpp

 #include "Target.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/DenseMap.h"
-#include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/TextAPI/InterfaceFile.h"
StringRef macho::rerootPath(StringRef path) {
 
   if (Optional<StringRef> rerootedPath =
           findPathCombination(path, config->systemLibraryRoots))
     return *rerootedPath;
 
   return path;
 }
 
-Optional<InputFile *> macho::loadArchiveMember(MemoryBufferRef mb,
-                                               uint32_t modTime,
-                                               StringRef archiveName,
-                                               bool objCOnly,
-                                               uint64_t offsetInArchive) {
-  if (config->zeroModTime)
-    modTime = 0;
-
-  switch (identify_magic(mb.getBuffer())) {
-  case file_magic::macho_object:
-    if (!objCOnly || hasObjCSection(mb))
-      return make<ObjFile>(mb, modTime, archiveName);
-    return None;
-  case file_magic::bitcode:
-    if (!objCOnly || check(isBitcodeContainingObjCCategory(mb)))
-      return make<BitcodeFile>(mb, archiveName, offsetInArchive);
-    return None;
-  default:
-    error(archiveName + ": archive member " + mb.getBufferIdentifier() +
-          " has unhandled file type");
-    return None;
-  }
-}
-
 uint32_t macho::getModTime(StringRef path) {
   if (config->zeroModTime)
     return 0;
 
   fs::file_status stat;
   if (!fs::status(path, stat))
     if (fs::exists(stat))
       return toTimeT(stat.getLastModificationTime());

lld/MachO/InputFiles.h

private:
   void handleLDInstallNameSymbol(StringRef name, StringRef originalName);
   void checkAppExtensionSafety(bool dylibIsAppExtensionSafe) const;
 };
 
 // .a file
 class ArchiveFile final : public InputFile {
 public:
   explicit ArchiveFile(std::unique_ptr<llvm::object::Archive> &&file);
+  void addLazySymbols();
+  void fetch(const llvm::object::Archive::Symbol &);
+  // LLD normally doesn't use Error for error-handling, but the underlying
+  // Archive library does, so this is the cleanest way to wrap it.
+  Error fetch(const llvm::object::Archive::Child &, StringRef reason);
+  const llvm::object::Archive &getArchive() const { return *file; };
   static bool classof(const InputFile *f) { return f->kind() == ArchiveKind; }
-  void fetch(const llvm::object::Archive::Symbol &sym);
 
 private:
   std::unique_ptr<llvm::object::Archive> file;
   // Keep track of children fetched from the archive by tracking
   // which address offsets have been fetched already.
   llvm::DenseSet<uint64_t> seen;
 };
 
 class BitcodeFile final : public InputFile {
 public:
   explicit BitcodeFile(MemoryBufferRef mb, StringRef archiveName,
                        uint64_t offsetInArchive);

lld/MachO/InputFiles.cpp

 }
 
 void DylibFile::checkAppExtensionSafety(bool dylibIsAppExtensionSafe) const {
   if (config->applicationExtension && !dylibIsAppExtensionSafe)
     warn("using '-application_extension' with unsafe dylib: " + toString(this));
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<object::Archive> &&f)
-    : InputFile(ArchiveKind, f->getMemoryBufferRef()), file(std::move(f)) {
+    : InputFile(ArchiveKind, f->getMemoryBufferRef()), file(std::move(f)) {}
+
+void ArchiveFile::addLazySymbols() {
   for (const object::Archive::Symbol &sym : file->symbols())
     symtab->addLazy(sym.getName(), this, sym);
 }
 
-void ArchiveFile::fetch(const object::Archive::Symbol &sym) {
-  object::Archive::Child c =
-      CHECK(sym.getMember(), toString(this) +
-                                 ": could not get the member for symbol " +
-                                 toMachOString(sym));
+static Expected<InputFile *> loadArchiveMember(MemoryBufferRef mb,
+                                               uint32_t modTime,
+                                               StringRef archiveName,
+                                               uint64_t offsetInArchive) {
+  if (config->zeroModTime)
+    modTime = 0;
+
+  switch (identify_magic(mb.getBuffer())) {
+  case file_magic::macho_object:
+    return make<ObjFile>(mb, modTime, archiveName);
+  case file_magic::bitcode:
+    return make<BitcodeFile>(mb, archiveName, offsetInArchive);
+  default:
+    return createStringError(inconvertibleErrorCode(),
+                             mb.getBufferIdentifier() +
+                                 " has unhandled file type");
+  }
+}
 
+Error ArchiveFile::fetch(const object::Archive::Child &c, StringRef reason) {
   if (!seen.insert(c.getChildOffset()).second)
-    return;
+    return Error::success();
 
-  MemoryBufferRef mb =
-      CHECK(c.getMemoryBufferRef(),
-            toString(this) +
-                ": could not get the buffer for the member defining symbol " +
-                toMachOString(sym));
+  Expected<MemoryBufferRef> mb = c.getMemoryBufferRef();
+  if (!mb)
+    return mb.takeError();
 
+  // Thin archives refer to .o files, so --reproduce needs the .o files too.
   if (tar && c.getParent()->isThin())
-    tar->append(relativeToRoot(CHECK(c.getFullName(), this)), mb.getBuffer());
+    tar->append(relativeToRoot(CHECK(c.getFullName(), this)), mb->getBuffer());
+
+  Expected<TimePoint<std::chrono::seconds>> modTime = c.getLastModified();
+  if (!modTime)
+    return modTime.takeError();
 
-  uint32_t modTime = toTimeT(
-      CHECK(c.getLastModified(), toString(this) +
-                                     ": could not get the modification time "
-                                     "for the member defining symbol " +
-                                     toMachOString(sym)));
+  Expected<InputFile *> file =
+      loadArchiveMember(*mb, toTimeT(*modTime), getName(), c.getChildOffset());
+
+  if (!file)
+    return file.takeError();
+
+  inputFiles.insert(*file);
+  printArchiveMemberLoad(reason, *file);
+  return Error::success();
+}
+
+void ArchiveFile::fetch(const object::Archive::Symbol &sym) {
+  object::Archive::Child c =
+      CHECK(sym.getMember(), toString(this) +
+                                 ": could not get the member defining symbol " +
+                                 toMachOString(sym));
 
   // `sym` is owned by a LazySym, which will be replace<>()d by make<ObjFile>
   // and become invalid after that call. Copy it to the stack so we can refer
   // to it later.
   const object::Archive::Symbol symCopy = sym;
 
-  if (Optional<InputFile *> file = loadArchiveMember(
-          mb, modTime, getName(), /*objCOnly=*/false, c.getChildOffset())) {
-    inputFiles.insert(*file);
-    // ld64 doesn't demangle sym here even with -demangle.
-    // Match that: intentionally don't call toMachOString().
-    printArchiveMemberLoad(symCopy.getName(), *file);
-  }
+  if (Error e = fetch(c, symCopy.getName()))
+    error(toString(this) + ": could not get the member defining symbol " +
+          toMachOString(symCopy) + ": " + toString(std::move(e)));
 }
 
 static macho::Symbol *createBitcodeSymbol(const lto::InputFile::Symbol &objSym,
                                           BitcodeFile &file) {
   StringRef name = saver.save(objSym.getName());
 
   // TODO: support weak references
   if (objSym.isUndefined())

lld/MachO/ObjC.cpp

 //===- ObjC.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 "ObjC.h"
 #include "InputFiles.h"
 #include "InputSection.h"
 #include "OutputSegment.h"
 #include "Target.h"
 
 #include "llvm/BinaryFormat/MachO.h"
+#include "llvm/Bitcode/BitcodeReader.h"
 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace lld;
 using namespace lld::macho;
 
-template <class LP> static bool hasObjCSection(MemoryBufferRef mb) {
+template <class LP> static bool objectHasObjCSection(MemoryBufferRef mb) {
   using Section = typename LP::section;
 
   auto *hdr =
       reinterpret_cast<const typename LP::mach_header *>(mb.getBufferStart());
   if (hdr->magic != LP::magic)
     return false;
 
   if (const auto *c =
for (const Section &sec : sectionHeaders) {
            sectname == section_names::swift)) {
         return true;
       }
     }
   }
   return false;
 }
 
-bool macho::hasObjCSection(MemoryBufferRef mb) {
+static bool objectHasObjCSection(MemoryBufferRef mb) {
   if (target->wordSize == 8)
-    return ::hasObjCSection<LP64>(mb);
+    return ::objectHasObjCSection<LP64>(mb);
   else
-    return ::hasObjCSection<ILP32>(mb);
+    return ::objectHasObjCSection<ILP32>(mb);
+}
+
+bool macho::hasObjCSection(MemoryBufferRef mb) {
+  switch (identify_magic(mb.getBuffer())) {
+  case file_magic::macho_object:
+    return objectHasObjCSection(mb);
+  case file_magic::bitcode:
+    return check(isBitcodeContainingObjCCategory(mb));
+  default:
+    return false;
+  }
 }

lld/test/MachO/invalid/bad-archive-member.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
 # RUN: %lld -dylib -lSystem %t/foo.o -o %t/foo.dylib
 # RUN: llvm-ar rcs %t/foo.a %t/foo.dylib
-# RUN: not %lld %t/test.o %t/foo.a -o /dev/null 2>&1 | FileCheck %s -DFILE=%t/foo.a
-# RUN: not %lld %t/test.o -force_load %t/foo.a -o /dev/null 2>&1 | FileCheck %s -DFILE=%t/foo.a
-# RUN: not %lld %t/test.o -ObjC %t/foo.a -o /dev/null 2>&1 | FileCheck %s -DFILE=%t/foo.a
-# CHECK: error: [[FILE]]: archive member foo.dylib has unhandled file type
+# RUN: not %lld %t/test.o %t/foo.a -o /dev/null 2>&1 | FileCheck %s \
+# RUN:   --check-prefix=SYM -DFILE=%t/foo.a
+# RUN: not %lld %t/test.o -ObjC %t/foo.a -o /dev/null 2>&1 | FileCheck %s \
+# RUN:   --check-prefix=SYM -DFILE=%t/foo.a
+# RUN: not %lld %t/test.o -force_load %t/foo.a -o /dev/null 2>&1 | FileCheck %s \
+# RUN:   --check-prefix=FORCE-LOAD -DFILE=%t/foo.a
+# SYM: error: [[FILE]]: could not get the member defining symbol _foo: foo.dylib has unhandled file type
+# FORCE-LOAD: error: [[FILE]]: -force_load failed to load archive member: foo.dylib has unhandled file type
 
 #--- foo.s
 .globl _foo
 _foo:
   ret
 
 #--- test.s
 .globl _main
 _main:
   callq _foo
   ret

lld/test/MachO/thin-archive.s

 # RUN: llvm-nm %t/out
 # RUN: not %lld %t/main.o %t/lib_thin.a -demangle -o %t/out 2>&1 | \
 # RUN:     FileCheck --check-prefix=NOOBJ %s
 # RUN: not %lld %t/main.o %t/lib_thin.a -o %t/out 2>&1 | \
 # RUN:     FileCheck --check-prefix=NOOBJNODEMANGLE %s
 
 # CHECK: __Z1fv
 # CHECK: _main
-# NOOBJ: error: {{.*}}lib_thin.a: could not get the buffer for the member defining symbol f(): '{{.*}}lib.o':
-# NOOBJNODEMANGLE: error: {{.*}}lib_thin.a: could not get the buffer for the member defining symbol __Z1fv: '{{.*}}lib.o':
+# NOOBJ: error: {{.*}}lib_thin.a: could not get the member defining symbol f(): '{{.*}}lib.o': {{[N|n]}}o such file or directory
+# NOOBJNODEMANGLE: error: {{.*}}lib_thin.a: could not get the member defining symbol __Z1fv: '{{.*}}lib.o': {{[N|n]}}o such file or directory
 
 #--- mangled-symbol.s
 .globl  __Z1fv
 __Z1fv:
   retq
 
 #--- main.s
 .global _main
 _main:
   callq __Z1fv
   mov $0, %rax
   retq