inglorion (Bob Haarman)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Jun 23

inglorion added a comment to D33961: encode YAML scalars using double quotes so all values can be expressed.

@joerg, any comments?

Fri, Jun 23, 2:20 PM

Jun 21 2017

inglorion committed rL305965: [codeview] respect signedness of APSInts when printing to YAML.
[codeview] respect signedness of APSInts when printing to YAML
Jun 21 2017, 3:32 PM
inglorion closed D34013: [codeview] respect signedness of APSInts when printing to YAML by committing rL305965: [codeview] respect signedness of APSInts when printing to YAML.
Jun 21 2017, 3:32 PM

Jun 20 2017

inglorion added a comment to D33961: encode YAML scalars using double quotes so all values can be expressed.

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

Jun 20 2017, 2:52 PM

Jun 15 2017

inglorion committed rL305486: make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects.
make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects
Jun 15 2017, 10:46 AM
inglorion closed D34065: make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects by committing rL305486: make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects.
Jun 15 2017, 10:46 AM

Jun 13 2017

inglorion added a comment to D34065: make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects.

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 13 2017, 3:50 PM

Jun 12 2017

inglorion added a comment to D34065: make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects.

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

Jun 12 2017, 3:33 PM

Jun 9 2017

inglorion updated the diff for D33961: encode YAML scalars using double quotes so all values can be expressed.

Implemented @zturner's suggestions

Jun 9 2017, 2:55 PM
inglorion created D34065: make AnnotatedBuilder use LLVMBuildFactory and depends_on_projects.
Jun 9 2017, 1:48 PM
inglorion accepted D34062: [llvm-pdbdump] Rename the tool to llvm-pdbutil.

Thanks! Assuming the tests pass, lgtm

Jun 9 2017, 11:34 AM
inglorion added inline comments to D33968: [codeview] use 32-bit integer for RelocOffset in DebugLinesSubsection.
Jun 9 2017, 10:47 AM

Jun 8 2017

inglorion committed rL305043: [codeview] use 32-bit integer for RelocOffset in DebugLinesSubsection.
[codeview] use 32-bit integer for RelocOffset in DebugLinesSubsection
Jun 8 2017, 6:18 PM
inglorion closed D33968: [codeview] use 32-bit integer for RelocOffset in DebugLinesSubsection by committing rL305043: [codeview] use 32-bit integer for RelocOffset in DebugLinesSubsection.
Jun 8 2017, 6:18 PM
inglorion accepted D33996: Improve consistency and flexibility with llvm-pdbdump subcommands.

Thanks! lgtm

Jun 8 2017, 4:03 PM
inglorion added inline comments to D33996: Improve consistency and flexibility with llvm-pdbdump subcommands.
Jun 8 2017, 3:09 PM
inglorion added inline comments to D33996: Improve consistency and flexibility with llvm-pdbdump subcommands.
Jun 8 2017, 2:44 PM
inglorion added inline comments to D34015: Allow subsections in raw output mode to be printed in the order they appear in the file..
Jun 8 2017, 2:31 PM
inglorion added a comment to D33996: Improve consistency and flexibility with llvm-pdbdump subcommands.

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?

Jun 8 2017, 11:08 AM
inglorion added a comment to D33996: Improve consistency and flexibility with llvm-pdbdump subcommands.

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

Jun 8 2017, 11:06 AM
inglorion added a comment to D33996: Improve consistency and flexibility with llvm-pdbdump subcommands.

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

Jun 8 2017, 10:24 AM
inglorion added a comment to D33996: Improve consistency and flexibility with llvm-pdbdump subcommands.

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

Jun 7 2017

inglorion added inline comments to D33961: encode YAML scalars using double quotes so all values can be expressed.
Jun 7 2017, 4:49 PM
inglorion created D34013: [codeview] respect signedness of APSInts when printing to YAML.
Jun 7 2017, 3:16 PM

Jun 6 2017

inglorion created D33968: [codeview] use 32-bit integer for RelocOffset in DebugLinesSubsection.
Jun 6 2017, 5:13 PM
inglorion added a comment to D33748: [PDB] Fix alignment of symbol record string.

This change is no longer necessary, right?

Jun 6 2017, 2:53 PM
inglorion added a comment to D33961: encode YAML scalars using double quotes so all values can be expressed.

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 6 2017, 2:47 PM
inglorion created D33961: encode YAML scalars using double quotes so all values can be expressed.
Jun 6 2017, 2:47 PM

