Page MenuHomePhabricator

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 2:49 AM (142 w, 5 d)

Recent Activity

Wed, Oct 9

jhenderson added inline comments to D68146: [FileCheck] Implement --ignore-case option..
Wed, Oct 9, 9:21 AM · Restricted Project
jhenderson updated subscribers of D68645: MinidumpYAML: Add support for the memory info list stream.

FYI, I'm going to be away for 2 and a half weeks from the end of work today, so won't have time to look at these if I don't get to them later today. I have no issues with other people reviewing them. You might want to add @grimar and @MaskRay to the reviews as they've been doing a lot of work in obj2yaml/yaml2obj recently.

Wed, Oct 9, 2:20 AM · Restricted Project

Tue, Oct 8

jhenderson added a comment to D68636: [llvm-readobj] - Refine the LLVM-style output to be consistent..

Did you remember to run LLD tests? I'd expect to see changes there...

Sure. That will be a separate change, once we agree with LLVM side.

Tue, Oct 8, 8:07 AM
jhenderson added inline comments to D68636: [llvm-readobj] - Refine the LLVM-style output to be consistent..
Tue, Oct 8, 7:01 AM
jhenderson added a comment to D68636: [llvm-readobj] - Refine the LLVM-style output to be consistent..

Did you remember to run LLD tests? I'd expect to see changes there...

Tue, Oct 8, 6:42 AM
jhenderson accepted D68210: Object/minidump: Add support for the MemoryInfoList stream.

Thanks, LGTM (modulo the Minidump-specific details).

Tue, Oct 8, 4:05 AM · Restricted Project
jhenderson added inline comments to D68210: Object/minidump: Add support for the MemoryInfoList stream.
Tue, Oct 8, 1:54 AM · Restricted Project
jhenderson added inline comments to D68462: [llvm-readobj/llvm-readelf] - Add checks for GNU-style to "all.test" test case..
Tue, Oct 8, 1:45 AM · Restricted Project

Mon, Oct 7

jhenderson accepted D68462: [llvm-readobj/llvm-readelf] - Add checks for GNU-style to "all.test" test case..

LGTM.

Mon, Oct 7, 7:20 AM · Restricted Project
jhenderson added inline comments to D68462: [llvm-readobj/llvm-readelf] - Add checks for GNU-style to "all.test" test case..
Mon, Oct 7, 4:46 AM · Restricted Project
jhenderson added a comment to D68210: Object/minidump: Add support for the MemoryInfoList stream.

Can't comment too much on the file format details, but I've made some more general comments.

Mon, Oct 7, 4:36 AM · Restricted Project

Fri, Oct 4

jhenderson accepted D68383: [llvm-readelf/llvm-objdump] - Improve/refactor the implementation of SHT_LLVM_ADDRSIG section dumping..

LGTM.

Fri, Oct 4, 8:37 AM · Restricted Project
jhenderson added inline comments to D68462: [llvm-readobj/llvm-readelf] - Add checks for GNU-style to "all.test" test case..
Fri, Oct 4, 8:36 AM · Restricted Project
jhenderson added inline comments to D68383: [llvm-readelf/llvm-objdump] - Improve/refactor the implementation of SHT_LLVM_ADDRSIG section dumping..
Fri, Oct 4, 2:37 AM · Restricted Project
jhenderson added a comment to D68033: [llvm-ar] Make paths case insensitive when on windows.

