[lld-macho] Disambiguate bitcode files with the same name by archive name/offset in archive

[lgrey]

Summary

Ported from COFF/ELF; test is adapted from test/COFF/thinlto-archivecollision.ll

LTO expects every bitcode file to have a unique name. If given multiple bitcode files with the same name, it errors with "Expected at most one ThinLTO module per
bitcode file".

This change incorporates the archive name, to disambiguate members with the same name in different archives and the offset in archive to disambiguate members with the same name in the same archive.

[int3]

clearing my queue

[lgrey]

Add test. Now ready for review :)

[thakis]

Generally lg, but I'm confused by the comment (I think I unconfused myself though), and some nits about the test. Thanks!

lld/MachO/InputFiles.cpp

1267

The code adds the archiveName now, which should disambiguate two archives having the same name. Why do we need the offset too?

Looks like the offset used to be added only if archiveName is empty and D75422 changed that.

But when can the archive name be empty?

...aha, D60549 that added this to the COFF port has two obj files with the same name in a single archive. So for that including the offset makes sense. Oh, you have that test too :) So it's just the comment that's confusing. It should say "If one archive defines two members with the same name", right?

lld/test/MachO/lto-archivecollision.ll

4

.obj is a windows name. use .o.

8

nit: one space is enough

12

No need to do this again, %t/a/bar.o and %t/b/bar.o are still around from the previous test

20

Do you even need this? If the link succeeded, it means we pulled in both .o files

[lgrey]

Reword comment, refine test

lld/MachO/InputFiles.cpp

1267

It's both (when I ran into this with Chrome, it was two different archives). Rejiggered the comment a little (part about collision is no longer true since it will just error inside LTO); does it make more sense now?

lld/test/MachO/lto-archivecollision.ll

20

Agree, but this applies to all the FileCheck stuff in here. Do you think it's worth keeping as documentation, or should I just run the links, add a comment re: how they would fail, and call it a day?

[thakis]

lld/MachO/InputFiles.cpp

1263

It doesn't have to be a thin archive, regular archives can contain several files with the same name:

% echo 'void a() {}' > a.cc
% echo 'void b() {}' > b.cc
% mkdir a
% mkdir b
% clang -flto=thin -c a.cc -o a/a.o
% clang -flto=thin -c b.cc -o b/a.o
% ar -t libfoo.a
__.SYMDEF
a.o
a.o

lld/test/MachO/lto-archivecollision.ll

20

I'd keep comments as is and just remove the FIXME line, but /shruggie

[lgrey]

Reword comment, rm FIXME from test

lld/MachO/InputFiles.cpp

1263

Reworded

[lgrey]

Typo fix

[thakis]

int3, could you retract your "needs changes"? Else phab thinks this isn't accepted.

[thakis]

…and lgtm :)

[int3]

ah yes, I forgot the external phabricator works differently. done!

lld/MachO/Driver.h

findPathCombination(const llvm::Twine &name,
                     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);
+                                              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);
 

lld/MachO/Driver.cpp

static llvm::CachePruningPolicy getLTOCachePolicy(InputArgList &args) {
   }
   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),
MemoryBufferRef mbref =
               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});
+    v.push_back({mbref, modTime, c.getChildOffset()});
   }
   if (err)
     fatal(mb.getBufferIdentifier() +
           ": Archive::children failed: " + toString(std::move(err)));
 
   return v;
 }
 
case file_magic::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)) {
           if (Optional<InputFile *> file = loadArchiveMember(
-                  member.mbref, member.modTime, path, /*objCOnly=*/false)) {
+                  member.mbref, member.modTime, path, /*objCOnly=*/false,
+                  member.offsetInArchive)) {
             inputFiles.insert(*file);
             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(), /*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).
       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.mbref, member.modTime, path, /*objCOnly=*/true,
+                  member.offsetInArchive)) {
             inputFiles.insert(*file);
             printArchiveMemberLoad("-ObjC", inputFiles.back());
           }
         }
       }
     }
 
     newFile = loadedArchives[path] = 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:
   case file_magic::tapi_file:
     if (DylibFile *dylibFile = loadDylib(mbref)) {
       if (isExplicit)
         dylibFile->explicitlyLinked = true;
       newFile = dylibFile;
     }
     break;
   case file_magic::bitcode:
-    newFile = make<BitcodeFile>(mbref);
+    newFile = make<BitcodeFile>(mbref, "", 0);
     break;
   case file_magic::macho_executable:
   case file_magic::macho_bundle:
     // We only allow executable and bundle type here if it is used
     // as a bundle loader.
     if (!isBundleLoader)
       error(path + ": unhandled file type");
     if (DylibFile *dylibFile = loadDylib(mbref, nullptr, isBundleLoader))

lld/MachO/DriverUtils.cpp

if (Optional<StringRef> rerootedPath =
     return *rerootedPath;
 
   return path;
 }
 
 Optional<InputFile *> macho::loadArchiveMember(MemoryBufferRef mb,
                                                uint32_t modTime,
                                                StringRef archiveName,
-                                               bool objCOnly) {
+                                               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);
+      return make<BitcodeFile>(mb, archiveName, offsetInArchive);
     return None;
   default:
     error(archiveName + ": archive member " + mb.getBufferIdentifier() +
           " has unhandled file type");
     return None;
   }
 }
 

