Page MenuHomePhabricator

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 2:49 AM (134 w, 4 d)

Recent Activity

Today

jhenderson added inline comments to D65633: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 3].
Mon, Aug 19, 2:31 AM · Restricted Project
jhenderson added a comment to D66358: [llvm-readobj] Fallback to PT_NOTE if file doesn't have sections.

Thanks for doing this. This was something on my radar too as an odd inconsistency.

Mon, Aug 19, 2:21 AM · Restricted Project
jhenderson added inline comments to D65190: [X86] X86ATTInstPrinter: replace markup with startMarkup/endMarkup.
Mon, Aug 19, 2:06 AM · Restricted Project
jhenderson accepted D66291: [test/Object] - Move/rewrite 2 more test cases..

LGTM.

Mon, Aug 19, 1:57 AM
jhenderson added a comment to D66063: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 5].

I'm concerned with the getHeader() call changes in ELFObjectFile, mostly because again it's introducing a very obvious point of failure. What if somebody added a new piece of functionality to the base class to get another piece of data from the header? There's a good change they'll call EF.getHeader(). Say the new function is called getFoo(): if another user then decides they want to call getFoo() on a MutableELFObject, then they'll end up using the unmodified header rather than the modified one, which could cause surprising bugs. I don't have an obvious answer to this, but I'm beginning to wonder if we should be using a different style of inheritance, one that only makes the functions available that we know to be safe from a MutableELFObject's point of view. This might be something like private inheritance. I'm not sure though, as it would mean providing shim functions to re-publicise the "safe" functions we care about. What are your thoughts?

Mon, Aug 19, 1:57 AM · Restricted Project
jhenderson accepted D66062: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 4].

LGTM, though @rupprecht should confirm too.

Mon, Aug 19, 1:57 AM · Restricted Project
jhenderson accepted D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2].

LGTM.

Mon, Aug 19, 1:31 AM · Restricted Project

Fri, Aug 16

jhenderson added inline comments to D66305: [docs] Convert remaining command guide entries from md to rst..
Fri, Aug 16, 3:43 AM · Restricted Project

Thu, Aug 15

jhenderson added inline comments to D66291: [test/Object] - Move/rewrite 2 more test cases..
Thu, Aug 15, 9:08 AM
jhenderson added inline comments to D66286: [llvm-readobj/llvm-readelf] - Improve/cleanup the error reporting API..
Thu, Aug 15, 8:58 AM · Restricted Project
jhenderson added inline comments to D66062: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 4].
Thu, Aug 15, 8:23 AM · Restricted Project
jhenderson added inline comments to D65633: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 3].
Thu, Aug 15, 4:46 AM · Restricted Project
jhenderson added inline comments to D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2].
Thu, Aug 15, 4:02 AM · Restricted Project
jhenderson accepted D65190: [X86] X86ATTInstPrinter: replace markup with startMarkup/endMarkup.

LGTM.

Thu, Aug 15, 3:44 AM · Restricted Project
jhenderson added inline comments to D66281: [llvm-objcopy][MachO] Implement --strip-all.
Thu, Aug 15, 3:39 AM · Restricted Project
jhenderson added inline comments to D66283: [llvm-objcopy][MachO] Implement --add-section.
Thu, Aug 15, 3:39 AM · Restricted Project
jhenderson added inline comments to D66282: [llvm-objcopy][MachO] Implement --remove-section.
Thu, Aug 15, 3:35 AM · Restricted Project
jhenderson added a comment to D65541: [llvm-objcopy][MachO] Implement --only-section.

This goes for all the new functionality you are adding, but make sure to review the llvm-strip and llvm-objcopy documentation (llvm/docs/CommandGuide) for any changes that need to be made. For example, if any of the switches are currently listed as "ELF specific" they need to be moved into the general switch section.

Thu, Aug 15, 3:35 AM · Restricted Project
jhenderson added a comment to D62462: [llvm-objdump] Add warning messages if disassembly + source for problematic inputs.

@mmpozulp, can I confirm that this is now in? It looks like Phabricator hasn't auto-closed this issue.

It's in now.

Thu, Aug 15, 1:53 AM · Restricted Project

Wed, Aug 14

jhenderson added a comment to D66063: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 5].

I think you need to be careful that you any methods in ELFObjectFile etc that query the header return the updated value. For example, I see there's a getStartAddress function, which returns the value of e_entry. If you change e_entry, you'll want this to return the updated value, I think.

