jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

jhenderson added inline comments to D50744: [llvm-strip] Add support for -p/--preserve-dates.
Fri, Aug 17, 1:59 AM

Yesterday

jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

I've just noticed that your .debug_str section appears to have SHF_MERGE and SHF_STRINGS set in the input, but the output does not have these flags set. Is this correct? Does GNU objcopy strip the flags? I doubt that it should...

Thu, Aug 16, 3:48 AM
jhenderson accepted D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..
Thu, Aug 16, 3:18 AM
jhenderson added a comment to D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..

This LGTM. But maybe worth giving it a day or two to see if anybody responds to the email thread. Or you could commit it as is, and then fix things if people complain in the thread, I don't mind.

Thu, Aug 16, 3:18 AM
jhenderson accepted D50744: [llvm-strip] Add support for -p/--preserve-dates.

If I get a chance, I'll verify this on Windows. Otherwise, this LGTM.

Thu, Aug 16, 3:07 AM

Wed, Aug 15

jhenderson accepted D50776: [yaml2obj] - Teach yaml2obj to produce SHT_GROUP section with a custom Info field..

LGTM, with nit.

Wed, Aug 15, 6:48 AM
jhenderson added a comment to D49964: [DWARF] Refactor DWARF classes to use unified error reporting. NFC..

Hi @vleschuk, I just wanted to quickly check-in on this to see if you need any help with making further improvements as suggested?

Wed, Aug 15, 4:45 AM · debug-info
jhenderson accepted D50761: [yaml2obj] - Teach tool to produce SHT_GROUP section with a custom type..

LGTM.

Wed, Aug 15, 4:40 AM
jhenderson added a comment to D50761: [yaml2obj] - Teach tool to produce SHT_GROUP section with a custom type..

After one more look, since the field name is SectionOrType, I think it is ok to accept decimals here too for specifying a section index.
It will also simplify the implementation a bit. I'll update the patch.

Wed, Aug 15, 4:36 AM
jhenderson added a comment to D50761: [yaml2obj] - Teach tool to produce SHT_GROUP section with a custom type..

Oops, hang on, didn't see your change as I wrote my review...

Wed, Aug 15, 4:35 AM
jhenderson accepted D50761: [yaml2obj] - Teach tool to produce SHT_GROUP section with a custom type..

LGTM, with nit. I guess if somebody wants decimal support later, they can add it then.

Wed, Aug 15, 4:34 AM
jhenderson added a comment to D50761: [yaml2obj] - Teach tool to produce SHT_GROUP section with a custom type..

Is it normal for yaml2obj to only parse hex values and not decimal for other fields?

Wed, Aug 15, 3:53 AM
jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

Could I make one request going forwards. It's helpful if you post a quick summary of the changes you made each time you update the diff (a bullet-point list would be fine). Due to the scale of the change, I'd prefer not to re-review everything each time I look, and sometimes it's a little hard to remember whether a bit has changed.

Wed, Aug 15, 3:36 AM
jhenderson added inline comments to D50744: [llvm-strip] Add support for -p/--preserve-dates.
Wed, Aug 15, 2:56 AM
jhenderson accepted D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..

I basically ran --globalize-symbol a --keep-global-symbol b for a/b being local, weak, and global. (I added this as a separate test case).
For GNU, b ended up global in the output file. But a was kept local unless it was originally local; when a was local in the input, it ended up being global in the output.

