- User Since
- Dec 15 2015, 10:55 AM (75 w, 3 d)
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?
Wed, May 24
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.
Tue, May 23
Mon, May 22
Thank for the review, @gkistanova! I've added support for the clean and jobs properties.
Fri, May 19
Thu, May 18
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.
Wed, May 17
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:
Fri, May 12
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.
Mon, May 1
Fri, Apr 28
Please also update the example in the summary, e.g. createBlock() now only takes one argument.
Thu, Apr 27
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.
Apr 13 2017
- No warning when not using Ninja.
- Simplified the change.
- Fixed buggy behavior on Linux.
Apr 12 2017
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.
Apr 11 2017
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.
Apr 8 2017
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?
Apr 6 2017
also test the case where the comdat leader has !type metadata and the other member doesn't
@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.
Apr 5 2017
Apr 4 2017
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.
Apr 3 2017
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.