COFF: include archive name in LTO object name

[inglorion]

Summary

In the ELF linker, we create the buffer identifier for bitcode files by appending the object name to the archive name. This change makes the COFF linker do the same. Without the change, ThinLTO builds can fail with an error message about multiple ThinLTO modules per object file, caused by object files contained in different archives having the same name.

[mehdi_amini]

In ELF I believe the offset to the member name is also added. Because an archive can contain two member with the same name.

[pcc]

In D31402#711441, @mehdi_amini wrote:

In ELF I believe the offset to the member name is also added. Because an archive can contain two member with the same name.

And it is possible to construct such an archive using lib.exe with some effort:

>touch foo.cpp

>cl /c foo.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

foo.cpp

>lib /out:foo.lib foo.obj
Microsoft (R) Library Manager Version 14.00.24215.1
Copyright (C) Microsoft Corporation.  All rights reserved.


>copy foo.lib foo2.lib
        1 file(s) copied.

>lib /out:foo3.lib foo.lib foo2.lib
Microsoft (R) Library Manager Version 14.00.24215.1
Copyright (C) Microsoft Corporation.  All rights reserved.


>\src\llvm-project\ra\bin\llvm-ar t foo3.lib
foo.obj
foo.obj

So we probably want to support such archives eventually. I see it as a lower priority than this bug, though, so up to you whether you want to fix it now.

For this change, please add a test case.

[inglorion]

Adding the offset also affects non-LTO archive members, right? If so, I prefer to fix that in a follow up. For this one, I'd like to change ParentName + MB.getBufferIdentifier() to toString(this), which is more consistent with how we form the names in other places. I will also add a test.

[pcc]

In D31402#711498, @inglorion wrote:

Adding the offset also affects non-LTO archive members, right? If so, I prefer to fix that in a follow up. For this one, I'd like to change ParentName + MB.getBufferIdentifier() to toString(this), which is more consistent with how we form the names in other places. I will also add a test.

That doesn't seem like a good idea to me. toString is mostly meant to be used for error reporting. Here's the implementation: http://llvm-cs.pcc.me.uk/tools/lld/COFF/InputFiles.cpp#395

As you can see it converts names to basenames and normalizes case, so you'd have problems with files with similar names.

[ruiu]

Yes, toString() is for generating error messages, and message contents can change anytime. It is not designed for other purposes.

[inglorion]

added test

[pcc]

LGTM

lld/trunk/COFF/InputFiles.cpp

void ImportFile::parse() {
   // DLL functions just like regular non-DLL functions.)
   if (Hdr->getType() != llvm::COFF::IMPORT_CODE)
     return;
   ThunkSym = cast<DefinedImportThunk>(
       Symtab->addImportThunk(Name, ImpSym, Hdr->Machine)->body());
 }
 
 void BitcodeFile::parse() {
-  Obj = check(lto::InputFile::create(
-      MemoryBufferRef(MB.getBuffer(), Saver.save(MB.getBufferIdentifier()))));
+  Obj = check(lto::InputFile::create(MemoryBufferRef(
+      MB.getBuffer(), Saver.save(ParentName + MB.getBufferIdentifier()))));
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols()) {
     StringRef SymName = Saver.save(ObjSym.getName());
     auto Flags = ObjSym.getFlags();
     Symbol *Sym;
     if (Flags & object::BasicSymbolRef::SF_Undefined) {
       Sym = Symtab->addUndefined(SymName, this, false);
     } else if (Flags & object::BasicSymbolRef::SF_Common) {
       Sym = Symtab->addCommon(this, SymName, ObjSym.getCommonSize());

lld/trunk/test/COFF/Inputs/bar.ll

+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define void @bar() {
+  ret void
+}

lld/trunk/test/COFF/thinlto-archives.ll

+; 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:     -subsystem:console %T/thinlto-archives/main.obj \
+; RUN:     %T/thinlto-archives/a.lib %T/thinlto-archives/b.lib
+
+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
+}

lld/trunk/test/ELF/lto/archive-no-index.ll

+; REQUIRES: x86
+; Tests that we suggest that LTO symbols missing from an archive index
+; may be the cause of undefined references, but only if we both
+; encountered an empty archive index and undefined references (to prevent
+; noisy false alarms).
+
+; RUN: rm -fr %T/archive-no-index
+; RUN: mkdir %T/archive-no-index
+; RUN: llvm-as %S/Inputs/archive.ll -o %T/archive-no-index/f.o
+; RUN: llvm-ar cr %T/archive-no-index/libf.a
+; RUN: llvm-ar qS %T/archive-no-index/libf.a %T/archive-no-index/f.o
+; RUN: llvm-as %s -o %t.o
+; RUN: not ld.lld -emain -m elf_x86_64 %t.o -o %t %T/archive-no-index/libf.a \
+; RUN:     2>&1 | FileCheck --check-prefix=NOTE %s
+
+; RUN: llvm-ar crs %T/archive-no-index/libfs.a %T/archive-no-index/f.o
+; RUN: ld.lld -emain -m elf_x86_64 %t.o -o %t %T/archive-no-index/libf.a \
+; RUN:     %T/archive-no-index/libfs.a
+
+; RUN: llvm-as %S/Inputs/archive-3.ll -o %T/archive-no-index/foo.o
+; RUN: llvm-ar crs %T/archive-no-index/libfoo.a %T/archive-no-index/foo.o
+; RUN: not ld.lld -emain -m elf_x86_64 %t.o -o %t %T/archive-no-index/libfoo.a \
+; RUN:     2>&1 | FileCheck --check-prefix=NO-NOTE %s
+
+; NOTE: undefined symbol 'f'
+; NOTE: archive listed no symbols
+
+; NO-NOTE: undefined symbol 'f'
+; NO-NOTE-NOT: archive listed no symbols
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @f()
+
+define i32 @main() {
+  call void @f()
+  ret i32 0
+}