For reference, the GNU business logic is here: https://github.com/redox-os/binutils-gdb/blob/a9d73bb6dacfbbdc826fea178e3a47bf7a4154b1/binutils/objcopy.c#L1552
It seems that:

  • --globalize intentionally only applies to local symbols, so we're already broken w.r.t gnu compatability. (I don't know what the history of that is in llvm-objcopy -- I can fix that if you want). So running GNU objcopy --globalize Weak1 --keep-global-symbol Weak2 will result in Weak1 being local.
  • --globalize omits already global symbols (i.e. (flags & BSF_LOCAL)), but it only checks on the *original* binding type (i.e. it does not look at sym->flags). So --globalize G1 --keep-global-symbol G2 means that G1 will go from global->local (because it isn't included in --keep-global-symbol), but won't go back from local->global because the original binding was already global.

    I'm not sure if the weak symbol binding is an issue; I can fix that in its own commit if you want. For the global symbol binding issue, IMHO, that's a bug, and we should do the functionality you suggested: --globalize foo should keep foo global, whether or not --keep-global-symbol covers it. OTOH, it's also an edge case that probably never comes up.

I can't remember whether we deliberately decided to deviate from GNU on the --globalize behaviour. To me, our behaviour makes sense: if a user deliberately requests to globalize a weak symbol, why shouldn't we? I could even see a use-case for it: a member, in an archive, with a weak symbol definition, won't normally be used, unless other symbols from the archive are needed, so --globalize could force its inclusion. I'd go back and dig out the change that introduced the behaviour and check if it was discussed previously, and if not, file a bug or send an email on the mailing list to discuss it further.

Wed, Aug 15, 2:48 AM
jhenderson accepted D50343: [llvm-objcopy] Add support for -I binary -B <arch>..

LGTM, but there is one thing still outstanding, I think, which @jakehehrlich mentioned in a comment regarding the CopyConfig options:

Wed, Aug 15, 2:22 AM

Tue, Aug 14

jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

Please make sure to rebase this patch when you get a chance, because at the moment, it's a little hard to apply the patch on trunk.

Tue, Aug 14, 3:37 AM
jhenderson requested changes to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..

No null symbol is a blocker, so I've marked this as requesting changes. Sorry!

Tue, Aug 14, 2:55 AM
jhenderson added a comment to D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..

What happens if you specify a mixture of --globalize-symbol and --keep-global-symbol?

The symbol will be a global in the output file.
That's how it accidentally was in the way I wrote it, but I added a test case to make sure it stays that way and left some more comments.

I think I wasn't explicit enough with my wording to explain what I was thinking: what happens if you specify --globalize-symbol a --keep-global-symbol b? It looks like the code will turn a into STB_LOCAL, and b will remain as it currently is (similar points about --localize|weaken-symbol also apply). Does this match GNU's behaviour? I'd expect a to be STB_GLOBAL and b to remain unchanged.

Tue, Aug 14, 2:12 AM

Mon, Aug 13

jhenderson added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Mon, Aug 13, 5:00 AM
jhenderson added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Mon, Aug 13, 3:58 AM
jhenderson added a comment to D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..

What happens if you specify a mixture of --globalize-symbol and --keep-global-symbol?

Mon, Aug 13, 3:46 AM

Fri, Aug 10

jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

I've run out of time of review the rest of this change today, but I will get back to it next week. Please could you try to fix the issues I've highlighted in the meantime.

Fri, Aug 10, 9:32 AM
jhenderson added a comment to D50463: [llvm-objcopy] Add --prefix-sections option.

I don't think I want this option to do anything more than literally add a prefix to all section names. It isn't clear that those precise semantics are being relied on anywhere and the behavior of making relocation names and not renaming symtab etc... is a result of how bfd reconstructs the output binary without consideration for what the user initially put in which is opposite the stated goal of llvm-objcopy.

I can't think of any semantic reason why names should matter for the cases discussed, with the exception of a tiny number of sections (usually to do with debug information or similar such things), names of sections shouldn't matter (and indeed, this has been discussed on the ELF gABI mailing list as a stated goal of ELF). This option WILL break DWARF sections though, which rely on the section naming. I'm not sure if this is something we should care about though?

Fri, Aug 10, 3:28 AM
jhenderson added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Fri, Aug 10, 3:16 AM
jhenderson added a comment to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..

Thanks for the review!

Alex/James/anyone else who's lurking, mind taking a look for a second set of eyes?

Fri, Aug 10, 2:16 AM

Thu, Aug 9

jhenderson added inline comments to D50381: [llvm-objcopy] Add --prefix-symbols option.
Thu, Aug 9, 8:11 AM
jhenderson added a comment to D50463: [llvm-objcopy] Add --prefix-sections option.

I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour for relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"

It turns out that this is a wider bug in llvm-objcopy that is related to, but not specific to this issue: if you do --rename-section in GNU objcopy, you also implicitly rename relocation sections. Meanwhile, if you do --rename-section on relocation sections or strtab or symtab sections, nothing happens (if less concerned about this, because you requested something specific for those sections, but still, it is a behaviour difference).

Thu, Aug 9, 2:56 AM
jhenderson requested changes to D50463: [llvm-objcopy] Add --prefix-sections option.

I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour fro relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"

Thu, Aug 9, 2:04 AM

Wed, Aug 8

jhenderson added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Wed, Aug 8, 9:10 AM

Tue, Aug 7

jhenderson added inline comments to D50381: [llvm-objcopy] Add --prefix-symbols option.
Tue, Aug 7, 9:36 AM
jhenderson added inline comments to D50381: [llvm-objcopy] Add --prefix-symbols option.
Tue, Aug 7, 8:46 AM
jhenderson added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Tue, Aug 7, 5:59 AM

Mon, Aug 6

jhenderson added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Mon, Aug 6, 6:51 AM
jhenderson added a comment to D50235: [yaml2obj] - Add a support for changing EntSize..

LGTM with one nit.

Mon, Aug 6, 6:02 AM
jhenderson accepted D49979: [llvm-objcopy] Add --dump-section.

LGTM, with the suggested changes, but please get @jakehehrlich to confirm he's happy with the test changes.

Mon, Aug 6, 5:51 AM

Fri, Aug 3

jhenderson added inline comments to D50235: [yaml2obj] - Add a support for changing EntSize..
Fri, Aug 3, 4:40 AM
jhenderson added inline comments to D50235: [yaml2obj] - Add a support for changing EntSize..
Fri, Aug 3, 4:34 AM
jhenderson added a comment to D49964: [DWARF] Refactor DWARF classes to use unified error reporting. NFC..

I just noticed that llvmObject has an object_error equivalent to std::errc, with a make_error_code method allowing us to turn them into std::error_code for use with the new function. In particular, there are unexpected_eof and parser_failed errors, which might be more appropriate for certain situations in this file. What do you think?

Fri, Aug 3, 2:07 AM · debug-info
jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

Would you mind when you mark things as done, to upload the latest diff, as it makes it a little confusing for me seeing something marked as "done" when it doesn't appear to have been yet (might be best to then hold off marking off things as done until you are ready to upload).

Fri, Aug 3, 2:03 AM
jhenderson added inline comments to D49979: [llvm-objcopy] Add --dump-section.
Fri, Aug 3, 2:01 AM
jhenderson added a comment to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..

@alexshap, are you happy for me to forward the email thread we had about refactoring llvm-objcopy to @rupprecht?

Fri, Aug 3, 1:48 AM

Thu, Aug 2

jhenderson added inline comments to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Thu, Aug 2, 6:28 AM
jhenderson added a comment to D49979: [llvm-objcopy] Add --dump-section.

Due to the soon encroaching change where -I binary will be implemented I'm very much in favor of the OriginalData field being added to SectionBase. Really seems to handle everything very cleanly IMO.

Okay, go for it. I have nerves around lifetime issues with references like that, but I think the structure means that the Reader (and therefore the file it reads) will be around long enough for it to not matter.

Thu, Aug 2, 6:19 AM
jhenderson requested changes to D49964: [DWARF] Refactor DWARF classes to use unified error reporting. NFC..
Thu, Aug 2, 3:29 AM · debug-info
jhenderson added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Thu, Aug 2, 2:44 AM

Wed, Aug 1

jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

I think @jakehehrlich's proposal more or less matches my thoughts. But I'm wondering why we need the OriginalData field? Do we have anything else that can modify the contents of a debug section? If not, why do we need a different field here? I may be missing something obviously...

Wed, Aug 1, 2:59 AM
jhenderson added a comment to D49979: [llvm-objcopy] Add --dump-section.

So here's an idea. It is ELF specific but it extends the SectionBase interface in a way I've considered for a while and would make both this and another change quite easy. If we add an OriginalData field (ArrayRef<uint8_t>) to SectionBase we can dump that inside of HandleArgs.

Wed, Aug 1, 2:52 AM
jhenderson added a comment to D49676: [DWARF] support for .debug_addr (consumer).

Closed by rL338447

Wed, Aug 1, 2:38 AM · debug-info
jhenderson added inline comments to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Wed, Aug 1, 2:26 AM
jhenderson accepted D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

I don't really have an immediate preference, so I just added those two to the preserve mask -- let me know if I should add others.

I think this is a reasonable approach for now. I can't think of any situation where converting between TLS and non-TLS (and vice versa) makes sense, since the relocations needed are different.

Wed, Aug 1, 2:14 AM

Tue, Jul 31

jhenderson added a comment to D49676: [DWARF] support for .debug_addr (consumer).
  • Fixed spacing in tests
Tue, Jul 31, 8:42 AM · debug-info
jhenderson added a comment to D49676: [DWARF] support for .debug_addr (consumer).

Related aside to my comments regarding version mismatches: is it likely that future versions of DWARF (e.g. DWARF 6) will update the version number of the .debug_addr tables without changing anything in them? I know in previous versions of DWARF, some sections didn't increase in version number in line with the overall standard version. If not, then the version mismatch check may not make sense anyway.

Tue, Jul 31, 8:40 AM · debug-info
jhenderson added a comment to D49676: [DWARF] support for .debug_addr (consumer).

Okay, I think I'm happy with this, but please let the others give their feedback and thumbs up too.

Tue, Jul 31, 8:11 AM · debug-info
jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

I'm not seeing anything that checks that the requested compression style is a supported style. I'd expect an error to be generated if a nonsensical version is specified.

Tue, Jul 31, 3:03 AM
jhenderson added inline comments to D49676: [DWARF] support for .debug_addr (consumer).
Tue, Jul 31, 2:47 AM · debug-info
jhenderson added inline comments to D49979: [llvm-objcopy] Add --dump-section.
Tue, Jul 31, 2:30 AM
jhenderson added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

The code looks correct to me (barring a couple of nits) for what you are trying to do, but I do have a couple of outstanding concerns, that I'd like to discuss further:

Tue, Jul 31, 2:19 AM

Mon, Jul 30

jhenderson added inline comments to D49676: [DWARF] support for .debug_addr (consumer).
Mon, Jul 30, 2:50 AM · debug-info
jhenderson requested changes to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

Marking as "Request Changes" to clear the LGTM until my above points about section flags are addressed.

Mon, Jul 30, 2:12 AM
jhenderson added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

My main concern is still the interaction between these switches and existing flags. Specifically, I'm worried about what will happen if this is used on a section with lots of flags that cannot be controlled via this switch. A quick look at the spec shows that this will clear the following flags unconditionally: SHF_INFO_LINK, SHF_GROUP, SHF_TLS, SHF_COMPRESSED, and any OS/processor-specific flags. I haven't checked what GNU objcopy does, but in basically all of those, changing the flags could be bad, and have a negative impact on the ability to link the resultant object. Could you investigate what GNU does in each of these cases, and write tests to show the behaviour, please.

Mon, Jul 30, 2:11 AM

Fri, Jul 27

jhenderson added inline comments to D49676: [DWARF] support for .debug_addr (consumer).
Fri, Jul 27, 8:39 AM · debug-info
jhenderson added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

What is the behaviour of --rename-section when it touches a section with existing flags that don't conflict with the specified value? For example alloc when the section has SHF_EXECINSTR already? Does it preserve those flags? Can multiple modes be specified for the same section (e.g. code + alloc)? I don't think the tests really show these.

Fri, Jul 27, 7:59 AM
jhenderson added inline comments to D49676: [DWARF] support for .debug_addr (consumer).
Fri, Jul 27, 1:57 AM · debug-info

Thu, Jul 26

jhenderson committed rL338035: Revert r338027 to pacify build bot.
Revert r338027 to pacify build bot
Thu, Jul 26, 8:55 AM
jhenderson committed rL338027: Fix raw_fd_ostream::write_impl hang with large output.
Fix raw_fd_ostream::write_impl hang with large output
Thu, Jul 26, 6:22 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Thu, Jul 26, 6:22 AM
jhenderson added inline comments to D49676: [DWARF] support for .debug_addr (consumer).
Thu, Jul 26, 2:53 AM · debug-info
jhenderson added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

Also I think you can build llvm with or without zlib. I think that means you're going to need to #ifdef a lot of this code.

As an additional point, it would be good to minimise the number of distinct areas where we have to add these #ifdefs. I'd recommend trying to hide as much as possible of this behind an interface of some kind, which turns into a no-op or warning/error if --compress-debug-sections is used without zlib support.

Thu, Jul 26, 2:00 AM

Wed, Jul 25

jhenderson added inline comments to D49676: [DWARF] support for .debug_addr (consumer).
Wed, Jul 25, 7:51 AM · debug-info
jhenderson added inline comments to D49676: [DWARF] support for .debug_addr (consumer).
Wed, Jul 25, 2:19 AM · debug-info

Tue, Jul 24

jhenderson accepted D49016: [llvm-objdump] Add dynamic section printing to private-headers option.

LGTM.

Tue, Jul 24, 3:12 AM
jhenderson added a comment to D49676: [DWARF] support for .debug_addr (consumer).

Not specific to your change, but I noticed that a lot of the DWARFContext code uses 32-bit offsets. Would it not make sense where we are writing new stuff, to use 64-bit offsets, since DWARF does support this?

Tue, Jul 24, 2:54 AM · debug-info
jhenderson added inline comments to D49016: [llvm-objdump] Add dynamic section printing to private-headers option.
Tue, Jul 24, 2:31 AM

Mon, Jul 23

jhenderson added inline comments to D49016: [llvm-objdump] Add dynamic section printing to private-headers option.
Mon, Jul 23, 9:40 AM
jhenderson added a comment to D49016: [llvm-objdump] Add dynamic section printing to private-headers option.

Hmm no.. I'm pretty sure it is a bad idea to do a PR that is completely relying on this one. I just duplicated the code, so I don't see any problem even if someone modifies this area in llvm-readobj. I will just refactor no worries :)

