[COFF] Assign unique identifiers to ObjFiles from LTO

[rnk]

Summary

Use the unique filenames that are used when /lldsavetemps is passed.
After this change, module names for LTO blobs in PDBs will be unique.

Revert some changes from 1e0b158db (2017) that are no longer necessary
after removing MSVC LTO support.

[rnk]

• fix bug with lto object cache

[MaskRay]

Use the unique filenames that are used when /lldsavetemps is passed.

Is this for debug convenience? (llvm-pdbutil dump output) Or required by some tools?

lld/COFF/LTO.cpp

210

When can objBuf be empty?

lld/test/COFF/pdb-thinlto.ll

2

%T is deprecated

https://llvm.org/docs/CommandGuide/lit.html

You can use %t.dir or just %t

; RUN: rm -rf %t.dir
; RUN: mkdir %t.dir
; RUN: cd %t.dir

Then you can remove one rm -Rf below

14

Not sure whether COFF wants to adopt this convention: in lld/ELF and some binary utilities, we start to use ## (translated to .ll: ;; ) for comments, # (; ) for RUN/CHECK lines.

This convention makes comments slightly easier to be discovered.

31

Delete trailing spaces (invisible on Phabricator)

33

You may indent Mod 0000 by 2 spaces to reflect the actual output... but why does llvm-pdbutil print two spaces only for Mod 0000?

lld/test/COFF/weak-external.test

8

Nit: add 5 spaces to make CHECK1 content aligned with the next line.

lld/test/COFF/weak-external3.test

9

Nit: add 5 spaces to make CHECK1 content aligned with the next line.

[MaskRay]

lld/test/COFF/pdb-thinlto.ll

4

Will .bc be slightly better for the bitcode extension name? .bc makes it clear it is not a regular object file.

[asmith]

In D78221#1985170, @MaskRay wrote:

Use the unique filenames that are used when /lldsavetemps is passed.

Is this for debug convenience? (llvm-pdbutil dump output) Or required by some tools?

MSVC expects a unique name for each module in a pdb.

[rnk]

• upload changes

[MaskRay]

Looks great!

MSVC expects a unique name for each module in a pdb.

Maybe worth mentioning this for the justification.

[rnk]

Apparently I didn't hit send...

lld/COFF/LTO.cpp

210

This is meant to preserve existing behavior from the original split loops. In the case where neither buf nor files contains an object, no input file was added. I assume that this can happen if there was an error during code generation.

---

Also, it occurs to me that split loops from before were reordering the native object files for the final link, which seems like asking non-determinism: if the object is found in the cache, it gets added later. This could be a serious cache bug fix...

lld/test/COFF/pdb-thinlto.ll

31

Speaking of oddities in the dump output, I wish it wouldn't print trailing spaces. =P

33

I think the extra spaces in the real output are a bug, I would prefer to see the output as I have it here.

lld/COFF/LTO.h

 class InputFile;
 
 class BitcodeCompiler {
 public:
   BitcodeCompiler();
   ~BitcodeCompiler();
 
   void add(BitcodeFile &f);
-  std::vector<StringRef> compile();
+  std::vector<InputFile *> compile();
 
 private:
   std::unique_ptr<llvm::lto::LTO> ltoObj;
   std::vector<SmallString<0>> buf;
   std::vector<std::unique_ptr<MemoryBuffer>> files;
   std::unique_ptr<llvm::raw_fd_ostream> indexFile;
   llvm::DenseSet<StringRef> thinIndices;
 };
 }
 }
 
 #endif

lld/COFF/LTO.cpp

for (const lto::InputFile::Symbol &objSym : obj.symbols()) {
     if (r.Prevailing)
       undefine(sym);
   }
   checkError(ltoObj->add(std::move(f.obj), resols));
 }
 
 // Merge all the bitcode files we have seen, codegen the result
 // and return the resulting objects.
