Page MenuHomePhabricator

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

jhenderson created D58543: [yaml2obj]Re-allow dynamic sections to have raw content.
Fri, Feb 22, 4:21 AM · Restricted Project
jhenderson added a comment to D58510: [yaml2obj]Allow explicit symbol indexes in relocations and emit error for bad names.

I just ran into https://bugs.llvm.org/show_bug.cgi?id=40818, which is preventing me landing this change as is. I could modify the llvm-objcopy compress-debug-sections.yaml file to not reference a symbol at all, but I think that loses some of the intention of that test (plus it would just hide the bug).

Fri, Feb 22, 3:52 AM · Restricted Project
jhenderson committed rG0cc32dd4a134: [ELF][test]Remove unnecessary empty symbol references in yaml/add missing… (authored by jhenderson).
[ELF][test]Remove unnecessary empty symbol references in yaml/add missing…
Fri, Feb 22, 3:26 AM
jhenderson committed rLLD354666: [ELF][test]Remove unnecessary empty symbol references in yaml/add missing….
[ELF][test]Remove unnecessary empty symbol references in yaml/add missing…
Fri, Feb 22, 3:26 AM
jhenderson committed rL354666: [ELF][test]Remove unnecessary empty symbol references in yaml/add missing….
[ELF][test]Remove unnecessary empty symbol references in yaml/add missing…
Fri, Feb 22, 3:26 AM
jhenderson closed D58508: [ELF][test]Remove unnecessary empty symbol references in yaml/add missing symbols for relocs.
Fri, Feb 22, 3:26 AM · Restricted Project
jhenderson updated the diff for D58510: [yaml2obj]Allow explicit symbol indexes in relocations and emit error for bad names.

Address review comments.

Fri, Feb 22, 2:52 AM · Restricted Project
jhenderson added inline comments to D58510: [yaml2obj]Allow explicit symbol indexes in relocations and emit error for bad names.
Fri, Feb 22, 2:50 AM · Restricted Project
jhenderson added a comment to D54697: [llvm-objdump] Add `Version References` dumper.

I'll take a look at this next week, if nobody else does in the meantime.

Fri, Feb 22, 1:41 AM · Restricted Project
jhenderson added a comment to D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start.

I agree with @rupprecht on this. It gets even more involved when multiple build scripts start interacting with each other, as it may not even be clear to the developer that they are using both switches. I would be happy either with --change-start changing the value set by --set-start, or a warning or error.

Fri, Feb 22, 1:37 AM

Thu, Feb 21

jhenderson added a parent revision for D58510: [yaml2obj]Allow explicit symbol indexes in relocations and emit error for bad names: D58508: [ELF][test]Remove unnecessary empty symbol references in yaml/add missing symbols for relocs.
Thu, Feb 21, 7:45 AM · Restricted Project
jhenderson added a child revision for D58508: [ELF][test]Remove unnecessary empty symbol references in yaml/add missing symbols for relocs: D58510: [yaml2obj]Allow explicit symbol indexes in relocations and emit error for bad names.
Thu, Feb 21, 7:45 AM · Restricted Project
Herald added a reviewer for D58510: [yaml2obj]Allow explicit symbol indexes in relocations and emit error for bad names: alexshap.
Thu, Feb 21, 7:44 AM · Restricted Project
jhenderson created D58508: [ELF][test]Remove unnecessary empty symbol references in yaml/add missing symbols for relocs.
Thu, Feb 21, 7:40 AM · Restricted Project
jhenderson accepted D58498: [obj2yaml] - Do not miss section index for special symbols..

LGTM.

Thu, Feb 21, 7:31 AM · Restricted Project
jhenderson added inline comments to D58498: [obj2yaml] - Do not miss section index for special symbols..
Thu, Feb 21, 7:31 AM · Restricted Project
jhenderson added inline comments to D58498: [obj2yaml] - Do not miss section index for special symbols..
Thu, Feb 21, 6:39 AM · Restricted Project
jhenderson committed rGe9461a716d2b: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider… (authored by jhenderson).
[llvm-readobj]Add testing for ELF symbol and section table printing for a wider…
Thu, Feb 21, 4:48 AM
jhenderson committed rL354577: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider….
[llvm-readobj]Add testing for ELF symbol and section table printing for a wider…
Thu, Feb 21, 4:48 AM
jhenderson closed D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values.
Thu, Feb 21, 4:48 AM · Restricted Project
jhenderson updated the diff for D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values.