Okay. Please make sure in the commit comment to explain that this is a copy of the llvm-readobj code, and that a subsequent refactor is still to come.

Mon, Jul 23, 2:29 AM

Thu, Jul 19

jhenderson accepted D49544: [llvm-readobj] - Do not report invalid amount of sections..

LGTM.

Thu, Jul 19, 7:41 AM

Jul 18 2018

jhenderson added a comment to D49016: [llvm-objdump] Add dynamic section printing to private-headers option.

Hmm... I don't like using the dynamic symbol table to lookup the dynamic string table. There is a reason for a specific DT_STRTAB tag. We should use that. Also, this latest version won't work if the section headers have been stripped, but we should be able to dump the dynamic table in this instance, by looking at the DT_* tags in the PT_DYNAMIC segment (if present). Try adding a test for this case (i.e. section headers stripped), and you'll see an error, I suspect... Also, please make sure you have a test that has no section headers or PT_DYNAMIC segment.

Jul 18 2018, 4:23 AM
jhenderson added a comment to D49402: [STLExtras] Add size() for arrays.

This appears to be identical to llvm::array_lengthof; do you plan to deprecate that function?

Good catch! I agree with a simple swap from one to the other at some point, but actually, maybe we should just abandon this change, and use array_lengthof, until we are ready to use C++17 directly? It would make the find/replace much easier: I took a quick look, and it looks like array_lengthof is solely used for this particular function, and not some other invocation. But "size" on the other hand will be much harder to isolate.