-std::vector<StringRef> BitcodeCompiler::compile() {
+std::vector<InputFile *> BitcodeCompiler::compile() {
   unsigned maxTasks = ltoObj->getMaxTasks();
   buf.resize(maxTasks);
   files.resize(maxTasks);
 
   // The /lldltocache option specifies the path to a directory in which to cache
   // native object files for ThinLTO incremental builds. If a path was
   // specified, configure LTO to use it as the cache directory.
   lto::NativeObjectCache cache;
if (config->thinLTOIndexOnly) {
     if (indexFile)
       indexFile->close();
     return {};
   }
 
   if (!config->ltoCache.empty())
     pruneCache(config->ltoCache, config->ltoCachePolicy);
 
-  std::vector<StringRef> ret;
+  std::vector<InputFile *> ret;
   for (unsigned i = 0; i != maxTasks; ++i) {
-    if (buf[i].empty())
-      continue;
-    if (config->saveTemps) {
-      if (i == 0)
-        saveBuffer(buf[i], config->outputFile + ".lto.obj");
-      else
-        saveBuffer(buf[i], config->outputFile + Twine(i) + ".lto.obj");
-    }
-    ret.emplace_back(buf[i].data(), buf[i].size());
-  }
+    // Assign unique names to LTO objects. This ensures they have unique names
+    // in the PDB if one is produced. The names should look like:
+    // - foo.exe.lto.obj
+    // - foo.exe.lto.1.obj
+    // - ...
+    StringRef ltoObjName =
+        saver.save(Twine(config->outputFile) + ".lto" +
+                   (i == 0 ? Twine("") : Twine('.') + Twine(i)) + ".obj");
+
+    // Get the native object contents either from the cache or from memory.  Do
+    // not use the cached MemoryBuffer directly, or the PDB will not be
+    // deterministic.
+    StringRef objBuf;
+    if (files[i])
+      objBuf = files[i]->getBuffer();
+      objBuf = buf[i];
+    if (objBuf.empty())
+      continue;
 
-  for (std::unique_ptr<MemoryBuffer> &file : files)
-    if (file)
-      ret.push_back(file->getBuffer());
+    if (config->saveTemps)
+      saveBuffer(buf[i], ltoObjName);
+    ret.push_back(make<ObjFile>(MemoryBufferRef(objBuf, ltoObjName)));
+  }
 
   return ret;
 }

lld/COFF/SymbolTable.h

public:
   // for U from the symbol table, and if found, set the symbol as
   // a weak alias for U.
   Symbol *findMangle(StringRef name);
 
   // Build a set of COFF objects representing the combined contents of
   // BitcodeFiles and add them to the symbol table. Called after all files are
   // added and before the writer writes results to a file.
   void addCombinedLTOObjects();
-  std::vector<StringRef> compileBitcodeFiles();
 
   // Creates an Undefined symbol for a given name.
   Symbol *addUndefined(StringRef name);
 
   Symbol *addSynthetic(StringRef n, Chunk *c);
   Symbol *addAbsolute(StringRef n, uint64_t va);
 
   Symbol *addUndefined(StringRef name, InputFile *f, bool isWeakAlias);

lld/COFF/SymbolTable.cpp

Symbol *SymbolTable::findMangle(StringRef name) {
   // Search for x86 C++ non-member function.
   return findByPrefix("?" + name.substr(1) + "@@Y");
 }
 
 Symbol *SymbolTable::addUndefined(StringRef name) {
   return addUndefined(name, nullptr, false);
 }
 
-std::vector<StringRef> SymbolTable::compileBitcodeFiles() {
-  lto.reset(new BitcodeCompiler);
-  for (BitcodeFile *f : BitcodeFile::instances)
-    lto->add(*f);
-  return lto->compile();
-}
-
 void SymbolTable::addCombinedLTOObjects() {
   if (BitcodeFile::instances.empty())
     return;
 
   ScopedTimer t(ltoTimer);
-  for (StringRef object : compileBitcodeFiles()) {
-    auto *obj = make<ObjFile>(MemoryBufferRef(object, "lto.tmp"));
+  lto.reset(new BitcodeCompiler);
+  for (BitcodeFile *f : BitcodeFile::instances)
+    lto->add(*f);
+  for (InputFile *newObj : lto->compile()) {
+    ObjFile *obj = cast<ObjFile>(newObj);
     obj->parse();
     ObjFile::instances.push_back(obj);
   }
 }
 
 } // namespace coff
 } // namespace lld

