inglorion (Bob Haarman)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 15 2015, 10:55 AM (75 w, 3 d)

Recent Activity

Yesterday

inglorion committed rL303917: [pdb] pad source file name buffer at the end instead of the beginning.
[pdb] pad source file name buffer at the end instead of the beginning
Thu, May 25, 2:12 PM
inglorion closed D33475: [pdb] pad source file name buffer at the end instead of the beginning by committing rL303917: [pdb] pad source file name buffer at the end instead of the beginning.
Thu, May 25, 2:12 PM
inglorion added a comment to D33475: [pdb] pad source file name buffer at the end instead of the beginning.

No worries, thanks for your comments! Landing now.

Thu, May 25, 2:10 PM
inglorion updated the diff for D33475: [pdb] pad source file name buffer at the end instead of the beginning.

@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?

Thu, May 25, 1:42 PM
inglorion committed rL303891: [llvm-pdbdump] [yaml2pdb] always include object file name in module info.
[llvm-pdbdump] [yaml2pdb] always include object file name in module info
Thu, May 25, 11:04 AM
inglorion closed D33463: [llvm-pdbdump] [yaml2pdb] always include object file name in module info by committing rL303891: [llvm-pdbdump] [yaml2pdb] always include object file name in module info.
Thu, May 25, 11:04 AM

Wed, May 24

inglorion added inline comments to D33463: [llvm-pdbdump] [yaml2pdb] always include object file name in module info.
Wed, May 24, 4:44 PM
inglorion updated the diff for D33463: [llvm-pdbdump] [yaml2pdb] always include object file name in module info.

stricter check

Wed, May 24, 4:42 PM
inglorion added inline comments to D33475: [pdb] pad source file name buffer at the end instead of the beginning.
Wed, May 24, 4:37 PM
inglorion updated the diff for D33475: [pdb] pad source file name buffer at the end instead of the beginning.

removed trailing whitespace in yaml files

Wed, May 24, 3:28 PM
inglorion updated the diff for D33475: [pdb] pad source file name buffer at the end instead of the beginning.

renamed confusingly named variable and cut some redundant text from tests

Wed, May 24, 3:27 PM
inglorion added inline comments to D33475: [pdb] pad source file name buffer at the end instead of the beginning.
Wed, May 24, 3:23 PM
inglorion updated the diff for D33463: [llvm-pdbdump] [yaml2pdb] always include object file name in module info.

added test

Wed, May 24, 3:20 PM
inglorion updated the diff for D33475: [pdb] pad source file name buffer at the end instead of the beginning.

added test

Wed, May 24, 3:04 PM
inglorion added a comment to D33475: [pdb] pad source file name buffer at the end instead of the beginning.

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.

Wed, May 24, 11:47 AM
inglorion added a comment to D33475: [pdb] pad source file name buffer at the end instead of the beginning.

@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.

Wed, May 24, 11:36 AM

Tue, May 23

inglorion created D33475: [pdb] pad source file name buffer at the end instead of the beginning.
Tue, May 23, 6:02 PM
inglorion created D33463: [llvm-pdbdump] [yaml2pdb] always include object file name in module info.
Tue, May 23, 2:47 PM

Mon, May 22

inglorion updated the diff for D33148: add generic annotated builder and clang-with-thin-lto-windows.

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

Mon, May 22, 5:04 PM

Fri, May 19

inglorion added inline comments to D33323: [llvm-pdbdump] Add the ability to merge PDBs.
Fri, May 19, 11:35 AM

Thu, May 18

inglorion added a comment to D33323: [llvm-pdbdump] Add the ability to merge PDBs.

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.

Thu, May 18, 1:26 PM
inglorion added a comment to D33323: [llvm-pdbdump] Add the ability to merge PDBs.

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

Thu, May 18, 1:14 PM
inglorion added a comment to D33323: [llvm-pdbdump] Add the ability to merge PDBs.

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.

Thu, May 18, 11:19 AM

Wed, May 17

inglorion accepted D33267: [lld] Write the correct age and machine type to the PDB..

lgtm

Wed, May 17, 4:37 PM
inglorion updated the diff for D33148: add generic annotated builder and clang-with-thin-lto-windows.

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

Wed, May 17, 4:29 PM
inglorion added a comment to D33148: add generic annotated builder and clang-with-thin-lto-windows.

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

Wed, May 17, 4:27 PM
inglorion committed rL303299: [llvm-pdbdump] in yaml2pdb, generate default output filename if none given.
[llvm-pdbdump] in yaml2pdb, generate default output filename if none given
Wed, May 17, 2:00 PM
inglorion closed D33296: [llvm-pdbdump] in yaml2pdb, generate default output filename if none given by committing rL303299: [llvm-pdbdump] in yaml2pdb, generate default output filename if none given.
Wed, May 17, 2:00 PM
inglorion created D33296: [llvm-pdbdump] in yaml2pdb, generate default output filename if none given.
Wed, May 17, 1:18 PM

Fri, May 12

