@joerg, any comments?

zturner: Do you have some time to look at this (D33961) and D34013?

I see. Yes, it's unfortunate that this essentially repeats the list of projects in multiple places. On the other hand, I like having as much as possible in the part that can be modified without requiring a buildmaster reload, which would speak for leaving the checkout code in the builder. This is also what the sanitizer builders do. They trigger on changes to any of llvm, cfe, compiler-rt, libcxx, libcxxabi, libunwind, and lld, which we could also do here. It's basically triggering on everything, which is safe. Theoretically, it might increase resource usage a bit, but the ThinLTO builder as-is is basically always building, so the number of builds triggered and the amount of work the worker does should be the same. Would you like me to make that change?

What do you mean by "the scheduling logic gets out of sync"?

Implemented @zturner's suggestions

Thanks! Assuming the tests pass, lgtm

Thanks! lgtm

Hmm, good point. I hadn't considered that changing the name in the same change would mix a lot of mechanical changes with actual interesting to review changes. Let's keep them in two separate changes, then. Can you land them close together in time, though?

Thanks for doing this, zturner! I have some more inline comments and questions.

Oops, I submitted that before I was done reviewing. More comments probably coming soon.

I would like to change the changes to these option names to coincide with renaming llvm-pdbdump to llvm-pdbtool (or some such) so that we only break the interface once.

This change is no longer necessary, right?

The vast majority of changes here are updating tests to expect double quotes instead of single quotes. The actual change to the encoder is in llvm/lib/Support/YAMLTraits.cpp.

lgtm. Thanks!

lgtm

Happy you're doing this, but see inline comments for a few requested changes.

Superseded by D33785

lgtm modulo inline comment about messsage in the assert

I guess I'm saying it seems nice to always write aligned records until we come up with some reason to not write aligned records.

Thanks for fixing this, @zturner! I bumped into this a few days ago, but never really came up with a fix I was confident was the right thing.

pass -j argument to lit if jobs parameter is present

This is not ready for review just yet. There are some other problems with the line information (all offsets show up as 0 when read with the DIA library) that I want to look into a bit more before I'm confident that this change is doing the right thing.

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.