lld/test/COFF/lto-parallel.ll

 ; REQUIRES: x86
 ; RUN: llvm-as -o %t.obj %s
-; RUN: lld-link -opt:noicf /out:%t.exe /entry:foo /include:bar /opt:lldltopartitions=2 /subsystem:console /lldmap:%t.map %t.obj
+; RUN: lld-link -opt:noicf /out:%t.exe /entry:foo /include:bar /opt:lldltopartitions=2 /subsystem:console /lldmap:%t.map %t.obj /debug /pdb:%t.pdb
 ; RUN: FileCheck %s < %t.map
+; RUN: llvm-pdbutil dump %t.pdb --modules | FileCheck %s --check-prefix=PDB
 
 target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-windows-msvc"
 
-; CHECK: lto.tmp
-; CHECK: lto.tmp
+; CHECK: lto-parallel.ll.tmp.exe.lto.obj:
+; CHECK: lto-parallel.ll.tmp.exe.lto.obj:
 ; CHECK-NEXT: foo
 define void @foo() {
   call void @bar()
   ret void
 }
 
-; CHECK: lto.tmp
-; CHECK: lto.tmp
+; CHECK: lto-parallel.ll.tmp.exe.lto.1.obj:
+; CHECK: lto-parallel.ll.tmp.exe.lto.1.obj:
 ; CHECK: bar
 define void @bar() {
   call void @foo()
   ret void
 }
+
+
+; Objects in the PDB should receive distinct names.
+
+; PDB: Modules
+; PDB: Mod 0000 | `{{.*}}lto-parallel.ll.tmp.exe.lto.obj`:
+; PDB: Obj: `{{.*}}lto-parallel.ll.tmp.exe.lto.obj`:
+; PDB: Mod 0001 | `{{.*}}lto-parallel.ll.tmp.exe.lto.1.obj`:
+; PDB: Obj: `{{.*}}lto-parallel.ll.tmp.exe.lto.1.obj`:
+; PDB: Mod 0002 | `* Linker *`:
+; PDB: Obj: ``:

lld/test/COFF/pdb-thinlto.ll

+; REQUIRES: x86
+; RUN: rm -fr %T/thinlto
+; RUN: mkdir %T/thinlto
+; RUN: opt -thinlto-bc -o %T/thinlto/main.obj %s
+; RUN: opt -thinlto-bc -o %T/thinlto/foo.obj %S/Inputs/lto-dep.ll
+
+; Even if the native object is cached, the PDB must be the same.
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+
+; RUN: lld-link /lldltocache:%t.cache /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.obj /debug /pdb:%t.pdb
+
+; RUN: llvm-pdbutil dump --modules %t.pdb | FileCheck %s
+
+; Run again with the cache. Make sure we get the same object names.
+
+; RUN: lld-link /lldltocache:%t.cache /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.obj /debug /pdb:%t.pdb
+
+; RUN: llvm-pdbutil dump --modules %t.pdb | FileCheck %s
+
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define i32 @main() {
+  call void @foo()
+  ret i32 0
+}
+
+declare void @foo()
+
+; CHECK:                           Modules                           
+; CHECK: ============================================================
+; CHECK: Mod 0000 | `{{.*}}main.exe.lto.obj`: 
+; CHECK: Obj: `{{.*}}main.exe.lto.obj`: 
+; CHECK: Mod 0001 | `{{.*}}main.exe.lto.1.obj`: 
+; CHECK: Obj: `{{.*}}main.exe.lto.1.obj`: 
+; CHECK: Mod 0002 | `{{.*}}main.exe.lto.2.obj`: 
+; CHECK: Obj: `{{.*}}main.exe.lto.2.obj`: 
+; CHECK: Mod 0003 | `* Linker *`:

