Page MenuHomePhabricator

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 2:49 AM (166 w, 3 d)

Recent Activity

Yesterday

jhenderson added reviewers for D76878: Implement new DW_OP_LLVM_* operations: dblaikie, ikudrin, aprantl, probinson, djtodoro, ostannard.

I've added a few reviewers who are probably better placed to review things from the DWARF/DW_OP perspective than me.

Fri, Mar 27, 4:50 AM · debug-info, Restricted Project
jhenderson added reviewers for D76877: Implement DW_CFA_LLVM_def_cfa_aspace: dblaikie, aprantl, djtodoro, ostannard, probinson, ikudrin.
Fri, Mar 27, 4:50 AM · debug-info, Restricted Project
jhenderson added a comment to D76852: [lld test] Tighten ELF/pre_init_fini_array.s test.

Looks good from my perspective. I'll leave it up to @MaskRay to approve once you've taken a look at his suggestion.

Fri, Mar 27, 4:50 AM · Restricted Project
jhenderson added a comment to D74205: [llvm-dwarfdump] Add the --show-sections-sizes option.

Do you think a regular llvm-ar archive test is sufficient for this testing, or do you think a mach-o fat archive is important to test independently? I don't see a massive need for the latter personally.

Fri, Mar 27, 4:18 AM · Restricted Project, debug-info
jhenderson accepted D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D.

LGTM, from a stylistic point of view. Not sure I can comment much on the behaviour parts that I haven't commented on. Please wait for other reviewers to approve too.

Fri, Mar 27, 3:44 AM · Restricted Project
jhenderson accepted D76706: [llvm-readobj] - Fix the crash when DT_STRTAB is broken..

LGTM, with one fix.

Fri, Mar 27, 2:38 AM · Restricted Project
jhenderson added inline comments to D75342: [obj2yaml] - Teach tool to dump program headers..
Fri, Mar 27, 2:38 AM · Restricted Project
jhenderson accepted D75833: [RISCV] Support RISC-V ELF attribute section in llvm-readobj.

LGTM. Please wait for @MaskRay to approve too, since I don't know very much about the details here.

Fri, Mar 27, 2:06 AM · Restricted Project

Thu, Mar 26

jhenderson committed rG3ff3c6986b1a: [lld][ELF] Fix error message (authored by jhenderson).
[lld][ELF] Fix error message
Thu, Mar 26, 8:40 AM
jhenderson closed D76846: [lld][ELF] Fix error message.
Thu, Mar 26, 8:39 AM · Restricted Project
jhenderson updated the diff for D76833: [CodingStandards] Document coding standard for error and warning messages.

Address review comments.

Thu, Mar 26, 8:38 AM · Restricted Project
jhenderson updated the diff for D76851: [lld][ELF][test] Improve deplib.s.

Remove unneeded test input.

Thu, Mar 26, 8:06 AM · Restricted Project
jhenderson created D76851: [lld][ELF][test] Improve deplib.s.
Thu, Mar 26, 8:06 AM · Restricted Project
jhenderson created D76846: [lld][ELF] Fix error message.
Thu, Mar 26, 7:33 AM · Restricted Project
jhenderson updated the diff for D76833: [CodingStandards] Document coding standard for error and warning messages.

Mention the static analyzer and other tools explicitly.

Thu, Mar 26, 5:55 AM · Restricted Project
jhenderson added a comment to D76834: [yaml2obj] - Add NBucket and NChain fields for the SHT_HASH section..

Looks fine to me. I'd like to see an example of it being used in a real test case, so I'm holding off on the LGTM until that point.

Thu, Mar 26, 4:50 AM
jhenderson committed rG3110ac15c513: [NFC][llvm-readobj] Refactor unique warning handler (authored by jhenderson).
[NFC][llvm-readobj] Refactor unique warning handler
Thu, Mar 26, 3:13 AM
jhenderson closed D76777: [NFC][llvm-readobj] Refactor unique warning handler.
Thu, Mar 26, 3:13 AM · Restricted Project
jhenderson created D76833: [CodingStandards] Document coding standard for error and warning messages.
Thu, Mar 26, 3:12 AM · Restricted Project
jhenderson added inline comments to D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D.
Thu, Mar 26, 2:40 AM · Restricted Project
jhenderson accepted D75131: [llvm-objdump][XCOFF][AIX] Implement -r option.