Jun 2 2017

inglorion accepted D33858: [PDB] Fix use after free..

lgtm. Thanks!

Jun 2 2017, 5:26 PM
inglorion accepted D33807: [CodeView] Allow Debug Subsections to be read/written in any order.

lgtm

Jun 2 2017, 12:29 PM

Jun 1 2017

inglorion requested changes to D33807: [CodeView] Allow Debug Subsections to be read/written in any order.

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

Jun 1 2017, 4:01 PM
inglorion abandoned D33612: [pdb] handle checksum and line info records that are not aligned to 4 bytes.

Superseded by D33785

Jun 1 2017, 3:10 PM
inglorion accepted D33785: [CodeView] Fix alignment / padding when writing symbol records.

lgtm modulo inline comment about messsage in the assert

Jun 1 2017, 2:38 PM
inglorion added a comment to D33785: [CodeView] Fix alignment / padding when writing symbol records.

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.

Jun 1 2017, 2:34 PM
inglorion added a comment to D33785: [CodeView] Fix alignment / padding when writing symbol 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.

Jun 1 2017, 11:30 AM

May 30 2017

inglorion committed rL304258: add generic annotated builder and clang-with-thin-lto-windows.
add generic annotated builder and clang-with-thin-lto-windows
May 30 2017, 5:11 PM
inglorion closed D33148: add generic annotated builder and clang-with-thin-lto-windows by committing rL304258: add generic annotated builder and clang-with-thin-lto-windows.
May 30 2017, 5:11 PM
inglorion updated the diff for D33148: add generic annotated builder and clang-with-thin-lto-windows.

pass -j argument to lit if jobs parameter is present

May 30 2017, 5:05 PM

May 26 2017

inglorion committed rL304047: [llvm-pdbdump] pdb2yaml: add an -all option to dump everything we can.
[llvm-pdbdump] pdb2yaml: add an -all option to dump everything we can
May 26 2017, 4:46 PM
inglorion closed D33613: [llvm-pdbdump] pdb2yaml: add an -all option to dump everything we can by committing rL304047: [llvm-pdbdump] pdb2yaml: add an -all option to dump everything we can.
May 26 2017, 4:46 PM
inglorion created D33613: [llvm-pdbdump] pdb2yaml: add an -all option to dump everything we can.
May 26 2017, 4:33 PM
inglorion added a comment to D33612: [pdb] handle checksum and line info records that are not aligned to 4 bytes.

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 26 2017, 4:15 PM
inglorion created D33612: [pdb] handle checksum and line info records that are not aligned to 4 bytes.
May 26 2017, 4:12 PM

May 25 2017

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
May 25 2017, 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.
May 25 2017, 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.

May 25 2017, 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?

May 25 2017, 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
May 25 2017, 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.
May 25 2017, 11:04 AM

May 24 2017

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

stricter check

May 24 2017, 4:42 PM
inglorion added inline comments to D33475: [pdb] pad source file name buffer at the end instead of the beginning.
May 24 2017, 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

May 24 2017, 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

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

added test

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

added test

May 24 2017, 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.

May 24 2017, 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.

May 24 2017, 11:36 AM

May 23 2017

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

May 22 2017

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.

May 22 2017, 5:04 PM

May 19 2017

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

May 18 2017

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.

May 18 2017, 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?

May 18 2017, 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.

May 18 2017, 11:19 AM

May 17 2017

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

lgtm

May 17 2017, 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.

May 17 2017, 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:

May 17 2017, 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
May 17 2017, 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.
May 17 2017, 2:00 PM
inglorion created D33296: [llvm-pdbdump] in yaml2pdb, generate default output filename if none given.
May 17 2017, 1:18 PM

May 12 2017

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)

May 12 2017, 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.

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

May 1 2017

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

Apr 28 2017

inglorion added inline comments to D32615: [pdb, lld] Write CodeView line tables to PDB..
Apr 28 2017, 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.

Apr 28 2017, 1:36 PM
inglorion committed rL301676: limit to 2 parallel links when using thinlto.
limit to 2 parallel links when using thinlto
Apr 28 2017, 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.
Apr 28 2017, 1:30 PM

Apr 27 2017

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?

Apr 27 2017, 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