Jul 18 2018, 2:08 AM

Jul 17 2018

jhenderson accepted D49369: [llvm-readobj] - Teach tool to dump objects with >= SHN_LORESERVE of sections. .

LGTM, with a comment change in the test.

Jul 17 2018, 8:02 AM
jhenderson added inline comments to D49369: [llvm-readobj] - Teach tool to dump objects with >= SHN_LORESERVE of sections. .
Jul 17 2018, 7:00 AM
jhenderson added inline comments to D49369: [llvm-readobj] - Teach tool to dump objects with >= SHN_LORESERVE of sections. .
Jul 17 2018, 6:22 AM
jhenderson accepted D49402: [STLExtras] Add size() for arrays.

LGTM. But I'd like somebody else to chip in on this as well before you commit. If nobody else does in the next couple of days, go ahead and commit.

Jul 17 2018, 3:52 AM
jhenderson added inline comments to D49016: [llvm-objdump] Add dynamic section printing to private-headers option.
Jul 17 2018, 2:31 AM
jhenderson accepted D49372: [llvm-objdump] - Stop reporting bogus section IDs..

LGTM, with a couple of minor improvements to the test.

Jul 17 2018, 1:58 AM
jhenderson requested changes to D49369: [llvm-readobj] - Teach tool to dump objects with >= SHN_LORESERVE of sections. .
Jul 17 2018, 1:55 AM
jhenderson added a comment to D49402: [STLExtras] Add size() for arrays.