lld/MachO/InputFiles.h

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);
+  explicit BitcodeFile(MemoryBufferRef mb, StringRef archiveName,
+                       uint64_t offsetInArchive);
   static bool classof(const InputFile *f) { return f->kind() == BitcodeKind; }
 
   std::unique_ptr<llvm::lto::InputFile> obj;
 };
 
 extern llvm::SetVector<InputFile *> inputFiles;
 
 llvm::Optional<MemoryBufferRef> readFile(StringRef path);

lld/MachO/InputFiles.cpp

uint32_t modTime = toTimeT(
                                      "for 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)) {
+  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);
   }
 }
 
 static macho::Symbol *createBitcodeSymbol(const lto::InputFile::Symbol &objSym,
static macho::Symbol *createBitcodeSymbol(const lto::InputFile::Symbol &objSym,
 
   return symtab->addDefined(name, &file, /*isec=*/nullptr, /*value=*/0,
                             /*size=*/0, objSym.isWeak(), isPrivateExtern,
                             /*isThumb=*/false,
                             /*isReferencedDynamically=*/false,
                             /*noDeadStrip=*/false);
 }
 
-BitcodeFile::BitcodeFile(MemoryBufferRef mbref)
-    : InputFile(BitcodeKind, mbref) {
+BitcodeFile::BitcodeFile(MemoryBufferRef mb, StringRef archiveName,
+                         uint64_t offsetInArchive)
+    : InputFile(BitcodeKind, mb) {
+  std::string path = mb.getBufferIdentifbier().str();
+  // ThinLTO assumes that all MemoryBufferRefs given to it have a unique
+  // name. If two members with the same name are provided, this causes a
+  // collision and ThinLTO can't proceed.
+  // So, we append the archive name to disambiguate two members with the same
+  // name from multiple different archives, and offset within the archive to
+  // disambiguate two members of the same name from a single archive.
+  MemoryBufferRef mbref(
+      mb.getBuffer(),
+      saver.save(archiveName.empty() ? path
+                                     : archiveName + sys::path::filename(path) +
+                                           utostr(offsetInArchive)));
+
   obj = check(lto::InputFile::create(mbref));
 
   // Convert LTO Symbols to LLD Symbols in order to perform resolution. The
   // "winning" symbol will then be marked as Prevailing at LTO compilation
   // time.
   for (const lto::InputFile::Symbol &objSym : obj->symbols())
     symbols.push_back(createBitcodeSymbol(objSym, *this));
 }
 
 template void ObjFile::parse<LP64>();

lld/test/MachO/lto-archivecollision.ll

+; RUN: rm -rf %t; split-file %s %t
+; RUN: mkdir %t/a %t/b
+; RUN: opt -thinlto-bc -o %t/main.o %t/main.ll
+; RUN: opt -thinlto-bc -o %t/a/bar.o %t/foo.ll
+; RUN: opt -thinlto-bc -o %t/b/bar.o %t/bar.ll
+; RUN: llvm-ar crs %t/libbar.a %t/a/bar.o %t/b/bar.o
+; RUN: %lld -save-temps %t/main.o %t/libbar.a -o %t/test
+; RUN: FileCheck %s --check-prefix=SAME-ARCHIVE < %t/test.resolution.txt
+
+; RUN: llvm-ar crs %t/liba.a %t/a/bar.o
+; RUN: llvm-ar crs %t/libb.a %t/b/bar.o
+; RUN: %lld -save-temps %t/main.o %t/liba.a %t/libb.a -o %t/test
+; RUN: FileCheck %s --check-prefix=DIFFERENT-ARCHIVES < %t/test.resolution.txt
+
+; SAME-ARCHIVE: libbar.abar.o[[#OFFSET:]]
+; SAME-ARCHIVE-NEXT: -r={{.*}}/libbar.abar.o[[#OFFSET:]],_foo,p
+; SAME-ARCHIVE-NEXT: libbar.abar.o[[#OTHEROFFSET:]]
+; SAME-ARCHIVE-NEXT: -r={{.*}}/libbar.abar.o[[#OTHEROFFSET:]],_bar,p
+
+; DIFFERENT-ARCHIVES: liba.abar.o[[#OFFSET:]]
+; DIFFERENT-ARCHIVES-NEXT: -r={{.*}}/liba.abar.o[[#OFFSET:]],_foo,p
+; DIFFERENT-ARCHIVES-NEXT: libb.abar.o[[#OTHEROFFSET:]]
+; DIFFERENT-ARCHIVES-NEXT: -r={{.*}}/libb.abar.o[[#OTHEROFFSET:]],_bar,p
+
+;--- main.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"
+
+declare void @bar()
+declare void @foo()
+
+define i32 @main() {
+  call void @foo()
+  call void @bar()
+  ret i32 0
+}
+
+;--- foo.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"
+
+define void @foo() {
+  ret void
+}
+
+;--- bar.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"
+
+define void @bar() {
+  ret void
+}