Wed, Aug 14, 8:44 AM · Restricted Project
jhenderson added a comment to D66062: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 4].

I think some other interesting test cases are a) adding more than one symbol/section, b) adding and then removing a symbol/section, c) removing stuff and then adding stuff, showing that the iteration still works correctly.

Wed, Aug 14, 8:40 AM · Restricted Project
jhenderson added inline comments to D65633: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 3].
Wed, Aug 14, 8:27 AM · Restricted Project
jhenderson accepted D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1].

Latest changes LGTM.

Wed, Aug 14, 8:08 AM · Restricted Project
jhenderson added inline comments to D65190: [X86] X86ATTInstPrinter: replace markup with startMarkup/endMarkup.
Wed, Aug 14, 8:05 AM · Restricted Project
jhenderson accepted D65189: [MC] Support returning marked up ranges in the disassembly.

LGTM. No need to get further review from my point of view after you've fixed my remaining nit.

Wed, Aug 14, 7:59 AM · Restricted Project
jhenderson accepted D66075: [llvm-readobj][MachO] Fix section type printing.

LGTM too.

Wed, Aug 14, 6:04 AM · Restricted Project
jhenderson committed rGa8eef4e5f500: [llvm-size][test] Improve llvm-size testing (authored by jhenderson).
[llvm-size][test] Improve llvm-size testing
Wed, Aug 14, 3:21 AM
jhenderson committed rL368821: [llvm-size][test] Improve llvm-size testing.
[llvm-size][test] Improve llvm-size testing
Wed, Aug 14, 3:21 AM
jhenderson closed D66134: [llvm-size][test] Improve llvm-size testing.
Wed, Aug 14, 3:21 AM · Restricted Project

Tue, Aug 13

jhenderson added a comment to D65633: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 3].

Self note: I still need to look at coverage of the unit tests.

Tue, Aug 13, 9:05 AM · Restricted Project
jhenderson added inline comments to D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2].
Tue, Aug 13, 8:21 AM · Restricted Project
jhenderson added inline comments to D66134: [llvm-size][test] Improve llvm-size testing.
Tue, Aug 13, 7:56 AM · Restricted Project
jhenderson updated the diff for D66134: [llvm-size][test] Improve llvm-size testing.

Addressed review comments. In particular, I have removed the pre-compiled Mach-O binaries, in favour of YAML inputs, as they're not too bad.

Tue, Aug 13, 7:56 AM · Restricted Project
jhenderson accepted D65189: [MC] Support returning marked up ranges in the disassembly.

You should update your patch title and description too.

What do you think what the patch title should describe?

How about "[MC] Support returning marked up ranges in the disassembly" or "[MCInstPrinter] Add MarkupStart/MarkupEnd/WithMarkup?

Tue, Aug 13, 7:36 AM · Restricted Project
jhenderson added inline comments to D66075: [llvm-readobj][MachO] Fix section type printing.
Tue, Aug 13, 6:57 AM · Restricted Project
jhenderson added a comment to D62462: [llvm-objdump] Add warning messages if disassembly + source for problematic inputs.

@mmpozulp, can I confirm that this is now in? It looks like Phabricator hasn't auto-closed this issue.

Tue, Aug 13, 6:47 AM · Restricted Project
jhenderson accepted D66140: [llvm-objdump] - Add a relocation-xindex-symbol.test test case..

LGTM.

Tue, Aug 13, 6:39 AM · Restricted Project
jhenderson added a comment to D66134: [llvm-size][test] Improve llvm-size testing.

I'll address the comments later today hopefully.

Tue, Aug 13, 5:32 AM · Restricted Project
jhenderson accepted D66011: [llvm-readobj] - Remove 'error(Error EC)' helper..

LGTM, with the suggestions.

Tue, Aug 13, 4:37 AM · Restricted Project
jhenderson added inline comments to D66011: [llvm-readobj] - Remove 'error(Error EC)' helper..
Tue, Aug 13, 4:01 AM · Restricted Project
jhenderson added a comment to D66134: [llvm-size][test] Improve llvm-size testing.

The file renaming hasn't been reflected in my patch creation script for some reason. For reference:

  • no-input.test was originally basic.test
  • elf-berkeley.test was originally X86/elf-sizes.test
  • common.test was originally X86/test-common.s
Tue, Aug 13, 3:54 AM · Restricted Project
jhenderson created D66134: [llvm-size][test] Improve llvm-size testing.
Tue, Aug 13, 3:52 AM · Restricted Project

