[LLD][COFF] use offset in archive to disambiguate archive members

[inglorion]

Summary

Archives can contain multiple members with the same name. This would
cause ThinLTO links to fail ("Expected at most one ThinLTO module per
bitcode file"). This change implements the same strategy we use in
the ELF linker: make the offset in the archive part of the module
name so that names are unique.

[ruiu]

lld/COFF/InputFiles.h

298 ↗

(On Diff #194617)

Do you still need this ctor?

[inglorion]

without second constructor

[inglorion]

lld/COFF/InputFiles.h

298 ↗

(On Diff #194617)

Alternatively, I could do without this constructor; there is just one call site where I would have to specify the parameters. I liked the constructor version a bit better because it doesn't end up with a bunch of parameters whose meaning is hard to deduce at the call site. I'm happy to go with whichever version you like better.

[ruiu]

LGTM

COFF/Driver.h

private:
   // choose the right one depending on which "main" function is defined.
   // This function looks up the symbol table and resolve corresponding
   // entry point name.
   StringRef findDefaultEntry();
   WindowsSubsystem inferSubsystem();
 
   void addBuffer(std::unique_ptr<MemoryBuffer> MB, bool WholeArchive);
   void addArchiveBuffer(MemoryBufferRef MBRef, StringRef SymName,
-                        StringRef ParentName);
+                        StringRef ParentName, uint64_t OffsetInArchive);
 
   void enqueuePath(StringRef Path, bool WholeArchive);
 
   void enqueueTask(std::function<void()> Task);
   bool run();
 
   std::list<std::function<void()>> TaskQueue;
   std::vector<StringRef> FilePaths;

COFF/Driver.cpp

case file_magic::windows_resource:
     Resources.push_back(MBRef);
     break;
   case file_magic::archive:
     if (WholeArchive) {
       std::unique_ptr<Archive> File =
           CHECK(Archive::create(MBRef), Filename + ": failed to parse archive");
 
       for (MemoryBufferRef M : getArchiveMembers(File.get()))
-        addArchiveBuffer(M, "<whole-archive>", Filename);
+        addArchiveBuffer(M, "<whole-archive>", Filename, 0);
       return;
     }
     Symtab->addFile(make<ArchiveFile>(MBRef));
     break;
   case file_magic::bitcode:
-    Symtab->addFile(make<BitcodeFile>(MBRef));
+    Symtab->addFile(make<BitcodeFile>(MBRef, "", 0));
     break;
   case file_magic::coff_object:
   case file_magic::coff_import_library:
     Symtab->addFile(make<ObjFile>(MBRef));
     break;
   case file_magic::coff_cl_gl_object:
     error(Filename + ": is not a native COFF file. Recompile without /GL");
     break;
enqueueTask([=]() {
     if (MBOrErr.second)
       error("could not open " + PathStr + ": " + MBOrErr.second.message());
     else
       Driver->addBuffer(std::move(MBOrErr.first), WholeArchive);
   });
 }
 
 void LinkerDriver::addArchiveBuffer(MemoryBufferRef MB, StringRef SymName,
-                                    StringRef ParentName) {
+                                    StringRef ParentName,
+                                    uint64_t OffsetInArchive) {
   file_magic Magic = identify_magic(MB.getBuffer());
   if (Magic == file_magic::coff_import_library) {
     InputFile *Imp = make<ImportFile>(MB);
     Imp->ParentName = ParentName;
     Symtab->addFile(Imp);
     return;
   }
 
   InputFile *Obj;
   if (Magic == file_magic::coff_object) {
     Obj = make<ObjFile>(MB);
   } else if (Magic == file_magic::bitcode) {
-    Obj = make<BitcodeFile>(MB);
+    Obj = make<BitcodeFile>(MB, ParentName, OffsetInArchive);
   } else {
     error("unknown file type: " + MB.getBufferIdentifier());
     return;
   }
 
   Obj->ParentName = ParentName;
   Symtab->addFile(Obj);
   log("Loaded " + toString(Obj) + " for " + SymName);
 }
 
 void LinkerDriver::enqueueArchiveMember(const Archive::Child &C,
                                         StringRef SymName,
                                         StringRef ParentName) {
 
   auto ReportBufferError = [=](Error &&E,
                               StringRef ChildName) {
     fatal("could not get the buffer for the member defining symbol " +
           SymName + ": " + ParentName + "(" + ChildName + "): " +
           toString(std::move(E)));
   };
 
   if (!C.getParent()->isThin()) {
+    uint64_t OffsetInArchive = C.getChildOffset();
     Expected<MemoryBufferRef> MBOrErr = C.getMemoryBufferRef();
     if (!MBOrErr)
       ReportBufferError(MBOrErr.takeError(), check(C.getFullName()));
     MemoryBufferRef MB = MBOrErr.get();
-    enqueueTask([=]() { Driver->addArchiveBuffer(MB, SymName, ParentName); });
+    enqueueTask([=]() {
+      Driver->addArchiveBuffer(MB, SymName, ParentName, OffsetInArchive);
+    });
     return;
   }
 
   std::string ChildName = CHECK(
       C.getFullName(),
       "could not get the filename for the member defining symbol " +
       SymName);
   auto Future = std::make_shared<std::future<MBErrPair>>(
       createFutureForFile(ChildName));
   enqueueTask([=]() {
     auto MBOrErr = Future->get();
     if (MBOrErr.second)
       ReportBufferError(errorCodeToError(MBOrErr.second), ChildName);
     Driver->addArchiveBuffer(takeBuffer(std::move(MBOrErr.first)), SymName,
-                             ParentName);
+                             ParentName, /* OffsetInArchive */ 0);
   });
 }
 
 static bool isDecorated(StringRef Sym) {
   return Sym.startswith("@") || Sym.contains("@@") || Sym.startswith("?") ||
          (!Config->MinGW && Sym.contains('@'));
 }
 