Not going to review the code (I'll leave others to do that), but a quick reminder to please include the full context when uploading diffs.

Fri, Oct 4, 2:33 AM · Restricted Project
jhenderson accepted D68425: [NFC] [FileCheck] Fix init of objects in unit tests.

LGTM, since it makes things more readable. Beyond that, is there any particular motivation for this?

Fri, Oct 4, 1:11 AM · Restricted Project

Thu, Oct 3

jhenderson added inline comments to D68383: [llvm-readelf/llvm-objdump] - Improve/refactor the implementation of SHT_LLVM_ADDRSIG section dumping..
Thu, Oct 3, 6:15 AM · Restricted Project
jhenderson accepted D68333: [yaml2obj/obj2yaml] - Add support for SHT_LLVM_ADDRSIG sections..

LGTM.

Thu, Oct 3, 6:05 AM · Restricted Project
jhenderson accepted D68334: [yaml2obj] - Add a Size tag support for SHT_LLVM_ADDRSIG sections..

LGTM.

Thu, Oct 3, 6:05 AM · Restricted Project
jhenderson accepted D68386: [llvm-readobj] - Stop using a precompiled binary in all.test.

LGTM, with one request. I think we need a GNU version of this test too, but that's probably a separate change.

Thu, Oct 3, 6:01 AM · Restricted Project
jhenderson accepted D68066: [llvm-objdump] Further rearrange llvm-objdump sections for compatability.

LGTM. I'll leave @rupprecht to answer @justice_adams's comment.

Thu, Oct 3, 1:40 AM · Restricted Project

Wed, Oct 2

jhenderson added a comment to D68334: [yaml2obj] - Add a Size tag support for SHT_LLVM_ADDRSIG sections..

One suggestion. Otherwise, looks good.

Wed, Oct 2, 8:22 AM · Restricted Project
jhenderson added inline comments to D68333: [yaml2obj/obj2yaml] - Add support for SHT_LLVM_ADDRSIG sections..
Wed, Oct 2, 8:19 AM · Restricted Project
jhenderson added a comment to D68146: [FileCheck] Implement --ignore-case option..

I'm pleasantly surprised by how simple that FileCheck change is! You also need to update the FileCheck documentation (see llvm/docs/CommandGuide/FileCheck.rst) for the new option.

Wed, Oct 2, 6:57 AM · Restricted Project
jhenderson added inline comments to D68066: [llvm-objdump] Further rearrange llvm-objdump sections for compatability.
Wed, Oct 2, 1:39 AM · Restricted Project
jhenderson added inline comments to D68303: [llvm-objdump] Also suppress --dyn-relocs when --disassemble is provided.
Wed, Oct 2, 1:30 AM · Restricted Project

Tue, Oct 1

jhenderson accepted D68086: [llvm-readelf] - Report a warning when .hash section contains a chain with a cycle..

LGTM, with one nit.

Tue, Oct 1, 6:01 AM · Restricted Project
jhenderson accepted D68216: [yaml2obj] - Alow Size tag for describing SHT_HASH sections..

LGTM.

Tue, Oct 1, 5:57 AM · Restricted Project
jhenderson added inline comments to D67649: [FileCheck] Move private interface to its own header.
Tue, Oct 1, 2:05 AM · Restricted Project
jhenderson added a comment to D68249: [llvm-objdump] Don't throw error for empty dynamic section.

I would argue that this is indeed an invalid case, though objdump -p does not error.

http://www.sco.com/developers/gabi/latest/ch5.dynamic.html#dynamic_section

Except for the DT_NULL element at the end of the array, and the relative order of DT_NEEDED elements, entries may appear in any order. Tag values not appearing in the table are reserved.

I take it that there should be a DT_NULL element at the end, so this section cannot be empty. The if (Dyn.back().d_tag != ELF::DT_NULL) test below becomes slightly more complex: "dynamic section must end with a DT_NULL" to "non-empty dynamic section must end with a DT_NULL".

That said, -p emitting an error may be a bit inconvenient because -x otherwise does not emit any error.

Maybe we should test: !empty or SHT_NOBITS?

BTW, do you have an opinion on D67090?

Tue, Oct 1, 2:05 AM · Restricted Project
jhenderson accepted D65541: [llvm-objcopy][MachO] Implement --only-section.

I don't know about any Mach-O specific details, but everything else looks good to me. Best get somebody else with Mach-O expertise to confirm.

Tue, Oct 1, 1:47 AM · Restricted Project

Mon, Sep 30

jhenderson added inline comments to D68216: [yaml2obj] - Alow Size tag for describing SHT_HASH sections..
Mon, Sep 30, 10:10 AM · Restricted Project
jhenderson accepted D68085: [yaml2obj/obj2yaml] - Add support for SHT_HASH sections..

LGTM, with a couple of minor points.

Mon, Sep 30, 10:03 AM · Restricted Project
jhenderson accepted D67649: [FileCheck] Move private interface to its own header.

LGTM, but I wouldn't mind a second opinion.

Mon, Sep 30, 9:59 AM · Restricted Project
jhenderson added a comment to D68214: [yaml2obj] - Allow specifying custom Link values for SHT_HASH section..

LGTM, with one suggestion.

Mon, Sep 30, 6:19 AM · Restricted Project
jhenderson added inline comments to D67649: [FileCheck] Move private interface to its own header.
Mon, Sep 30, 6:01 AM · Restricted Project
jhenderson added a comment to D68066: [llvm-objdump] Further rearrange llvm-objdump sections for compatability.

@rupprecht A bit of a beginner to LLVM, but why add the suppression for DynamicRelocations when --disassemble is provided?

Mon, Sep 30, 5:22 AM · Restricted Project
jhenderson added inline comments to D65541: [llvm-objcopy][MachO] Implement --only-section.
Mon, Sep 30, 5:09 AM · Restricted Project
jhenderson accepted D68186: [FileCheck] Remove implementation types from API.

LGTM, with a couple of minor nits.

Mon, Sep 30, 4:31 AM · Restricted Project
jhenderson added inline comments to D67649: [FileCheck] Move private interface to its own header.
Mon, Sep 30, 4:28 AM · Restricted Project
jhenderson added inline comments to D67649: [FileCheck] Move private interface to its own header.
Mon, Sep 30, 4:12 AM · Restricted Project
jhenderson added a comment to D68146: [FileCheck] Implement --ignore-case option..

Oh, this is painful.
I'm curious - what do you think about some other options, e.g. about extending FileCheck to add support for case insensitiveness,
but maybe smb else might have a better idea

Mon, Sep 30, 3:58 AM · Restricted Project

Fri, Sep 27

jhenderson added inline comments to D68085: [yaml2obj/obj2yaml] - Add support for SHT_HASH sections..
Fri, Sep 27, 2:20 AM · Restricted Project
jhenderson added inline comments to D68086: [llvm-readelf] - Report a warning when .hash section contains a chain with a cycle..
Fri, Sep 27, 2:03 AM · Restricted Project
jhenderson added inline comments to D67998: [llvm-readobj/llvm-readelf]: Dump stack-size entry for all function symbols that have a particular value (fixes PR43245).
Fri, Sep 27, 1:59 AM
jhenderson added inline comments to D68061: [docs] Document pattern of using CHECK-SAME to skip irrelevant lines.
Fri, Sep 27, 1:22 AM · Restricted Project

Thu, Sep 26

jhenderson added inline comments to D68066: [llvm-objdump] Further rearrange llvm-objdump sections for compatability.
Thu, Sep 26, 3:18 AM · Restricted Project
jhenderson accepted D67656: [llvm-objcopy] Add --set-section-alignment.

Latest changes LGTM.

Thu, Sep 26, 2:44 AM · Restricted Project
jhenderson added inline comments to D67656: [llvm-objcopy] Add --set-section-alignment.
Thu, Sep 26, 1:55 AM · Restricted Project
jhenderson added inline comments to D68061: [docs] Document pattern of using CHECK-SAME to skip irrelevant lines.
Thu, Sep 26, 1:52 AM · Restricted Project
jhenderson added reviewers for D68061: [docs] Document pattern of using CHECK-SAME to skip irrelevant lines: thopre, MaskRay, probinson.
Thu, Sep 26, 1:50 AM · Restricted Project
jhenderson added a comment to D67998: [llvm-readobj/llvm-readelf]: Dump stack-size entry for all function symbols that have a particular value (fixes PR43245).

Yes, good observation. There is also the question what to do if the sizes of 2 different entries with the same address don't match. This should probably elicit a warning.

I agree this should probably print a warning, and I think it then should do a best guess print of the stack sizes. I have three options, in rough order of preference for this:

  1. Display the entries with symbols in the order they appear in the symbol table, i.e. if foo and bar appear in that order in the symbol table, with the same address, I'd print foo with the first stack size and bar with the second. This is probably a reasonable guess based on what the linker might have done (putting the symbols in order they are discovered, same as the stack sizes entries).
  2. Display only the largest entry, but once per symbol with the same address. In practice, this is probably just as good. It doesn't really make sense for two symbols to have the same address but different stack sizes: that would imply that the same bit of code has different possible sizes, which doesn't really make sense.
  3. Display the first entry size, for each symbol. This is probably the simplest.
Thu, Sep 26, 1:50 AM

Wed, Sep 25

jhenderson committed rG12e309992121: [docs][llvm-strings] Clarify "printable character" wording (authored by jhenderson).
[docs][llvm-strings] Clarify "printable character" wording
Wed, Sep 25, 6:10 AM
jhenderson committed rG4dd9b2faec5a: [docs][llvm-strip] Update llvm-strip doc to better match llvm-objcopy's (authored by jhenderson).
[docs][llvm-strip] Update llvm-strip doc to better match llvm-objcopy's
Wed, Sep 25, 6:10 AM
jhenderson committed rL372865: [docs][llvm-strings] Clarify "printable character" wording.
[docs][llvm-strings] Clarify "printable character" wording
Wed, Sep 25, 6:09 AM
jhenderson closed D68016: [docs][llvm-strings] Clarify "printable character" wording.
Wed, Sep 25, 6:09 AM · Restricted Project
jhenderson committed rL372864: [docs][llvm-strip] Update llvm-strip doc to better match llvm-objcopy's.
[docs][llvm-strip] Update llvm-strip doc to better match llvm-objcopy's
Wed, Sep 25, 6:09 AM
jhenderson accepted D68012: [llvm-readobj/llvm-readelf] - .stack_sizes: demangle symbol names in warnings reported..

LGTM, thanks!

Wed, Sep 25, 6:08 AM · Restricted Project
jhenderson accepted D68018: [yaml2elf] - Support describing .stack_sizes sections using unique suffixes..

LGTM, with two suggestions.

Wed, Sep 25, 4:31 AM · Restricted Project
jhenderson accepted D67958: [yaml2obj] - Add a Size field for StackSizesSection..

LGTM.

Wed, Sep 25, 3:58 AM · Restricted Project
jhenderson created D68016: [docs][llvm-strings] Clarify "printable character" wording.
Wed, Sep 25, 3:55 AM · Restricted Project
jhenderson added a comment to D67962: [llvm-readobj] - Don't crash when dumping .stack_sizes and unable to find a relocation resolver..

I'm beginning to think that stack-sizes.test needs breaking up into smaller tests...!

I would probably start from splitting all the warning/error testing to stack-sizes-err.test.
They seem to take more than half (and still do not test all of the possible messages).

Wed, Sep 25, 3:16 AM · Restricted Project
jhenderson added inline comments to D68012: [llvm-readobj/llvm-readelf] - .stack_sizes: demangle symbol names in warnings reported..
Wed, Sep 25, 3:13 AM · Restricted Project
jhenderson accepted D67962: [llvm-readobj] - Don't crash when dumping .stack_sizes and unable to find a relocation resolver..

I'm beginning to think that stack-sizes.test needs breaking up into smaller tests...! This change LGTM anyway.

Wed, Sep 25, 2:10 AM · Restricted Project
jhenderson added a comment to D67759: [llvm-readobj] - Simplify stack-sizes.test test case..

The latest changes look good themselves, but should this extra issue you've uncovered have a direct yaml2obj test?

Wed, Sep 25, 2:07 AM · Restricted Project
jhenderson added inline comments to D67656: [llvm-objcopy] Add --set-section-alignment.
Wed, Sep 25, 2:00 AM · Restricted Project
jhenderson added a comment to D67998: [llvm-readobj/llvm-readelf]: Dump stack-size entry for all function symbols that have a particular value (fixes PR43245).

One issue that just occurred to me, is what happens if you still have the stack size entry in this case? Something like:

Wed, Sep 25, 1:55 AM
jhenderson added a reviewer for D67998: [llvm-readobj/llvm-readelf]: Dump stack-size entry for all function symbols that have a particular value (fixes PR43245): MaskRay.
Wed, Sep 25, 1:46 AM

Tue, Sep 24

jhenderson committed rG1b103864eea2: [docs][llvm-strip][llvm-objcopy] Improve wording and fix highlighting (authored by jhenderson).
[docs][llvm-strip][llvm-objcopy] Improve wording and fix highlighting
Tue, Sep 24, 6:44 AM
jhenderson committed rL372754: [docs][llvm-strip][llvm-objcopy] Improve wording and fix highlighting.
[docs][llvm-strip][llvm-objcopy] Improve wording and fix highlighting
Tue, Sep 24, 6:43 AM
jhenderson committed rGeefbc358eb8f: [docs][llvm-size] Fix typo (authored by jhenderson).
[docs][llvm-size] Fix typo
Tue, Sep 24, 6:14 AM
jhenderson committed rL372750: [docs][llvm-size] Fix typo.
[docs][llvm-size] Fix typo
Tue, Sep 24, 6:14 AM
jhenderson added inline comments to D67958: [yaml2obj] - Add a Size field for StackSizesSection..
Tue, Sep 24, 6:07 AM · Restricted Project
jhenderson accepted D67757: [yaml2obj/obj2yaml] - Add support for .stack_sizes sections..

LGTM, with one minor comment.

Tue, Sep 24, 6:04 AM · Restricted Project
jhenderson added a reviewer for D67689: [llvm-objcopy] Add support for --gap-fill and --pad-to options: jakehehrlich.

One major question to answer is how should --gap-fill interact with the preserving of segment contents that llvm-objcopy follows when segment contents are not covered by existing sections (see llvm/test/tools/llvm-objcopy/preserve-segment-contents.test for an example of the behaviour). I'd be inclined to have --gap-fill overwrite the data in this case, in which case a test should be written to show that the old data is no longer written in this case (see below for more details).

Tue, Sep 24, 3:52 AM · Restricted Project
jhenderson accepted D67759: [llvm-readobj] - Simplify stack-sizes.test test case..

LGTM.

Tue, Sep 24, 3:52 AM · Restricted Project
jhenderson accepted D67656: [llvm-objcopy] Add --set-section-alignment.

Looks good to me, but as agreed, let's wait for the GNU objcopy discussion to reach a conclusion before committing. I've posted there expressing my view. FWIW, our proprietary equivalent to objcopy uses raw values for this, rather than powers of two stuff, so the behaviour proposed in this patch would make life easier for them if and when they migrate over.

Tue, Sep 24, 2:04 AM · Restricted Project

Mon, Sep 23

jhenderson accepted D67139: [llvm-objcopy] Refactor ELF-specific config out to ELFCopyConfig. NFC..

LGTM, thanks!

Mon, Sep 23, 8:35 AM · Restricted Project
jhenderson added a comment to D67689: [llvm-objcopy] Add support for --gap-fill and --pad-to options.

Thinks for the patch! I haven't had time to review the meat of it yet, but will try to get back to it in the next couple of days. One quick question in the meantime: what's the motivation behind this? Is it to improve GNU compatibility?

Mon, Sep 23, 8:29 AM · Restricted Project
jhenderson added inline comments to D67757: [yaml2obj/obj2yaml] - Add support for .stack_sizes sections..
Mon, Sep 23, 8:14 AM · Restricted Project
jhenderson added a comment to D67656: [llvm-objcopy] Add --set-section-alignment.

I'm very happy to see this change, but I hesitate to diverge in behaviour from GNU's behaviour. I certainly prefer --set-section-alginment to set it to an arbitrary alignment though, so I think we need to lean on the GNU maintainers to change their end.

Mon, Sep 23, 7:46 AM · Restricted Project

Tue, Sep 17

jhenderson committed rG778a5e57349e: [docs] Make --version text more correct (authored by jhenderson).
[docs] Make --version text more correct
Tue, Sep 17, 4:44 AM
jhenderson committed rL372107: [docs] Make --version text more correct.
[docs] Make --version text more correct
Tue, Sep 17, 4:44 AM
jhenderson closed D67618: [docs] Make --version text more correct.
Tue, Sep 17, 4:44 AM · Restricted Project
jhenderson accepted D67652: [yaml2obj/obj2yaml] - Allow setting an arbitrary values for e_machine..

LGTM, with two minor suggestions.

Tue, Sep 17, 4:39 AM · Restricted Project
jhenderson accepted D67615: [obj2yaml] - Support PPC64 relocation types..

LGTM.

Tue, Sep 17, 3:42 AM · Restricted Project
jhenderson added a comment to D67328: [llvm-readobj] Warn user when dumping not supported version of stack map (PR38278)..

Looks good, once @grimar's comments have been addressed.

Tue, Sep 17, 2:17 AM · Restricted Project
jhenderson accepted D67617: [llvm-readobj] - Test PPC64 relocations properly..

LGTM.

Tue, Sep 17, 2:11 AM · Restricted Project
jhenderson added a comment to D67615: [obj2yaml] - Support PPC64 relocation types..

Looks good, aside from a concern with the error case.

Tue, Sep 17, 2:10 AM · Restricted Project
jhenderson accepted D67038: [llvm-ar] Removes repetition in the error message.

Thanks, LGTM.

Tue, Sep 17, 2:01 AM · Restricted Project
jhenderson accepted D67560: [llvm-ar] Parse 'h' and '-h': display help and exit.

LGTM.

Tue, Sep 17, 2:01 AM · Restricted Project

Mon, Sep 16

jhenderson committed rL371987: Request github commit access.
Request github commit access
Mon, Sep 16, 7:03 AM
jhenderson committed rG75b6279c5e7b: [docs][llvm-strings] Write llvm-strings documentation (authored by jhenderson).
[docs][llvm-strings] Write llvm-strings documentation
Mon, Sep 16, 6:59 AM
jhenderson committed rL371984: [docs][llvm-strings] Write llvm-strings documentation.
[docs][llvm-strings] Write llvm-strings documentation
Mon, Sep 16, 6:54 AM
jhenderson closed D67554: [docs][llvm-strings] Write llvm-strings documentation.
Mon, Sep 16, 6:54 AM · Restricted Project
jhenderson created D67618: [docs] Make --version text more correct.
Mon, Sep 16, 6:53 AM · Restricted Project
jhenderson updated the diff for D67554: [docs][llvm-strings] Write llvm-strings documentation.

Address review comments:

  • Reword introduction to be a bit more concise, and more closely match the POSIX description.
  • Remove redundant comment about there being differences to GNU.
  • Change --radix=x example to use -t x.
Mon, Sep 16, 6:31 AM · Restricted Project
jhenderson committed rGe8ed932683ee: [docs][llvm-size] Write llvm-size documentation (authored by jhenderson).
[docs][llvm-size] Write llvm-size documentation
Mon, Sep 16, 6:21 AM
jhenderson committed rL371983: [docs][llvm-size] Write llvm-size documentation.
[docs][llvm-size] Write llvm-size documentation
Mon, Sep 16, 6:21 AM
jhenderson closed D67555: [docs][llvm-size] Write llvm-size documentation.
Mon, Sep 16, 6:21 AM · Restricted Project