- User Since
- Dec 15 2015, 10:55 AM (83 w, 3 d)
Fri, Jun 23
@joerg, any comments?
Jun 21 2017
Jun 20 2017
Jun 15 2017
Jun 13 2017
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?
Jun 12 2017
What do you mean by "the scheduling logic gets out of sync"?
Jun 9 2017
Implemented @zturner's suggestions
Thanks! Assuming the tests pass, lgtm
Jun 8 2017
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.
Jun 7 2017
Jun 6 2017
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.
Jun 2 2017
Jun 1 2017
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.
May 30 2017
pass -j argument to lit if jobs parameter is present
May 26 2017
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.
May 25 2017
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?
May 24 2017
removed trailing whitespace in yaml files
renamed confusingly named variable and cut some redundant text from tests
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.
May 23 2017
May 22 2017
Thank for the review, @gkistanova! I've added support for the clean and jobs properties.
May 19 2017
May 18 2017
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.
May 17 2017
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:
May 12 2017
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.
May 1 2017
Apr 28 2017
Please also update the example in the summary, e.g. createBlock() now only takes one argument.
Apr 27 2017
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?
Apr 21 2017
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.
Apr 20 2017
@pcc's suggestions (thanks!)
@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.
Apr 18 2017
@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.