COFF/InputFiles.h

public:
   // to avoid creating unnecessary thunks.
   bool Live = !Config->DoGC;
   bool ThunkLive = !Config->DoGC;
 };
 
 // Used for LTO.
 class BitcodeFile : public InputFile {
 public:
-  explicit BitcodeFile(MemoryBufferRef M) : InputFile(BitcodeKind, M) {}
+  BitcodeFile(MemoryBufferRef MB, StringRef ArchiveName,
+              uint64_t OffsetInArchive);
   static bool classof(const InputFile *F) { return F->kind() == BitcodeKind; }
   ArrayRef<Symbol *> getSymbols() { return Symbols; }
   MachineTypes getMachineType() override;
   static std::vector<BitcodeFile *> Instances;
   std::unique_ptr<llvm::lto::InputFile> Obj;
 
 private:
   void parse() override;

COFF/InputFiles.cpp

void ImportFile::parse() {
   // If type is function, we need to create a thunk which jump to an
   // address pointed by the __imp_ symbol. (This allows you to call
   // DLL functions just like regular non-DLL functions.)
   if (Hdr->getType() == llvm::COFF::IMPORT_CODE)
     ThunkSym = Symtab->addImportThunk(
         Name, cast_or_null<DefinedImportData>(ImpSym), Hdr->Machine);
 }
 
+BitcodeFile::BitcodeFile(MemoryBufferRef MB, StringRef ArchiveName,
+                         uint64_t OffsetInArchive)
+    : InputFile(BitcodeKind, MB) {
+  std::string Path = MB.getBufferIdentifier().str();
+
+  // ThinLTO assumes that all MemoryBufferRefs given to it have a unique
+  // name. If two archives define two members with the same name, this
+  // causes a collision which result in only one of the objects being taken
+  // into consideration at LTO time (which very likely causes undefined
+  // symbols later in the link stage). So we append file offset to make
+  // filename unique.
+  MemoryBufferRef MBRef(
+      MB.getBuffer(),
+      Saver.save(ArchiveName + Path +
+                 (ArchiveName.empty() ? "" : utostr(OffsetInArchive))));
+
+  Obj = check(lto::InputFile::create(MBRef));
+}
+
 void BitcodeFile::parse() {
-  Obj = check(lto::InputFile::create(MemoryBufferRef(
-      MB.getBuffer(), Saver.save(ParentName + MB.getBufferIdentifier()))));
   std::vector<std::pair<Symbol *, bool>> Comdat(Obj->getComdatTable().size());
   for (size_t I = 0; I != Obj->getComdatTable().size(); ++I)
     // FIXME: lto::InputFile doesn't keep enough data to do correct comdat
     // selection handling.
     Comdat[I] = Symtab->addComdat(this, Saver.save(Obj->getComdatTable()[I]));
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols()) {
     StringRef SymName = Saver.save(ObjSym.getName());
     int ComdatIndex = ObjSym.getComdatIndex();

test/COFF/thinlto-archivecollision.ll

+; REQUIRES: x86
+; RUN: rm -fr %t
+; RUN: mkdir %t %t/a %t/b
+; RUN: opt -thinlto-bc -o %t/main.obj %s
+; RUN: opt -thinlto-bc -o %t/a/bar.obj %S/Inputs/lto-dep.ll
+; RUN: opt -thinlto-bc -o %t/b/bar.obj %S/Inputs/bar.ll
+; RUN: llvm-ar crs %t/libbar.lib %t/a/bar.obj %t/b/bar.obj
+; RUN: lld-link -out:%t/main.exe -entry:main -lldsavetemps \
+; RUN:     -subsystem:console %t/main.obj %t/libbar.lib
+; RUN: FileCheck %s < %t/main.exe.resolution.txt
+
+; CHECK: {{/thinlto-archivecollision.ll.tmp/main.obj$}}
+; CHECK: {{^-r=.*/thinlto-archivecollision.ll.tmp/main.obj,main,px$}}
+; CHECK: {{/thinlto-archivecollision.ll.tmp/libbar.libbar.obj[0-9]+$}}
+; CHECK-NEXT: {{^-r=.*/thinlto-archivecollision.ll.tmp/libbar.libbar.obj[0-9]+,foo,p$}}
+; CHECK-NEXT: {{/thinlto-archivecollision.ll.tmp/libbar.libbar.obj[0-9]+$}}
+; CHECK-NEXT: {{^-r=.*/thinlto-archivecollision.ll.tmp/libbar.libbar.obj[0-9]+,bar,p$}}
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+declare void @bar()
+declare void @foo()
+
+define i32 @main() {
+  call void @foo()
+  call void @bar()
+  ret i32 0
+}

test/COFF/thinlto-archives.ll

 ; REQUIRES: x86
 ; RUN: rm -fr %T/thinlto-archives
 ; RUN: mkdir %T/thinlto-archives %T/thinlto-archives/a %T/thinlto-archives/b
 ; RUN: opt -thinlto-bc -o %T/thinlto-archives/main.obj %s
 ; RUN: opt -thinlto-bc -o %T/thinlto-archives/a/bar.obj %S/Inputs/lto-dep.ll
 ; RUN: opt -thinlto-bc -o %T/thinlto-archives/b/bar.obj %S/Inputs/bar.ll
 ; RUN: llvm-ar crs %T/thinlto-archives/a.lib %T/thinlto-archives/a/bar.obj
 ; RUN: llvm-ar crs %T/thinlto-archives/b.lib %T/thinlto-archives/b/bar.obj
 ; RUN: lld-link -out:%T/thinlto-archives/main.exe -entry:main \
 ; RUN:     -lldsavetemps -subsystem:console %T/thinlto-archives/main.obj \
 ; RUN:     %T/thinlto-archives/a.lib %T/thinlto-archives/b.lib
 ; RUN: FileCheck %s < %T/thinlto-archives/main.exe.resolution.txt
 
 ; CHECK: {{/thinlto-archives/main.obj$}}
 ; CHECK: {{^-r=.*/thinlto-archives/main.obj,main,px$}}
-; CHECK: {{/thinlto-archives/a.libbar.obj$}}
-; CHECK-NEXT: {{^-r=.*/thinlto-archives/a.libbar.obj,foo,p$}}
-; CHECK-NEXT: {{/thinlto-archives/b.libbar.obj$}}
-; CHECK-NEXT: {{^-r=.*/thinlto-archives/b.libbar.obj,bar,p$}}
+; CHECK: {{/thinlto-archives/a.libbar.obj[0-9]+$}}
+; CHECK-NEXT: {{^-r=.*/thinlto-archives/a.libbar.obj[0-9]+,foo,p$}}
+; CHECK-NEXT: {{/thinlto-archives/b.libbar.obj[0-9]+$}}
+; CHECK-NEXT: {{^-r=.*/thinlto-archives/b.libbar.obj[0-9]+,bar,p$}}
 
 target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-windows-msvc"
 
 declare void @bar()
 declare void @foo()
 
 define i32 @main() {
   call void @foo()
   call void @bar()
   ret i32 0
 }