Mon, Aug 12

jhenderson added inline comments to D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2].
Mon, Aug 12, 9:30 AM · Restricted Project
jhenderson accepted D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1].

Aside from a comment change, this looks good to me. I'd wait for others to approve too though, and I wouldn't necessarily land it even then until some later patches are ready too, although I'm not sure about that.

Mon, Aug 12, 9:04 AM · Restricted Project
jhenderson added a comment to D65189: [MC] Support returning marked up ranges in the disassembly.

You should update your patch title and description too.

Mon, Aug 12, 8:59 AM · Restricted Project
jhenderson added a comment to D66075: [llvm-readobj][MachO] Fix section type printing.

The change looks fine to me, although I wasn't able to quickly find the list of section types anywhere to check against. Could you provide me some documentation?

Mon, Aug 12, 8:53 AM · Restricted Project
jhenderson added inline comments to D65190: [X86] X86ATTInstPrinter: replace markup with startMarkup/endMarkup.
Mon, Aug 12, 8:48 AM · Restricted Project
jhenderson committed rG3819316040ab: NFC. Remove trailing whitespace in test (authored by jhenderson).
NFC. Remove trailing whitespace in test
Mon, Aug 12, 4:40 AM
jhenderson committed rL368556: NFC. Remove trailing whitespace in test.
NFC. Remove trailing whitespace in test
Mon, Aug 12, 4:39 AM
jhenderson committed rGf23ce128fd61: [llvm-strings] Improve testing of llvm-strings (authored by jhenderson).
[llvm-strings] Improve testing of llvm-strings
Mon, Aug 12, 4:37 AM
jhenderson committed rL368555: [llvm-strings] Improve testing of llvm-strings.
[llvm-strings] Improve testing of llvm-strings
Mon, Aug 12, 4:37 AM
jhenderson closed D66015: [llvm-strings] Improve testing of llvm-strings.
Mon, Aug 12, 4:37 AM · Restricted Project
jhenderson added inline comments to D66015: [llvm-strings] Improve testing of llvm-strings.
Mon, Aug 12, 4:34 AM · Restricted Project
jhenderson added inline comments to D66015: [llvm-strings] Improve testing of llvm-strings.
Mon, Aug 12, 3:41 AM · Restricted Project
jhenderson updated the diff for D66015: [llvm-strings] Improve testing of llvm-strings.

Addressed review comments:

Mon, Aug 12, 3:41 AM · Restricted Project
jhenderson added inline comments to D66015: [llvm-strings] Improve testing of llvm-strings.
Mon, Aug 12, 3:21 AM · Restricted Project
jhenderson added inline comments to D66015: [llvm-strings] Improve testing of llvm-strings.
Mon, Aug 12, 3:07 AM · Restricted Project
jhenderson added a comment to D65991: [llvm-objcopy] Move duplicate tablegen from objcopy and strip into one file.

Copied from llvm/tools/llvm-objcopy/StripOpts.td

Phabricator is smart?

Mon, Aug 12, 2:31 AM · Restricted Project
jhenderson accepted D65372: [llvm-objcopy] Add support for response files in llvm-strip and llvm-objcopy.

LGTM, with a couple of nits. I'm okay with the refactoring being left to a different change, as long as you don't leave it a long time.

Mon, Aug 12, 2:04 AM · Restricted Project
jhenderson accepted D65991: [llvm-objcopy] Move duplicate tablegen from objcopy and strip into one file.

LGTM.

Mon, Aug 12, 2:00 AM · Restricted Project
jhenderson accepted D66036: [llvm-readobj] Downgrade 'PT_DYNAMIC segment offset + size exceeds the size of the file' from an error to a warning.

LGTM, aside from @grimar's comment.

Mon, Aug 12, 1:56 AM · Restricted Project

Fri, Aug 9

jhenderson added a comment to D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2].

Not had time to look at everything in this patch, but it feels like you don't want removal functionality to be added in this patch at the same time as basic symbol functionality. That should be a different patch, and can then include both symbol and section removal. What do you think?

Fri, Aug 9, 9:01 AM · Restricted Project
jhenderson added inline comments to D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1].
Fri, Aug 9, 8:44 AM · Restricted Project
jhenderson accepted D65799: [yaml2obj/obj2yaml][MachO] Allow setting custom section data.

LGTM. You might want @alexshap to confirm before landing this though.