inglorion updated the diff for D33148: add generic annotated builder and clang-with-thin-lto-windows.

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

Fri, May 12, 2:28 PM
inglorion added a comment to D33148: add generic annotated builder and clang-with-thin-lto-windows.

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

Fri, May 12, 12:54 PM
inglorion created D33148: add generic annotated builder and clang-with-thin-lto-windows.
Fri, May 12, 12:53 PM

Mon, May 1

inglorion added inline comments to D32716: Write CodeView file checksum and line information to PDB.
Mon, May 1, 2:39 PM
inglorion added inline comments to D32716: Write CodeView file checksum and line information to PDB.
Mon, May 1, 2:18 PM

Fri, Apr 28

inglorion added inline comments to D32615: [pdb, lld] Write CodeView line tables to PDB..
Fri, Apr 28, 1:38 PM
inglorion added a comment to D32615: [pdb, lld] Write CodeView line tables to PDB..

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

Fri, Apr 28, 1:36 PM
inglorion committed rL301676: limit to 2 parallel links when using thinlto.
limit to 2 parallel links when using thinlto
Fri, Apr 28, 1:30 PM
inglorion closed D31990: limit to 2 parallel links when using thinlto by committing rL301676: limit to 2 parallel links when using thinlto.
Fri, Apr 28, 1:30 PM

Thu, Apr 27

inglorion added a comment to D32615: [pdb, lld] Write CodeView line tables to PDB..

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?

Thu, Apr 27, 6:13 PM

Apr 21 2017

inglorion committed rL301045: [coff] for /msvclto, pass archive members with prevailing symbols first.
[coff] for /msvclto, pass archive members with prevailing symbols first
Apr 21 2017, 2:50 PM
inglorion closed D32317: [coff] for /msvclto, pass archive members with prevailing symbols first by committing rL301045: [coff] for /msvclto, pass archive members with prevailing symbols first.
Apr 21 2017, 2:50 PM
inglorion updated the diff for D31990: limit to 2 parallel links when using thinlto.

added message and fixed case

Apr 21 2017, 1:24 PM
inglorion updated the diff for D32317: [coff] for /msvclto, pass archive members with prevailing symbols first.

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

Apr 21 2017, 1:05 PM
inglorion added inline comments to D32317: [coff] for /msvclto, pass archive members with prevailing symbols first.
Apr 21 2017, 1:03 PM
inglorion added inline comments to D32317: [coff] for /msvclto, pass archive members with prevailing symbols first.
Apr 21 2017, 1:01 PM
inglorion updated the diff for D32317: [coff] for /msvclto, pass archive members with prevailing symbols first.

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

Apr 21 2017, 11:57 AM

Apr 20 2017

inglorion updated the diff for D32317: [coff] for /msvclto, pass archive members with prevailing symbols first.

@pcc's suggestions (thanks!)

Apr 20 2017, 6:23 PM
inglorion added inline comments to D32317: [coff] for /msvclto, pass archive members with prevailing symbols first.
Apr 20 2017, 5:56 PM
inglorion added inline comments to D31990: limit to 2 parallel links when using thinlto.
Apr 20 2017, 5:42 PM
inglorion created D32317: [coff] for /msvclto, pass archive members with prevailing symbols first.
Apr 20 2017, 3:17 PM
inglorion accepted D32235: VarStreamArrayIterator needed non-const operator* overload.

lgtm!

Apr 20 2017, 12:34 PM
inglorion added a comment to D31990: limit to 2 parallel links when using thinlto.

@mehdi_amini, still lgty?

Apr 20 2017, 11:45 AM
inglorion added a comment to D32235: VarStreamArrayIterator needed non-const operator* overload.

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 20 2017, 11:44 AM

Apr 18 2017

inglorion committed rL300616: [coff] fix test for msvclto.
[coff] fix test for msvclto
Apr 18 2017, 3:42 PM
inglorion committed rL300612: [coff] use newlines instead of spaces as separators in msvclto response file.
[coff] use newlines instead of spaces as separators in msvclto response file
Apr 18 2017, 3:13 PM
inglorion closed D32185: [coff] use newlines instead of spaces as separators in msvclto response file by committing rL300612: [coff] use newlines instead of spaces as separators in msvclto response file.
Apr 18 2017, 3:13 PM
inglorion added a comment to D32185: [coff] use newlines instead of spaces as separators in msvclto response file.

@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 18 2017, 1:19 PM
inglorion created D32185: [coff] use newlines instead of spaces as separators in msvclto response file.
Apr 18 2017, 12:59 PM

Apr 13 2017

inglorion updated the diff for D31990: limit to 2 parallel links when using thinlto.
  • No warning when not using Ninja.
  • Simplified the change.
  • Fixed buggy behavior on Linux.
Apr 13 2017, 11:47 AM

Apr 12 2017

inglorion planned changes to D31990: limit to 2 parallel links when using thinlto.

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.