Update comment. Rebase for new yaml2obj support for SHT_GNU_verdef sections.

Thu, Feb 21, 4:42 AM · Restricted Project
jhenderson added a comment to D58499: [CommandLine] Do not crash if an option has both ValueRequired and Grouping..

According to the documentation (https://llvm.org/docs/CommandLine.html#controlling-other-formatting-options) this is illegal even for the last value:

Note that cl::Grouping options cannot have values.
Thu, Feb 21, 4:00 AM · Restricted Project
jhenderson added inline comments to D58498: [obj2yaml] - Do not miss section index for special symbols..
Thu, Feb 21, 3:53 AM · Restricted Project
jhenderson updated the diff for D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values.
  • Add more comments.
  • Rebase to remove FIXMEs relating to SHT_MIPS_DWARF.
  • Rebase to add required fields for verneed sections.
Thu, Feb 21, 3:29 AM · Restricted Project
jhenderson accepted D58496: [llvm-readobj] Change "SHT_MIPS_DWARF" to "MIPS_DWARF".

Okay, LGTM.

Thu, Feb 21, 3:18 AM · Restricted Project
jhenderson committed rG6561a823d26f: [llvm-readobj]Test basic command-line handling (authored by jhenderson).
[llvm-readobj]Test basic command-line handling
Thu, Feb 21, 3:01 AM
jhenderson committed rL354567: [llvm-readobj]Test basic command-line handling.
[llvm-readobj]Test basic command-line handling
Thu, Feb 21, 3:00 AM
jhenderson closed D58455: [llvm-readobj]Test basic command-line handling.
Thu, Feb 21, 3:00 AM · Restricted Project
jhenderson committed rG67bdfb0a5973: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE (authored by jhenderson).
[yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE
Thu, Feb 21, 2:58 AM
jhenderson committed rL354566: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.
[yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE
Thu, Feb 21, 2:57 AM
jhenderson closed D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.
Thu, Feb 21, 2:56 AM · Restricted Project
jhenderson added a parent revision for D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values: D58496: [llvm-readobj] Change "SHT_MIPS_DWARF" to "MIPS_DWARF".
Thu, Feb 21, 2:49 AM · Restricted Project
jhenderson added a child revision for D58496: [llvm-readobj] Change "SHT_MIPS_DWARF" to "MIPS_DWARF": D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values.
Thu, Feb 21, 2:49 AM · Restricted Project
jhenderson added a comment to D58496: [llvm-readobj] Change "SHT_MIPS_DWARF" to "MIPS_DWARF".

I'd normally ask for a test, but as D58457 has yet to land, I'll fix that up!

Thu, Feb 21, 2:39 AM · Restricted Project
jhenderson updated the diff for D58455: [llvm-readobj]Test basic command-line handling.

Add comments.

Thu, Feb 21, 2:25 AM · Restricted Project
jhenderson accepted D58296: [llvm-objcopy] Make removeSectionReferences batched.

I'm surprised at just how much slower we are than GNU objcopy still, but this is definitely a big win.

Thu, Feb 21, 2:08 AM · Restricted Project
jhenderson accepted D57680: [llvm-objdump] Implement `-Mreg-names-raw`/`-std` options..

LGTM, although I don't really know anything about the ARM register stuff at all. Perhaps worth getting a second person to review before landing.

Thu, Feb 21, 2:00 AM · Restricted Project
jhenderson added inline comments to D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start.
Thu, Feb 21, 1:58 AM

Wed, Feb 20

jhenderson committed rGbbc9ed5eefb3: [llvm-readelf]Test a couple of corner-cases for --section-mapping (authored by jhenderson).
[llvm-readelf]Test a couple of corner-cases for --section-mapping
Wed, Feb 20, 9:24 AM
jhenderson added a parent revision for D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values: D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.
Wed, Feb 20, 9:24 AM · Restricted Project
jhenderson added a child revision for D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE: D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values.
Wed, Feb 20, 9:24 AM · Restricted Project
jhenderson committed rL354484: [llvm-readelf]Test a couple of corner-cases for --section-mapping.
[llvm-readelf]Test a couple of corner-cases for --section-mapping
Wed, Feb 20, 9:21 AM
jhenderson closed D58456: [llvm-readelf]Test a couple of corner-cases for --section-mapping.
Wed, Feb 20, 9:21 AM · Restricted Project
jhenderson created D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values.
Wed, Feb 20, 9:06 AM · Restricted Project
jhenderson retitled D58457: [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values from [llvm-readobj]Add testing for ELF symbol table printing for a wider range of values to [llvm-readobj]Add testing for ELF symbol and section table printing for a wider range of values.
Wed, Feb 20, 9:06 AM · Restricted Project
jhenderson created D58456: [llvm-readelf]Test a couple of corner-cases for --section-mapping.
Wed, Feb 20, 9:01 AM · Restricted Project
jhenderson created D58455: [llvm-readobj]Test basic command-line handling.
Wed, Feb 20, 8:56 AM · Restricted Project
jhenderson added a comment to D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.

Thanks @grimar! I'm going to sit on this for a day or two and see if any other reviewers want to chime in before I go ahead.

Wed, Feb 20, 7:37 AM · Restricted Project
jhenderson committed rG1498a59d9b28: [obj2yaml][yaml2obj]Locate all .yaml and .test tests (authored by jhenderson).
[obj2yaml][yaml2obj]Locate all .yaml and .test tests
Wed, Feb 20, 7:14 AM
jhenderson committed rL354474: [obj2yaml][yaml2obj]Locate all .yaml and .test tests.
[obj2yaml][yaml2obj]Locate all .yaml and .test tests
Wed, Feb 20, 7:13 AM
jhenderson closed D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.
Wed, Feb 20, 7:13 AM · Restricted Project
jhenderson added inline comments to D58437: [yaml2obj][obj2yaml] - Support SHT_GNU_verdef ( .gnu.version_d) section..
Wed, Feb 20, 7:10 AM · Restricted Project
jhenderson added a comment to D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.

By the way, I could see a use for a validate/no-validate option in other cases. Perhaps with the Index field it would validate that the index is either a valid section index or a reserved value.

Wed, Feb 20, 7:09 AM · Restricted Project
jhenderson added a comment to D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.

I thought before about some kind of conflict we might see in situations like this.
From one side we might want to keep (and maybe improve) the validating because it is probably a useful thing in general.
But we still want to have a way to produce invalid outputs when we want, as this patch does.

Should we instead of removing the validation code add an option to yaml2obj, something like -no-validate?

Hmm... interesting thought. In this case, I think if you've explicitly used Index, you probably know what you are doing by specifying an explicit section index outside the reserved range, instead of using one of the SHN_* values or using the Section field. I could be persuaded otherwise though. On a related note, without this change, you cannot specify Index: SHN_UNDEF, because SHN_UNDEF is less than SHN_LORESERVE. It's not strictly necessary, because the default is undefined, but it's still slightly weird behaviour.

Wed, Feb 20, 7:07 AM · Restricted Project
jhenderson updated the diff for D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.

Explicitly only accept .test and .yaml tests.

Wed, Feb 20, 7:04 AM · Restricted Project
jhenderson added a comment to D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.

Yes, you're probably right. I'll do that instead.

Wed, Feb 20, 7:01 AM · Restricted Project
jhenderson accepted D58437: [yaml2obj][obj2yaml] - Support SHT_GNU_verdef ( .gnu.version_d) section..

LGTM.

Wed, Feb 20, 6:57 AM · Restricted Project
jhenderson requested review of D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.

Turns out that the yaml2obj config file was also broken, and I just traded one bad thing for another. The suffixes list was being completely overwritten by a list containing only '.yaml', so only tests ending in .yaml were being run. This meant that we replaced 3 running tests with 5, when it should in fact be 8 in obj2yaml. I've fixed the yaml2obj config here too, increasing the number of tests from 27 to 29.

Wed, Feb 20, 6:31 AM · Restricted Project
jhenderson updated the diff for D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.

Update config files.

Wed, Feb 20, 6:29 AM · Restricted Project
jhenderson created D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.
Wed, Feb 20, 6:22 AM · Restricted Project
jhenderson accepted D58441: [yaml2obj] - Simplify implementation. NFCI..

LGTM.

Wed, Feb 20, 5:46 AM · Restricted Project
jhenderson added a comment to D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.

Oops, didn't see your later responses. Thanks!

Wed, Feb 20, 5:40 AM · Restricted Project
jhenderson updated the diff for D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.

Not quite sure what was/is going on. I ran my diff script incorrectly before, hence the "C:" bit. I don't know why this file is showing up as modified rather than added. I've uploaded a diff with the path cleaned up.

Wed, Feb 20, 5:40 AM · Restricted Project
jhenderson created D58439: [obj2yaml][yaml2obj]Locate all tests, including .yaml and .test.
Wed, Feb 20, 4:34 AM · Restricted Project
jhenderson added inline comments to D58234: [llvm-objcopy] Add --add-symbol.
Wed, Feb 20, 4:00 AM
jhenderson added inline comments to D57680: [llvm-objdump] Implement `-Mreg-names-raw`/`-std` options..
Wed, Feb 20, 3:56 AM · Restricted Project
jhenderson added inline comments to D58416: [llvm-cxxfilt] Split and demangle stdin input on certain non-alphanumerics..
Wed, Feb 20, 2:25 AM · Restricted Project
jhenderson accepted D58316: [llvm-objcopy][NFC] More error cleanup.

LGTM.

Wed, Feb 20, 2:16 AM · Restricted Project
jhenderson added inline comments to D58296: [llvm-objcopy] Make removeSectionReferences batched.
Wed, Feb 20, 2:08 AM · Restricted Project
jhenderson added inline comments to D58234: [llvm-objcopy] Add --add-symbol.
Wed, Feb 20, 2:00 AM
jhenderson added inline comments to D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start.
Wed, Feb 20, 1:52 AM

Tue, Feb 19

jhenderson committed rGd82914c8d271: [yaml2obj][obj2yaml] Remove section type range markers from allowed mappings… (authored by jhenderson).
[yaml2obj][obj2yaml] Remove section type range markers from allowed mappings…
Tue, Feb 19, 8:22 AM
jhenderson committed rL354344: [yaml2obj][obj2yaml] Remove section type range markers from allowed mappings….
[yaml2obj][obj2yaml] Remove section type range markers from allowed mappings…
Tue, Feb 19, 8:22 AM
jhenderson closed D58383: [yaml2obj][obj2yaml] Remove section type range markers from allowed mappings and support hex values.
Tue, Feb 19, 8:22 AM · Restricted Project
jhenderson created D58383: [yaml2obj][obj2yaml] Remove section type range markers from allowed mappings and support hex values.
Tue, Feb 19, 7:15 AM · Restricted Project
jhenderson added inline comments to D58280: [yaml2obj][obj2yaml] - Support SHT_GNU_versym (.gnu.version) section..
Tue, Feb 19, 2:17 AM · Restricted Project
jhenderson accepted D58119: [obj2yaml][yaml2obj] - Add support of parsing/dumping of the .gnu.version_r section..

LGTM.

Tue, Feb 19, 2:15 AM · Restricted Project
jhenderson accepted D58280: [yaml2obj][obj2yaml] - Support SHT_GNU_versym (.gnu.version) section..

LGTM, with two small comments.

Tue, Feb 19, 2:08 AM · Restricted Project
jhenderson accepted D58168: [yaml2obj] - Do not ignore explicit addresses for .dynsym and .dynstr.

LGTM.

Tue, Feb 19, 2:04 AM · Restricted Project
jhenderson accepted D58174: [yaml2obj] - Do not skip zeroes blocks if there are relocations against them..

LGTM.

Tue, Feb 19, 1:56 AM · Restricted Project
jhenderson added inline comments to D57680: [llvm-objdump] Implement `-Mreg-names-raw`/`-std` options..
Tue, Feb 19, 1:54 AM · Restricted Project
jhenderson added inline comments to D58119: [obj2yaml][yaml2obj] - Add support of parsing/dumping of the .gnu.version_r section..
Tue, Feb 19, 1:39 AM · Restricted Project
jhenderson added a comment to D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start.
  1. What's the use case for this?
  2. What's the interaction for program headers?

    For relocatable files I'd expect addresses to be ignored and for program headers I'd like to put more thought into this. In general, I'd like to take a stance that we not implement flags that we don't have a use case for.
Tue, Feb 19, 1:38 AM

Mon, Feb 18

jhenderson added inline comments to D58280: [yaml2obj][obj2yaml] - Support SHT_GNU_versym (.gnu.version) section..
Mon, Feb 18, 9:20 AM · Restricted Project
jhenderson added a comment to D58119: [obj2yaml][yaml2obj] - Add support of parsing/dumping of the .gnu.version_r section..

That was needed in this patch since .gnu.version_r adds strings to .dynsym.

I hope you're not adding any strings to .dynsym ;-) Also, your link in the description points to verdef sections, rather than verneed.

Mon, Feb 18, 9:14 AM · Restricted Project
jhenderson added a reviewer for D57904: [llvm-objdump] Allow short options to be grouped: rafauler.

Before relanding this @ormris, it's probably worth getting @rafauler to confirm he's okay with the option name change, as he was the original one to add it. I've added him as a reviewer. Personally, I think the justification is good, but if it is going to cause horrendous pain, perhaps we should consider alternatives?

Mon, Feb 18, 7:33 AM · Restricted Project
jhenderson added a comment to D58316: [llvm-objcopy][NFC] More error cleanup.

I guess this is okay, but I'm wondering if it's gone too far? The majority of cases you've changed here are in the driver layer, so wouldn't likely ever be librarified. What's the motivation for this round?

Mon, Feb 18, 7:26 AM · Restricted Project
jhenderson added a comment to D58296: [llvm-objcopy] Make removeSectionReferences batched.

Don't know if you're planning on using the description as your commit message (I usually do), but there's a typo in it: abyssmal -> abysmal.

Mon, Feb 18, 7:16 AM · Restricted Project
jhenderson added inline comments to D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start.
Mon, Feb 18, 7:10 AM
jhenderson added inline comments to D58234: [llvm-objcopy] Add --add-symbol.
Mon, Feb 18, 7:10 AM
jhenderson added inline comments to D58234: [llvm-objcopy] Add --add-symbol.
Mon, Feb 18, 7:01 AM
jhenderson added inline comments to D58234: [llvm-objcopy] Add --add-symbol.
Mon, Feb 18, 6:46 AM
jhenderson added a comment to D58116: [llvm-objcopy] Improve section removal.

Some things that can go wrong if we allow segments to be modified such that their size or addresses change:

Mon, Feb 18, 6:27 AM
jhenderson added inline comments to D58174: [yaml2obj] - Do not skip zeroes blocks if there are relocations against them..
Mon, Feb 18, 6:10 AM · Restricted Project
jhenderson added inline comments to D58168: [yaml2obj] - Do not ignore explicit addresses for .dynsym and .dynstr.
Mon, Feb 18, 6:01 AM · Restricted Project
jhenderson added a comment to D57975: [ObjectYAML] Let dynamic entries use section names as values.

Rebase, reduced patch contents to only include one feature addition. Rather than using a unified Value field, there's now Value and String fields. This was done because a unified Value field that could accept strings and numbers made it somewhat difficult for obj2yaml to output an appropriately formatted value. This is up for discussion, though, as I'm not sure what I've done is the best solution.

Honestly, I feel that a unified field would be better (note, I haven't thought much about how that would affect the implementation). I think that matches what we already do with other fields like Section's Info etc. It is also more intuitive, and you don't have to handle the case of both being specified. I also don't think it matters if yaml2obj -> obj2yaml produces the output identical to the input, as long as it is still valid. In this case, it could (should?) use the actual address of the referenced section, rather than trying to figure out the section name.

Mon, Feb 18, 5:52 AM · Restricted Project
jhenderson added a comment to D58146: [symbolizer] Avoid collecting symbols belonging to invalid sections..

FWIW, I think this test belongs in the DebugInfo directory of the testsuite rather than tools/llvm-symbolizer, because it is testing functionality in the library rather than in the tool layer.

Mon, Feb 18, 1:55 AM · Restricted Project

Fri, Feb 8

jhenderson accepted D57945: Small refactoring of FileError. NFC.

LGTM, with the comment fix.

Fri, Feb 8, 5:45 AM · Restricted Project
jhenderson committed rG6bb9b5943f7d: [LLD][ELF]Add test for missing thin archive member (authored by jhenderson).
[LLD][ELF]Add test for missing thin archive member
Fri, Feb 8, 2:32 AM
jhenderson committed rLLD353508: [LLD][ELF]Add test for missing thin archive member.
[LLD][ELF]Add test for missing thin archive member
Fri, Feb 8, 2:32 AM