Fri, Aug 9, 8:24 AM · Restricted Project
jhenderson created D66015: [llvm-strings] Improve testing of llvm-strings.
Fri, Aug 9, 8:20 AM · Restricted Project
jhenderson added inline comments to D66011: [llvm-readobj] - Remove 'error(Error EC)' helper..
Fri, Aug 9, 6:20 AM · Restricted Project
jhenderson committed rGbe39e398e98d: [llvm-readelf]Print filename for multiple inputs and fix formatting regression (authored by jhenderson).
[llvm-readelf]Print filename for multiple inputs and fix formatting regression
Fri, Aug 9, 5:32 AM
jhenderson committed rL368435: [llvm-readelf]Print filename for multiple inputs and fix formatting regression.
[llvm-readelf]Print filename for multiple inputs and fix formatting regression
Fri, Aug 9, 5:32 AM
jhenderson closed D65953: [llvm-readelf]Print filename for multiple inputs and fix formatting regression.
Fri, Aug 9, 5:32 AM · Restricted Project
jhenderson accepted D66000: [llvm-readobj] - Remove `error(llvm::Expected<T> &&E)`.

LGTM.

Fri, Aug 9, 3:31 AM · Restricted Project
jhenderson accepted D65946: [llvm-readobj] - Remove deprecated unwrapOrError(Expected<T> EO)..
Fri, Aug 9, 3:24 AM · Restricted Project
jhenderson added inline comments to D65953: [llvm-readelf]Print filename for multiple inputs and fix formatting regression.
Fri, Aug 9, 2:31 AM · Restricted Project
jhenderson added a comment to D65372: [llvm-objcopy] Add support for response files in llvm-strip and llvm-objcopy.

When you've addressed review comments, it's helpful if you mark the "Done" box in the relevant comment. That way it's obvious what hasn't been addressed. If you do it before you upload, then they will get submitted when the patch gets updated.

Fri, Aug 9, 2:25 AM · Restricted Project
jhenderson added inline comments to D65991: [llvm-objcopy] Move duplicate tablegen from objcopy and strip into one file.
Fri, Aug 9, 2:14 AM · Restricted Project
jhenderson accepted D65384: [Docs][llvm-strip] Add help text to llvm-strip rst doc.

LGTM! Thanks.

Fri, Aug 9, 1:52 AM · Restricted Project
jhenderson accepted D62462: [llvm-objdump] Add warning messages if disassembly + source for problematic inputs.

Yup, LGTM.

Fri, Aug 9, 1:45 AM · Restricted Project
jhenderson accepted D65537: Print reasonable representations of type names in llvm-nm, readelf and readobj..

LGTM.

Fri, Aug 9, 1:45 AM · Restricted Project

Thu, Aug 8

jhenderson created D65953: [llvm-readelf]Print filename for multiple inputs and fix formatting regression.
Thu, Aug 8, 7:55 AM · Restricted Project
jhenderson added a comment to D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2].

I don't think you want to be modifying the number of dynamic symbols at all. That would require you to make other changes to loadable code, which would impact addresses, and effectively require you to completely relink the program.

Thu, Aug 8, 7:28 AM · Restricted Project
jhenderson accepted D65951: [llvm-readobj] - Remove unwrapOrError(ErrorOr<T> EO) helper..

LGTM.

Thu, Aug 8, 7:09 AM · Restricted Project
jhenderson added inline comments to D65946: [llvm-readobj] - Remove deprecated unwrapOrError(Expected<T> EO)..
Thu, Aug 8, 7:05 AM · Restricted Project
jhenderson added inline comments to D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1].
Thu, Aug 8, 6:43 AM · Restricted Project
jhenderson added inline comments to D65799: [yaml2obj/obj2yaml][MachO] Allow setting custom section data.
Thu, Aug 8, 2:25 AM · Restricted Project
jhenderson added inline comments to D65191: [llvm-objdump] Implement highlighting.
Thu, Aug 8, 2:13 AM · Restricted Project
jhenderson added a reviewer for D65893: [llvm-objcopy] Allow the visibility of the start, end and size symbols created by --binary to be specified with --binary-symbol-visibility: jakehehrlich.

@jakehehrlich/@rupprecht, I looked at this offline with @chrisjackson, and it's something we would find useful. In particular, we're interested in being able to make the new symbols STV_HIDDEN, so that they don't get involved in symbol pre-emption (although I could imagine some users wanting STV_PROTECTED semantics too). Without this patch, there's no way of avoiding this as far as I know.

