No worries, thanks for your comments! Landing now.

@zturner: I rewrote the comment to emphasize that we're testing reading and writing of source file names, with alignment more as a side note. Do you like it better this way?

stricter check

removed trailing whitespace in yaml files

renamed confusingly named variable and cut some redundant text from tests

added test

added test

Actually, I think there is a way to verify that the alignment is correct. If I generate a PDB from YAML using the old code (without this change) and dump it with llvm-pdbdump pretty -all, I get corrupted filenames. If I apply this change, generate the PDB from YAML again, and dump with llvm-pdbdump pretty -all, the filenames are correct. Since pretty uses the DIA library, I think that proves that the change I've implemented here expresses the correct behavior.

@amccarth, you raise a good point that I neglected to address before. I looked at changing the location we read from vs. changing the location we write to, and concluded that the location we read from is probably correct, because we haven't seen corrupted filenames in PDBs not generated by our tools, to my knowledge. Looking at the reference implementation, e.g. QueryFileInfo and QueryFileInfo2 have the alignment code after the code that deals with filenames. So it seems correct to write the filenames after the fileinfos, and then pad the subsection as necessary, rather than pad between the fileinfos and the filenames. The real proof will be once I actually get the filenames to be displayed by Microsoft's tools, but we're not quite there yet.

Thank for the review, @gkistanova! I've added support for the clean and jobs properties.

Then again, I don't feel strongly enough about my preference for separate tools to really hold this up. I'm ok with just renaming this to llvm-pdbtool or some such. The code looks fine to me apart from the thing I mentioned inline.

What would be the advantage of having it be a separate tool?

I feel like llvm-pdbdump already does a ton of things. Having a tool to merge pdbs seems useful, but I'm not sure llvm-pdbdump should be that tool. If we want all that functionality in a single tool I think we should at least rename it to something like llvm-pdbtool (because we're not really dumping pdbs now), but I would actually prefer having this be a separate tool.

lgtm

Thanks for reviewing, @gkistanova. I think I have addressed all the points you raised.

Thank you for taking a look, @gkistanova. I've fixed the issues you pointend out inline. As for your questions:

ran it through autopep8 to fix the formatting and fixed escaping of percent (thanks @rnk)

I set up a buildmaster and worker locally and have been able to perform a number of successful builds using this configuration.

Please also update the example in the summary, e.g. createBlock() now only takes one argument.

It seems a little awkward to me that we're passing both a filename and an index for the same filename to createBlock. Looking through the code, I get the impression that we don't actually need the StringRef, only the index. If so, could you remove the StringRef?

added message and fixed case

check that msvclto-order-b.obj is not passed to link.exe

Good call on the unnecessary vector and the missing newline! Fixed.

@pcc's suggestions (thanks!)

lgtm!

@mehdi_amini, still lgty?

iterator_facade_base is already supposed to provide operator-> when you supply operator*. Looking into this offline together, it seems the type checker is tripping over the const ValueType on line 160, and that providing a non-const operator* may fix that.

@davide, we could extract the response file generation into its own function and write a unit test for it. On the other hand, I don't want to spend too much time polishing msvclto - I'd rather spend time on making it unnecessary.

• No warning when not using Ninja.
• Simplified the change.
• Fixed buggy behavior on Linux.

This will emit a warning when ThinLTO is enabled and the build system is not Ninja. I'll cook up a version that inhibits that warning.

Alternatively, we could limit the parallelism of the link jobs, but limiting the number of link jobs is better, because it avoids having to wait a very long time for the last link job. For the same reason, the number of concurrent link jobs is 2 rather than 1.

I think that's all your comments, @pcc. The functions are just there to make sure the globals are used. I've added a comment to that effect. The functions are purposely not part of the comdats, so that they can be externally visible without causing the comdats to have externally visible members.

removed unnecessary else

Implemented more of @pcc's suggestions:

• Use Comdat* instead of StringRef in MergedMComdats.
• Populate MergedMComdats in existing loop instead of creating a new one.
• Remove regexes when matching unpromoted names.

reordered test and put checks next to the IR they apply to

perform comdat renaming in promoteInternals

fixed bug that caused function definitions to disappear

I've implemented more of the suggestions (keeping comdats together, renaming instead of deleting comdats) in D31963. If that diff looks good to you guys, I would like to land that one instead of this one.

This is an alternative to D31735. Instead of deleting comdats that are invalidated, it keeps comdats valid by keeping their members in the same module and renaming the comdat if its leader is renamed. I think this is a cleaner solution.

It looks like this change causes problems for the Chromium asan build on Linux: https://bugs.chromium.org/p/chromium/issues/detail?id=709536
The issues go away when I revert this commit. Can you take a look?

also test the case where the comdat leader has !type metadata and the other member doesn't

two-pass implementation

@mehdi_amini: W.r.t. your question about verifying the requirements for comdats, this happens in Codegen/TargetLoweringObjectFileImpl.cpp for COFF, ELF, and Mach-O. They each have their own restrictions.

Fixed issues pointed out by @ruiu (thanks!)

Made changes requested by @pcc

We will not be needing this.

Previous diff: D29691. The early exit in Driver.cpp line 820 is now before any paths are enqueued, avoiding the race that existed in the previous diff.

addressed @pcc's comments

Added LLVM IR test.

Thanks, @pcc for helping me look into this and realizing that doing the alias processing first fixes the problem. I've also included a small change to use GA-> instead of I->, which prevents us from using the iterator in an invalid state and hitting another assert.

@mehdi_amini: This is reduced from a C++ program that triggered the problem. The compiler failed before outputing IR, which is why it is C++. I can try to see if I can create some IR that will reproduce the problem, but it will have to wait until tomorrow.