Apr 12 2017, 3:59 PM
inglorion added inline comments to D31990: limit to 2 parallel links when using thinlto.
Apr 12 2017, 12:54 PM
inglorion added a comment to D31990: limit to 2 parallel links when using thinlto.

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 12 2017, 12:54 PM
inglorion created D31990: limit to 2 parallel links when using thinlto.
Apr 12 2017, 12:53 PM
inglorion committed rL300089: [coff] default to multiple parallel ThinLTO jobs.
[coff] default to multiple parallel ThinLTO jobs
Apr 12 2017, 11:48 AM
inglorion closed D31986: [coff] default to multiple parallel ThinLTO jobs by committing rL300089: [coff] default to multiple parallel ThinLTO jobs.
Apr 12 2017, 11:48 AM
inglorion created D31986: [coff] default to multiple parallel ThinLTO jobs.
Apr 12 2017, 11:24 AM

Apr 11 2017

inglorion committed rL300019: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.
ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed
Apr 11 2017, 6:56 PM
inglorion closed D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed by committing rL300019: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.
Apr 11 2017, 6:55 PM
inglorion abandoned D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed.
Apr 11 2017, 6:47 PM
inglorion added a comment to D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.

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.

Apr 11 2017, 6:21 PM
inglorion updated the diff for D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.

removed unnecessary else

Apr 11 2017, 6:17 PM
inglorion updated the diff for D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.

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.
Apr 11 2017, 6:14 PM
inglorion updated the diff for D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.

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

Apr 11 2017, 5:51 PM
inglorion updated the diff for D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.

perform comdat renaming in promoteInternals

Apr 11 2017, 5:38 PM
inglorion updated the diff for D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.

fixed bug that caused function definitions to disappear

Apr 11 2017, 5:26 PM
inglorion added inline comments to D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.
Apr 11 2017, 5:23 PM
inglorion added inline comments to D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.
Apr 11 2017, 5:13 PM
inglorion added a comment to D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed.

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.

Apr 11 2017, 4:47 PM
inglorion added a comment to D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.

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 11 2017, 4:36 PM
inglorion created D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed.
Apr 11 2017, 4:33 PM

Apr 8 2017

inglorion added a comment to rL299697: [asan] Fix dead stripping of globals on Linux..

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 8 2017, 8:37 AM

Apr 6 2017

inglorion updated the diff for D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed.

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

Apr 6 2017, 4:04 PM
inglorion updated the diff for D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed.

two-pass implementation

Apr 6 2017, 2:10 PM
inglorion added a comment to D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed.

@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 6 2017, 2:09 PM

Apr 5 2017

inglorion added inline comments to D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed.
Apr 5 2017, 5:48 PM
inglorion created D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed.
Apr 5 2017, 4:42 PM

Apr 4 2017

inglorion committed rL299496: [COFF] support /ERRORLIMIT option.
[COFF] support /ERRORLIMIT option
Apr 4 2017, 5:56 PM
inglorion closed D31688: [COFF] support /ERRORLIMIT option by committing rL299496: [COFF] support /ERRORLIMIT option.
Apr 4 2017, 5:56 PM
inglorion committed rL299491: ThinLTOBitcodeWriter: handle aliases first in filterModule.
ThinLTOBitcodeWriter: handle aliases first in filterModule
Apr 4 2017, 5:54 PM
inglorion closed D31632: ThinLTOBitcodeWriter: handle aliases first in filterModule by committing rL299491: ThinLTOBitcodeWriter: handle aliases first in filterModule.
Apr 4 2017, 5:54 PM
inglorion updated the diff for D31688: [COFF] support /ERRORLIMIT option.

Fixed issues pointed out by @ruiu (thanks!)

Apr 4 2017, 5:48 PM
inglorion updated the diff for D31632: ThinLTOBitcodeWriter: handle aliases first in filterModule.

Made changes requested by @pcc

Apr 4 2017, 5:42 PM
inglorion abandoned D31633: test for thinlto handling of internal linkage.

We will not be needing this.

Apr 4 2017, 5:22 PM
inglorion added a comment to D31688: [COFF] support /ERRORLIMIT option.

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.

Apr 4 2017, 5:22 PM
inglorion created D31688: [COFF] support /ERRORLIMIT option.
Apr 4 2017, 5:20 PM
inglorion added inline comments to D31632: ThinLTOBitcodeWriter: handle aliases first in filterModule.
Apr 4 2017, 4:27 PM
inglorion updated the diff for D31632: ThinLTOBitcodeWriter: handle aliases first in filterModule.

addressed @pcc's comments

Apr 4 2017, 4:23 PM
inglorion updated the diff for D31632: ThinLTOBitcodeWriter: handle aliases first in filterModule.

Added LLVM IR test.

Apr 4 2017, 2:04 PM

Apr 3 2017

inglorion added a comment to D31632: ThinLTOBitcodeWriter: handle aliases first in filterModule.

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.

Apr 3 2017, 5:25 PM
inglorion added a comment to D31632: ThinLTOBitcodeWriter: handle aliases first in filterModule.

@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.

Apr 3 2017, 5:21 PM