I'd also make sure your commit comment mentions that this is intended to match std::size in C++17 (perhaps referencing the version you're implementing and the relevant version, i.e. version 2).

Apologies, I messed up that last part - it should probably say "(perhaps referencing the implementation you are following)"

Jul 17 2018, 1:41 AM
jhenderson added a reviewer for D49402: [STLExtras] Add size() for arrays: vsk.

You might want to canvas opinions from others who have developed in this area recently, such as @vsk, before putting this in, but aside from the missing comment and couple of nits, this looks good.

Jul 17 2018, 1:40 AM

Jul 16 2018

jhenderson added a comment to D49016: [llvm-objdump] Add dynamic section printing to private-headers option.

Little follow-up. I'm pretty sure it would require changes to yaml2obj to get this test written in yaml format.
Why not creating my own binary for the moment, and wait for someone (me if I have time ?) to implement this feature ?

I think this would be a reasonable way forward.

Jul 16 2018, 3:53 AM
jhenderson accepted D49043: [llvm-objdump] Add -demangle (-C) option.

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

Jul 16 2018, 3:15 AM

Jul 13 2018

jhenderson added a comment to D49043: [llvm-objdump] Add -demangle (-C) option.

The buildbots were not passing. Again it seemed to be because of the disassembling option on other architectures (arm etc..).
If someone have any insight about what to do to test this disassembling option, I'll be glad :)