LGTM, once my last comment has been addressed and question answered (assuming that the question doesn't highlight an issue in the patch itself). Thanks for putting up with my lengthy review!

Thu, Mar 26, 2:40 AM · Restricted Project
jhenderson accepted D76684: [obj2yaml] - Refactor how we dump sections. NFCI..

LGTM, with nit.

Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson added a comment to D76733: New symbolizer option to print files relative to the compilation directory..

With all of our tools with GNU equivalents, we should avoid single letter aliases unless we can get GNU to buy into the new option too. Long names are less of an issue, as it's unlikely we'll end up with a clash.

Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson added inline comments to D76739: [llvm-objdump] Replace array_pod_sort with llvm::stable_sort.
Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson added inline comments to D75631: [llvm-objdump] Fix reliability of call target disassembling.
Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson accepted D76574: [MCInstPrinter] Pass `Address` parameter to MCOI::OPERAND_PCREL typed operands. NFC.

LGTM.

Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson accepted D76591: [PPCInstPrinter] Change printBranchOperand(calltarget) to print the target address in hexadecimal form.

LGTM too.

Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson accepted D76580: [X86InstPrinter] Change printPCRelImm to print the target address in hexadecimal form.

I think this looks okay to me. LGTM. Caveat: I don't know much about the disassembler, so I leave it up to you whether you think somebody else should review.

Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson added a comment to D76706: [llvm-readobj] - Fix the crash when DT_STRTAB is broken..

Just to make sure I understand the issue: if DT_STRTAB is small enough to fit within a segment, according to the segment's size/offset fields, we currently crash, if the size/offset fields themselves are invalid? What about if only part of the DT_STRTAB range is within the file (i.e. DT_STRTAB is less than file size but DT_STRTAB + DT_STRSZ is past the end)?

Thu, Mar 26, 2:07 AM · Restricted Project
jhenderson added a comment to D75833: [RISCV] Support RISC-V ELF attribute section in llvm-readobj.

Latest veresion basically looks fine. One suggestion and one question remaining from me.

Thu, Mar 26, 1:35 AM · Restricted Project
jhenderson accepted D74023: [RISCV] ELF attribute section for RISC-V.

I haven't really reviewed the funcional parts of this change in the attribute parser stuff, but everything else LGTM. Please wait for somebody else to review the attribute parser bits.

Thu, Mar 26, 1:35 AM · Restricted Project, Restricted Project
jhenderson added a comment to D76572: Replace `T(x)` with `reinterpret_cast<T>(x)` everywhere it means reinterpret_cast. No functional change.

Btw, @jhenderson and @rsmith, if you wanted to land your own files' pieces of this patch while waiting on whoever-it-is to approve the pieces in other files, that'd be cool. As I mentioned initially, I don't have the access necessary to land any part of it myself.

Thu, Mar 26, 1:35 AM · Restricted Project

Wed, Mar 25

jhenderson created D76777: [NFC][llvm-readobj] Refactor unique warning handler.
Wed, Mar 25, 8:37 AM · Restricted Project
jhenderson accepted D74324: Tools emit the bug report URL on crash.

LGTM.

Wed, Mar 25, 5:22 AM · Restricted Project, Restricted Project
jhenderson added a comment to D76081: [Object] object::ELFObjectFile::dynamic_symbol_begin(): skip symbol index 0.

We might want to change the iterators to use the fallible iterator pattern described in the programmer's manual. That should ensure that all clients can cope with bad iteration somehow. I'm not sure I have any particular further insight into how to proceed here beyond that. I think imitating GNU's behaviour for now is a good start, and further improvements can be in later patches.

Wed, Mar 25, 3:13 AM · Restricted Project
jhenderson added a comment to D74205: [llvm-dwarfdump] Add the --show-sections-sizes option.

Code all looks fine now, just a few minor test suggestions.

Wed, Mar 25, 2:40 AM · Restricted Project, debug-info
jhenderson accepted D75756: [llvm-objdump] Teach `llvm-objdump` dump dynamic symbols..

Two minor comments. Otherwise, looks good to me.

Wed, Mar 25, 2:40 AM · Restricted Project
jhenderson added inline comments to D76684: [obj2yaml] - Refactor how we dump sections. NFCI..
Wed, Mar 25, 2:08 AM · Restricted Project
jhenderson added a comment to D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump..

It might make sense to get one of the other reviewers formal approval before landing this.

Given @ikudrin requested changes earlier, he'll need to approve it for this patch to move to "Approved" state officially, not that it matters that much.

Wed, Mar 25, 2:08 AM · Restricted Project, debug-info
jhenderson accepted D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump..

LGTM, with one minor suggestion. It might make sense to get one of the other reviewers formal approval before landing this.

Wed, Mar 25, 2:08 AM · Restricted Project, debug-info
jhenderson added inline comments to D76675: [llvm-objcopy] Match GNU behaviour regarding file symbols.
Wed, Mar 25, 2:08 AM
jhenderson added inline comments to D74324: Tools emit the bug report URL on crash.
Wed, Mar 25, 2:08 AM · Restricted Project, Restricted Project
jhenderson added a comment to D75131: [llvm-objdump][XCOFF][AIX] Implement -r option.

Thanks for the effort. This is nearly there.

Wed, Mar 25, 2:08 AM · Restricted Project
jhenderson accepted D76572: Replace `T(x)` with `reinterpret_cast<T>(x)` everywhere it means reinterpret_cast. No functional change.

LGTM, with nit, with regards to the llvm-readobj and libObject parts. I haven't looked at the other bits though.

Wed, Mar 25, 1:35 AM · Restricted Project

Tue, Mar 24

jhenderson added inline comments to D76081: [Object] object::ELFObjectFile::dynamic_symbol_begin(): skip symbol index 0.
Tue, Mar 24, 3:44 AM · Restricted Project
jhenderson added inline comments to D75756: [llvm-objdump] Teach `llvm-objdump` dump dynamic symbols..
Tue, Mar 24, 3:44 AM · Restricted Project
jhenderson added inline comments to D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump..
Tue, Mar 24, 3:43 AM · Restricted Project, debug-info
jhenderson added inline comments to D75342: [obj2yaml] - Teach tool to dump program headers..
Tue, Mar 24, 3:12 AM · Restricted Project
jhenderson added inline comments to D75756: [llvm-objdump] Teach `llvm-objdump` dump dynamic symbols..
Tue, Mar 24, 2:39 AM · Restricted Project
jhenderson added inline comments to D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump..
Tue, Mar 24, 2:39 AM · Restricted Project, debug-info
jhenderson added inline comments to D76591: [PPCInstPrinter] Change printBranchOperand(calltarget) to print the target address in hexadecimal form.
Tue, Mar 24, 2:39 AM · Restricted Project
jhenderson added a comment to D76574: [MCInstPrinter] Pass `Address` parameter to MCOI::OPERAND_PCREL typed operands. NFC.

What, if any, are the actual behaviour changes in this patch? I don't see any test updates, but the patch isn't labelled NFC.

Tue, Mar 24, 2:39 AM · Restricted Project
jhenderson added inline comments to D76081: [Object] object::ELFObjectFile::dynamic_symbol_begin(): skip symbol index 0.
Tue, Mar 24, 2:39 AM · Restricted Project
jhenderson added inline comments to D76580: [X86InstPrinter] Change printPCRelImm to print the target address in hexadecimal form.
Tue, Mar 24, 2:07 AM · Restricted Project
jhenderson added a comment to D74023: [RISCV] ELF attribute section for RISC-V.

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

@jhenderson, yes, I found test failures in harbormaster. The failures are occurred after I rebased my patch on master branch. After digging into error messages, I found the failures are triggered by find_if(). Maybe I misuse find_if() in this patch? Do you have any idea about this?
By the way, I also found some patch, D75015, landed even harbormaster is failed. I am curious about is it a necessary condition to pass harbormaster before landing?

Tue, Mar 24, 1:35 AM · Restricted Project, Restricted Project

Mon, Mar 23

jhenderson added inline comments to D74205: [llvm-dwarfdump] Add the --show-sections-sizes option.
Mon, Mar 23, 9:15 AM · Restricted Project, debug-info
jhenderson committed rGb259ce998f55: [llvm-readobj] Derive dynamic symtab size from DT_HASH (authored by jhenderson).
[llvm-readobj] Derive dynamic symtab size from DT_HASH
Mon, Mar 23, 5:26 AM
jhenderson closed D76352: [llvm-readobj] Derive dynamic symtab size from hash table.
Mon, Mar 23, 5:26 AM · Restricted Project
jhenderson accepted D76498: [DWARF] Fix v5 debug_line parsing of prologues with many files.

LGTM, with one minor suggestion.

Mon, Mar 23, 3:48 AM · Restricted Project
jhenderson added inline comments to D75756: [llvm-objdump] Teach `llvm-objdump` dump dynamic symbols..
Mon, Mar 23, 3:48 AM · Restricted Project
jhenderson added inline comments to D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump..
Mon, Mar 23, 3:15 AM · Restricted Project, debug-info
jhenderson added inline comments to D75131: [llvm-objdump][XCOFF][AIX] Implement -r option.
Mon, Mar 23, 2:43 AM · Restricted Project
jhenderson added inline comments to D74205: [llvm-dwarfdump] Add the --show-sections-sizes option.
Mon, Mar 23, 2:43 AM · Restricted Project, debug-info
jhenderson added inline comments to D74324: Tools emit the bug report URL on crash.
Mon, Mar 23, 2:43 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D75756: [llvm-objdump] Teach `llvm-objdump` dump dynamic symbols..
Mon, Mar 23, 2:43 AM · Restricted Project
jhenderson added a comment to D76572: Replace `T(x)` with `reinterpret_cast<T>(x)` everywhere it means reinterpret_cast. No functional change.

@Quuxplusone - did you see that there are several clang-format warnings reported against these changes? Please could you fix them.

Mon, Mar 23, 2:10 AM · Restricted Project
jhenderson added a comment to D76498: [DWARF] Fix v5 debug_line parsing of prologues with many files.

I suppose this should lead to questions about whether mis-encoded ULEBs can cause parsing sadness, and are those handled in a reasonable way?

Mon, Mar 23, 1:38 AM · Restricted Project
jhenderson added inline comments to D76562: [llvm-objcopy] Improve tool selection logic to recognize llvm-strip-$major as strip.
Mon, Mar 23, 1:38 AM · Restricted Project

Fri, Mar 20

jhenderson added a comment to D74023: [RISCV] ELF attribute section for RISC-V.

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

Fri, Mar 20, 7:00 AM · Restricted Project, Restricted Project
jhenderson committed rG86b093d1a18c: [llvm-readobj] Allow syms from all sections to match stack size entries (authored by jhenderson).
[llvm-readobj] Allow syms from all sections to match stack size entries
Fri, Mar 20, 4:19 AM
jhenderson closed D76425: [llvm-readobj] Allow syms from all sections to match stack size entries.
Fri, Mar 20, 4:18 AM · Restricted Project
jhenderson added inline comments to D76352: [llvm-readobj] Derive dynamic symtab size from hash table.
Fri, Mar 20, 3:46 AM · Restricted Project
jhenderson updated the diff for D76425: [llvm-readobj] Allow syms from all sections to match stack size entries.

Make --implicit-check-not patterns tighter.

Fri, Mar 20, 3:46 AM · Restricted Project
jhenderson added a comment to D76425: [llvm-readobj] Allow syms from all sections to match stack size entries.

(Woops, one comment didn't get added yesterday).

Fri, Mar 20, 3:46 AM · Restricted Project
jhenderson updated the diff for D76352: [llvm-readobj] Derive dynamic symtab size from hash table.

Add colons to --implicit-check-not=warning (missed because they didn't appear in the diff, as they were in the old test).

Fri, Mar 20, 3:45 AM · Restricted Project
jhenderson updated the diff for D76352: [llvm-readobj] Derive dynamic symtab size from hash table.

Address review comments from @MaskRay.

Fri, Mar 20, 3:45 AM · Restricted Project
jhenderson added inline comments to D75833: [RISCV] Support RISC-V ELF attribute section in llvm-readobj.
Fri, Mar 20, 3:13 AM · Restricted Project
jhenderson added a comment to D76003: [ELF][test] Add test for --gc-sections + many sections.

Ping?

Fri, Mar 20, 3:13 AM · Restricted Project
jhenderson accepted D75671: [llvm-readobj][llvm-readelf][test] - Add a test to check how we dump relocation addends..

LGTM, with a couple of nits.

Fri, Mar 20, 2:40 AM · Restricted Project
jhenderson added a comment to D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump..

Sorry, I've been quite busy with having about 15-20 different active reviews at once, some of which are not trivial in complexity. It may take me a couple of working days to respond to things.

Fri, Mar 20, 2:40 AM · Restricted Project, debug-info
jhenderson added inline comments to D75131: [llvm-objdump][XCOFF][AIX] Implement -r option.
Fri, Mar 20, 2:40 AM · Restricted Project
jhenderson added inline comments to D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D.
Fri, Mar 20, 2:08 AM · Restricted Project

Thu, Mar 19

jhenderson updated the diff for D76425: [llvm-readobj] Allow syms from all sections to match stack size entries.

Address review comments.

Thu, Mar 19, 8:04 AM · Restricted Project
jhenderson updated the diff for D76352: [llvm-readobj] Derive dynamic symtab size from hash table.

Address review comments.

Thu, Mar 19, 8:04 AM · Restricted Project
jhenderson added inline comments to D76352: [llvm-readobj] Derive dynamic symtab size from hash table.
Thu, Mar 19, 5:21 AM · Restricted Project
jhenderson added a comment to D76410: [ELF] Don't combine SHF_LINK_ORDER sections linking different output sections.

I've created D76425 to remove the check in llvm-readobj for fully linked output. This would work as an alternative to this linker change, to fix the PR (although there is one minor caveat - see the bug comment/that review), but I don't think this linker change hurts still. The llvm-readobj patch solves the issue for the case where stack_sizes sections have all been concatenated together via linker script.

Thu, Mar 19, 4:17 AM · Restricted Project
jhenderson created D76425: [llvm-readobj] Allow syms from all sections to match stack size entries.
Thu, Mar 19, 4:17 AM · Restricted Project
jhenderson added inline comments to D76352: [llvm-readobj] Derive dynamic symtab size from hash table.
Thu, Mar 19, 4:17 AM · Restricted Project
jhenderson added inline comments to D75342: [obj2yaml] - Teach tool to dump program headers..
Thu, Mar 19, 4:17 AM · Restricted Project
jhenderson accepted D76213: [llvm-nm] Add test for `--debug-syms --dynamic`.

LGTM. @Higuoxing, would you mind writing a separate dedicated test for --dynamic? It would allow for testing the corner cases, which are outside the scope of this test file. It can be a separate patch, no problem.

Thu, Mar 19, 3:11 AM · Restricted Project
jhenderson added a comment to D74205: [llvm-dwarfdump] Add the --show-sections-sizes option.

Forgot to mention: given this is intended to be common across other file formats, and there is a need to test the isDebugSection functions, I'd recommend adding tests for COFF and/or Mach-O as well as the ELF one. They don't need to be comprehensive necessarily.

Thu, Mar 19, 3:11 AM · Restricted Project, debug-info
jhenderson accepted D76276: [Object] Add the method for checking if a section is a debug section.

LGTM, but I'd like to see at least one other change that makes use of this to be ready before you commit this, as otherwise this is just dead and untested code. I agree a unit test probably doesn't make much sense.

Thu, Mar 19, 3:11 AM · debug-info
jhenderson accepted D76282: [obj2yaml] - SHT_REL*, SHT_DYNAMIC sections: add tests to document the behavior when sh_entsize is broken..

LGTM, with comment updates.

Thu, Mar 19, 3:11 AM · Restricted Project
jhenderson accepted D76281: [obj2yaml] - Stop dumping an empty sh_info field for SHT_RELA/SHT_REL sections..

LGTM.

Thu, Mar 19, 3:11 AM · Restricted Project
jhenderson added inline comments to D74205: [llvm-dwarfdump] Add the --show-sections-sizes option.
Thu, Mar 19, 3:11 AM · Restricted Project, debug-info
jhenderson added a comment to D75342: [obj2yaml] - Teach tool to dump program headers..

@jhenderson listed many good test cases. It requires some work. I've left some notes.

  1. Standalone empty segments

GNU ld can create a p_memsz=p_filesz=0 PT_LOAD. lld doesn't.

Thu, Mar 19, 2:40 AM · Restricted Project
jhenderson added a comment to D76410: [ELF] Don't combine SHF_LINK_ORDER sections linking different output sections.

I'm happy with this idea in principle, but does it work for users who have .stack_sizes grouped in linker scripts explicitly? (I'm betting not, and yes, such linker scripts do exist).

Thu, Mar 19, 2:07 AM · Restricted Project
jhenderson added reviewers for D76394: Cleanup the plumbing for DILineInfoSpecifier. [NFC]: phosek, pcc, dblaikie, eugenis.
Thu, Mar 19, 1:35 AM · Restricted Project
jhenderson added a comment to D76250: [llvm-objdump] Only reject -long-option in objdump mode.

I was also of the opinion that "llvm-objdump" (argv[0]) and "objdump" (argv[0]) should reject single-dash -long-option.

However, @compnerd mentioned that on Mach-O platforms, -long-option may be used. Xcode uses the source code of llvm-objdump but it is not clear how they use the tool. They have patches and tests which are not upstreamed. If they provided a concrete tool name, I would like to allow --long-option when argv[0] matches that tool name (may be otool or something).

If we know how they use the tool, for example, if they always pass --macho, we can get rid of the alias -m.

Thu, Mar 19, 1:35 AM · Restricted Project

Wed, Mar 18

jhenderson updated the diff for D76352: [llvm-readobj] Derive dynamic symtab size from hash table.

Fix unchecked Expected.

Wed, Mar 18, 5:56 AM · Restricted Project