[COFF] Assign unique identifiers to ObjFiles from LTO
ClosedPublic
Actions

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

Details

Reviewers

ruiu
asmith
MaskRay

Commits

rG91a6bfed6163: [COFF] Assign unique identifiers to ObjFiles from LTO

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

Repository: rG LLVM Github Monorepo

Event Timeline

rnk created this revision.Apr 15 2020, 11:15 AM

Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 11:15 AM

Herald added subscribers: dexonsmith, steven_wu, hiraditya, inglorion. · View Herald Transcript

Harbormaster failed remote builds in B53387: Diff 257767!Apr 15 2020, 11:30 AM

fix bug with lto object cache

Harbormaster failed remote builds in B53394: Diff 257782!Apr 15 2020, 11:33 AM

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.

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.

upload changes

Harbormaster failed remote builds in B53801: Diff 258448!Apr 17 2020, 4:59 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

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.