Is the problem as simple as adding a "REQUIRES: x86" to the test? Do the arm build bots etc have x86 target support?

I feel terrible, I don't know the REQUIRES thing... where should I put this ? :)

Take a look at the documentation for it here:
https://llvm.org/docs/TestingGuide.html#constraining-test-execution

Jul 13 2018, 6:12 AM
jhenderson added a comment to D49043: [llvm-objdump] Add -demangle (-C) option.

The buildbots were not passing. Again it seemed to be because of the disassembling option on other architectures (arm etc..).
If someone have any insight about what to do to test this disassembling option, I'll be glad :)

Jul 13 2018, 4:59 AM
jhenderson accepted D49206: [llvm-objcopy] Add support for large indexes.

LGTM, with the nits, assuming the tests aren't realistic without spending hours at it.

Jul 13 2018, 4:23 AM
jhenderson added a comment to D49084: FileCheck: Add support for variable expressions.

I case people are interested I have also implemented something similar in our fork of llvm. I chose [[@EXPR var + 0x42]] as the syntax. I also added some functions such as [[@EXPR hex(VAR)]].
Variables can either by referenced by name or if they conflict with one of the builtin functions using ${x} syntax: [[@EXPR toupper(hex(${hex}))]]

Simple functions are also on my wish-list! The main ones we care about are converting to and from hex numbers, and being able to do additions and subtractions. But I think just the basic integer and hex reading and additions would be a good start.

Jul 13 2018, 1:37 AM

Jul 12 2018

jhenderson added a comment to D49206: [llvm-objcopy] Add support for large indexes.

I might be going overboard with the comments around "auto", and I didn't highlight all of the ones I spotted. But here's the link to the corresponding style point: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable. Feel free to ignore what I said if you think they're valid uses.

Jul 12 2018, 4:57 AM

Jul 11 2018

jhenderson added a comment to D49016: [llvm-objdump] Add dynamic section printing to private-headers option.

(moving this comment out of line, because the comments got completely out of sync with the code):
Regarding the VirtBase calculation, yeah, you're right, there was a mistake on my part - I made an incorrect assumption regarding program layout.

Jul 11 2018, 2:36 AM
jhenderson added inline comments to D49043: [llvm-objdump] Add -demangle (-C) option.
Jul 11 2018, 1:38 AM

Jul 10 2018

jhenderson accepted D49043: [llvm-objdump] Add -demangle (-C) option.

LGTM, with a few minor points.

Jul 10 2018, 7:10 AM
jhenderson added inline comments to D49043: [llvm-objdump] Add -demangle (-C) option.
Jul 10 2018, 3:37 AM
jhenderson added a comment to D49084: FileCheck: Add support for variable expressions.

First up, I want to say thank you for doing this! This is one of the major missing features from FileCheck over the test harness I am used to using, and leads to fragile testing where people have hard-coded in values in tests that are subject to change.

Jul 10 2018, 3:28 AM