lld/test/COFF/thinlto.ll

 ; REQUIRES: x86
 ; RUN: rm -fr %T/thinlto
 ; RUN: mkdir %T/thinlto
 ; RUN: opt -thinlto-bc -o %T/thinlto/main.obj %s
 ; RUN: opt -thinlto-bc -o %T/thinlto/foo.obj %S/Inputs/lto-dep.ll
 ; RUN: lld-link /lldsavetemps /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.obj
-; RUN: llvm-nm %T/thinlto/main.exe1.lto.obj | FileCheck %s
+; RUN: llvm-nm %T/thinlto/main.exe.lto.1.obj | FileCheck %s
 
 ; Test various possible options for /opt:lldltojobs
 ; RUN: lld-link /lldsavetemps /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.obj /opt:lldltojobs=1
-; RUN: llvm-nm %T/thinlto/main.exe1.lto.obj | FileCheck %s
+; RUN: llvm-nm %T/thinlto/main.exe.lto.1.obj | FileCheck %s
 ; RUN: lld-link /lldsavetemps /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.obj /opt:lldltojobs=all
-; RUN: llvm-nm %T/thinlto/main.exe1.lto.obj | FileCheck %s
+; RUN: llvm-nm %T/thinlto/main.exe.lto.1.obj | FileCheck %s
 ; RUN: lld-link /lldsavetemps /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.obj /opt:lldltojobs=100
-; RUN: llvm-nm %T/thinlto/main.exe1.lto.obj | FileCheck %s
+; RUN: llvm-nm %T/thinlto/main.exe.lto.1.obj | FileCheck %s
 ; RUN: not lld-link /lldsavetemps /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.obj /opt:lldltojobs=foo 2>&1 | FileCheck %s --check-prefix=BAD-JOBS
 ; BAD-JOBS: error: /opt:lldltojobs: invalid job count: foo
 
 ; This command will store full path to foo.obj in the archive %t.lib
 ; Check that /lldsavetemps is still usable in such case.
 ; RUN: lld-link /lib %T/thinlto/foo.obj /out:%t.lib
 ; RUN: lld-link /lldsavetemps /out:%t.exe /entry:main /subsystem:console %T/thinlto/main.obj %t.lib
 

lld/test/COFF/weak-external.test

 # REQUIRES: x86
 # RUN: yaml2obj %s > %t.obj
 # RUN: llvm-as -o %t.lto.obj %S/Inputs/weak-external.ll
 # RUN: lld-link /out:%t1.exe /entry:g /subsystem:console %t.obj
 # RUN: lld-link /out:%t2.exe /entry:g /subsystem:console /lldmap:%t2.map %t.obj %t.lto.obj
 # RUN: FileCheck %s < %t2.map
 
-# CHECK: lto.tmp
-# CHECK-NEXT: lto.tmp
+# CHECK: .lto.obj:
+# CHECK-NEXT: .lto.obj:
 # CHECK-NEXT: 0 g
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: [  ]
 sections:
   - Name:            '.text'

lld/test/COFF/weak-external3.test

 # REQUIRES: x86
 # RUN: yaml2obj %s > %t.obj
 # RUN: llvm-as -o %t.lto.obj %S/Inputs/weak-external3.ll
 # RUN: lld-link /out:%t1.exe /entry:f /subsystem:console /lldmap:%t1.map %t.lto.obj
 # RUN: FileCheck --check-prefix=CHECK1 %s < %t1.map
 # RUN: lld-link /out:%t2.exe /entry:f /subsystem:console /lldmap:%t2.map %t.obj %t.lto.obj
 # RUN: FileCheck --check-prefix=CHECK2 %s < %t2.map
 
-# CHECK1: lto.tmp
-# CHECK1: lto.tmp
+# CHECK1: .lto.obj:
+# CHECK1-NEXT: .lto.obj:
 # CHECK1-NEXT: 0 g
 
 # CHECK2: weak-external3.test.tmp.obj
 # CHECK2-NEXT: 0 f
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64