This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Assign unique identifiers to ObjFiles from LTO
ClosedPublic

Authored by rnk on Apr 15 2020, 11:15 AM.

Details

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.

Diff Detail

Event Timeline

rnk created this revision.Apr 15 2020, 11:15 AM
rnk updated this revision to Diff 257782.Apr 15 2020, 11:33 AM
  • fix bug with lto object cache

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
3

%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

15

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.

32

Delete trailing spaces (invisible on Phabricator)

34

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–9

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

lld/test/COFF/weak-external3.test
9–10

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

MaskRay added inline comments.Apr 15 2020, 3:34 PM
lld/test/COFF/pdb-thinlto.ll
5

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

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 updated this revision to Diff 258448.Apr 17 2020, 4:54 PM
rnk marked 7 inline comments as done.
  • upload changes
MaskRay accepted this revision.Apr 17 2020, 5:03 PM

Looks great!

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

Maybe worth mentioning this for the justification.

This revision is now accepted and ready to land.Apr 17 2020, 5:03 PM
rnk added a comment.Apr 17 2020, 5:17 PM

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
32

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

34

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

This revision was automatically updated to reflect the committed changes.