Thu, Aug 8, 1:53 AM · Restricted Project
jhenderson added a reviewer for D65891: [llvm-objcopy] Allow 'protected' visibility to be set when using add-symbol: jakehehrlich.

@jakehehrlich/@rupprecht, I looked at this offline with @chrisjackson, and I think it makes sense to add this functionality. I believe it's above what GNU does (at the moment), but I can't think of any reason why a user shouldn't be allowed to add STV_PROTECTED symbols, and we have a use-case for it internally.

Thu, Aug 8, 1:50 AM · Restricted Project

Wed, Aug 7

jhenderson added a comment to D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1].

Aargh, you pushed your latest update whilst I was doing my last review, so I can't see what are the latest changes!

Wed, Aug 7, 8:56 AM · Restricted Project
jhenderson added inline comments to D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1].
Wed, Aug 7, 8:53 AM · Restricted Project
jhenderson accepted D65515: [llvm-readobj/libObject] - Introduce a custom warning handler for `ELFFile<ELFT>` methods..

LGTM.

Wed, Aug 7, 8:12 AM · Restricted Project
jhenderson accepted D65446: [yaml2obj/obj2yaml] - Add a basic support for extended section indexes..

LGTM, with one nit.

Wed, Aug 7, 8:10 AM · Restricted Project
jhenderson added inline comments to D65515: [llvm-readobj/libObject] - Introduce a custom warning handler for `ELFFile<ELFT>` methods..
Wed, Aug 7, 6:24 AM · Restricted Project
jhenderson added inline comments to D65515: [llvm-readobj/libObject] - Introduce a custom warning handler for `ELFFile<ELFT>` methods..
Wed, Aug 7, 5:40 AM · Restricted Project
jhenderson added inline comments to D65191: [llvm-objdump] Implement highlighting.
Wed, Aug 7, 4:59 AM · Restricted Project
jhenderson added a comment to D65847: [llvm-readelf] --notes: move 'Data size' column left by 1.

LGTM. I assume that the 64-bit note layout (i.e. one with a 64-bit size field) works too?

binutils/readelf.c

printf (_("  %-20s %10s\tDescription\n"), _("Owner"), _("Data size"));

I think it is very unlikely that the size is larger than 32-bit...

I agree a large size is unlikely. I was more concerned with it adding leading zeroes (correctly/incorrectly) for 64-bit numbers if the format was 64-bits. That broke the layout of some other tools, if I remember correctly.

typedef struct
{
  Elf64_Word n_namesz;			/* Length of the note's name.  */
  Elf64_Word n_descsz;			/* Length of the note's descriptor.  */
  Elf64_Word n_type;			/* Type of the note.  */
} Elf64_Nhdr;

We don't have to worry about that. n_descsz is 32-bit in both ELFCLASS32 and ELFCLASS64.

Wed, Aug 7, 4:41 AM · Restricted Project
jhenderson added a comment to D65638: Switch LLVM to use 64-bit offsets (2/5).

You missed one that got added very recently! See the FIXME introduced in rL367942.

Wed, Aug 7, 4:06 AM · Restricted Project
jhenderson added a comment to D65847: [llvm-readelf] --notes: move 'Data size' column left by 1.

LGTM. I assume that the 64-bit note layout (i.e. one with a 64-bit size field) works too?

binutils/readelf.c

printf (_("  %-20s %10s\tDescription\n"), _("Owner"), _("Data size"));

I think it is very unlikely that the size is larger than 32-bit...

Wed, Aug 7, 3:29 AM · Restricted Project
jhenderson added inline comments to D62462: [llvm-objdump] Add warning messages if disassembly + source for problematic inputs.
Wed, Aug 7, 2:19 AM · Restricted Project
jhenderson accepted D65847: [llvm-readelf] --notes: move 'Data size' column left by 1.

LGTM. I assume that the 64-bit note layout (i.e. one with a 64-bit size field) works too?

Wed, Aug 7, 1:55 AM · Restricted Project
jhenderson updated subscribers of D65384: [Docs][llvm-strip] Add help text to llvm-strip rst doc.

CC'ing @wolfgangp, due to D65787 adding a new switch. One of the two patches will need updating once the other lands.

Wed, Aug 7, 1:49 AM · Restricted Project
jhenderson updated subscribers of D65787: [llvm-strip] Support --strip-sections with the llvm-strip command..

CC'ing mmpozulp due to him writing the llvm-strip doc in D65384. This option will need adding, assuming this patch lands first.

Wed, Aug 7, 1:49 AM · Restricted Project