Does this look good now?

without second constructor

Updated as suggested by @rsmith (thanks!)

Use [=] instead of explicitly passing StringRefs to ReportBufferError.

(Following the same archive(member) format also used by toString() in InputFiles.cpp)

Before:

lld-link: error: could not get the buffer for the member defining symbol spvDiagnosticDestroy: No such file or directory

LGTM

See earlier comment.

lgtm, thanks!

LGTM, with some suggestions in the comments.

Thank you!

Re-upload only the test change.

Simpler test. This crashes without D57073 and passes with D57073.

pcc, does D57192 implement your suggestion?

Just saw your comment, pcc. I'll look into that.

add missing .1

replaced .c test with an .ll test

@fedor.sergeev: You're right, thanks for pointing that out. Sent D56336 to fix that.

Thanks! For the case where you don't want the files to expire, can you instead set the expiration age to 0? That should stop the pruner from removing them.

LGTM

I was thinking that as well, and it did fix the size regression: from 151,548,928 to 141,140,992 bytes, where the non-LTO version is 140,072,960 bytes.

Since r338767, "foo,p" and "bar,p" show up as "foo,px" and "bar,px", indicating that they are used in regular objects. However, there are no regular (non-LTO) objects in the test. Only main should have the 'x'.

added test

Building Chromium with ThinLTO without this patch causes test_chrome_with_chromedriver.py to fail. With the patch, it runs successfully. (https://crbug.com/881036).

We don't need this. Abandoning.

This is really useful. Thanks for doing this. I got a couple of questions and comments in-line.

update llvm/test/ThinLTO/X86/cache.ll to match the new expectations

Do you know why the temporary files are being left on disk? I wouldn't have expected that to happen because they are being created with delete-on-close.

Comment changed in rL338755.

Sorry, Peter. Saw your reply after I had already pushed the change. Would:

In D50127#1185981, @thakis wrote:

https://bugs.chromium.org/p/chromium/issues/detail?id=856635#c10 sounds like we only saw this if the pattern was made smaller. Are we sure this change fixes an actual problem?

Is this good to go?

Use 36-character set instead of 32-character set, per @zturner's suggestion.

Yeah. On top of the cost of getting a number from the cryptographically secure random number generator, I don't think using mod instead of and will cost that much. I'll make the change. We could add a few more characters ("-_@", probably others), but I think digits+letters is already a good improvement to what we currently have, without having to worry about file systems not liking some characters.

This diff fixes a problem where, on Windows, TempFile::create() would immediately fail when the same name is tried again before the first file with that name is gone. That makes collisions less of an issue, because we can actually use more of the available names before we give up. It also fixes a problem where trying to create a tempfile when all available names are taken would lead to an infinite loop.

@zturner, the models we use here are 6 characters. Chosen from a set of 16 characters gives 16M possibilities. Randomly generating them gives a 50%+ probability of a collision after 5000 names or so. I have another patch up (D50127) to use a 32-character set instead, which means the same 6 character model gives about a billion possible names and you need to generate about 40,000 of them before you have a 50% probability of a collision.

The inspiration for this is http://crbug.com/856635, where we're seeing the same file name generated multiple times during a single run.

fixed comment

put error message check back in

Hmm, seems like I lost checking the error message along the way.

Per @pcc's suggestions:

check for error message in test

Ah, just saw your comment about adding a check. Yes, I was checking that llvm-dis succeeds; I'll check for the error message instead to make it clearer what we're testing.

Thanks, @tejohnson! I made the test case smaller.

In D49777#1175774, @mehdi_amini wrote:

but declarations cannot be internal.

Do you mean that the IR verifier would fail?