Page MenuHomePhabricator

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Thu, Apr 18

jhenderson added a comment to D60502: [llvm-nm] Add --special-syms.

Just to see if I understand the comments so far: --special-syms is essentially already the default for llvm-nm (i.e. we print special symbols), but not for GNU nm. And so even though we print special symbols, a configure script that checks if $NM --special-syms is supported will fail when it shouldn't.

If so, then LGTM -- this patch makes things less worse in all regards.

There's a counter-example to this though, which I'm concerned about. If we silently accept --special-syms, with it being the default always, there could be configure scripts that don't specify --special-syms that expect to NOT see those symbols (e.g. they're looking for all "normal" symbols). This in turn could result in a silent breakage.

Thu, Apr 18, 4:25 AM
jhenderson accepted D60405: MinidumpYAML: Add support for ModuleList stream.
Thu, Apr 18, 3:50 AM · Restricted Project
jhenderson added a comment to D60502: [llvm-nm] Add --special-syms.

So what are further actions with this? Is emulating GNU nm seems like a better choice?

In general, we should try to emulate GNU nm behaviour (that was the conclusion from the recent LLVM conference). However, I'm slightly concerned about changing behaviour breaking users of the LLVM tools. It might be worth pinging llvm-dev to see if there's anybody who would be unhappy with changing the default at this point, or even just to let people know that this will happen, unless somebody objects. Personally, I have no objections to it, but I don't have any use for llvm-nm and ARM currently.

Thu, Apr 18, 3:25 AM
jhenderson committed rG66a9d0f8c6c9: [llvm-objcopy][llvm-strip] Add switch to allow removing referenced sections (authored by jhenderson).
[llvm-objcopy][llvm-strip] Add switch to allow removing referenced sections
Thu, Apr 18, 2:16 AM
jhenderson committed rL358649: [llvm-objcopy][llvm-strip] Add switch to allow removing referenced sections.
[llvm-objcopy][llvm-strip] Add switch to allow removing referenced sections
Thu, Apr 18, 2:16 AM
jhenderson closed D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Thu, Apr 18, 2:16 AM · Restricted Project
jhenderson added a comment to D60825: [llvm-objcopy] - Check dynamic relocation sections for broken references..

FYI, I've just landed D60324, and approved D60820. This looks good, except for the edge case mentioned by @jakehehrlich. The code should also implement --allow-broken-links now that that has landed.

Thu, Apr 18, 2:15 AM
jhenderson accepted D60820: [yaml2elf/obj2yaml] - Allow normal parsing/dumping of the .rela.dyn section..

LGTM, with two nits.

Thu, Apr 18, 2:08 AM · Restricted Project
jhenderson updated the diff for D60324: [llvm-objcopy] Add switch to allow removing referenced sections.

Add TODOs. Fix typos.

Thu, Apr 18, 1:55 AM · Restricted Project
jhenderson added inline comments to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Thu, Apr 18, 12:56 AM · Restricted Project

Wed, Apr 17

jhenderson updated the diff for D60324: [llvm-objcopy] Add switch to allow removing referenced sections.

Rebase & address review comments:

  • Remove unnecessary empty output checks.
  • Add strip option and tests.
  • Reformat one bit.
  • Use distinct check prefixes for the error messages.
Wed, Apr 17, 5:44 AM · Restricted Project
jhenderson accepted D60067: [llvm-symbolizer] Add llvm-addr2line.

LGTM.

Wed, Apr 17, 5:28 AM · Restricted Project
jhenderson accepted D60816: [llvm-symbolizer] Unhide and document the '-output-style' option.

LGTM, with a couple of minor comments.

Wed, Apr 17, 5:21 AM · Restricted Project
jhenderson added inline comments to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Wed, Apr 17, 3:45 AM · Restricted Project
jhenderson added inline comments to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Wed, Apr 17, 3:41 AM · Restricted Project
jhenderson added inline comments to D60770: [llvm-symbolizer] Make the output with `-output-style=GNU` closer to addr2line's.
Wed, Apr 17, 3:24 AM · Restricted Project
jhenderson updated the diff for D60324: [llvm-objcopy] Add switch to allow removing referenced sections.

Refactor an if block missed in previous change. Rename switch to --allow-broken-links.

Wed, Apr 17, 3:16 AM · Restricted Project
jhenderson added a comment to D60382: FileCheck [2/12]: Stricter parsing of -D option.

I have only one more minor comment. Happy for this to go in with or without it, but @probinson should give the final thumbs up.

Wed, Apr 17, 2:41 AM · Restricted Project
jhenderson added inline comments to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Wed, Apr 17, 2:37 AM · Restricted Project
jhenderson added inline comments to D60770: [llvm-symbolizer] Make the output with `-output-style=GNU` closer to addr2line's.
Wed, Apr 17, 2:36 AM · Restricted Project
jhenderson added inline comments to D60773: [llvm-objcopy] Support full list of bfd targets that lld uses..
Wed, Apr 17, 2:08 AM · Restricted Project
jhenderson added a comment to D60067: [llvm-symbolizer] Add llvm-addr2line.

Code all looks good, but you should talk about the effect of --output-style on the output for both llvm-symbolizer and llvm-addr2line as appropriate.

Do you want this option to be added to the documentation about llvm-symbolizer or something else? Or you meant other places where this info should be added?

Wed, Apr 17, 2:06 AM · Restricted Project
jhenderson accepted D60770: [llvm-symbolizer] Make the output with `-output-style=GNU` closer to addr2line's.

LGTM, but I wouldn't mind a second pair of eyes on the new logic (@rupprecht?), as I'm not really familiar with how the DILineInfo class works.

Wed, Apr 17, 2:06 AM · Restricted Project
jhenderson added inline comments to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Wed, Apr 17, 1:59 AM · Restricted Project

Tue, Apr 16

jhenderson added inline comments to D60773: [llvm-objcopy] Support full list of bfd targets that lld uses..
Tue, Apr 16, 8:59 AM · Restricted Project
jhenderson added inline comments to D60382: FileCheck [2/12]: Stricter parsing of -D option.
Tue, Apr 16, 8:51 AM · Restricted Project
jhenderson updated the diff for D60324: [llvm-objcopy] Add switch to allow removing referenced sections.

Re-arrange some if/else statements and improve/add some comments and the option help text.

Tue, Apr 16, 7:47 AM · Restricted Project
jhenderson added inline comments to D60773: [llvm-objcopy] Support full list of bfd targets that lld uses..
Tue, Apr 16, 7:38 AM · Restricted Project
jhenderson added inline comments to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Tue, Apr 16, 6:03 AM · Restricted Project
jhenderson added reviewers for D60321: [llvm-readobj] Add helper functions `printVersionSymbol()`, `printVersionDefinition()` and `printVersionDependency()`: grimar, rupprecht.

As I'll be away next week, I've added a couple of others to help with reviewing this too.

Tue, Apr 16, 6:00 AM · Restricted Project
jhenderson added inline comments to D60770: [llvm-symbolizer] Make the output with `-output-style=GNU` closer to addr2line's.
Tue, Apr 16, 5:30 AM · Restricted Project
jhenderson added a comment to D60067: [llvm-symbolizer] Add llvm-addr2line.

Code all looks good, but you should talk about the effect of --output-style on the output for both llvm-symbolizer and llvm-addr2line as appropriate.

Tue, Apr 16, 5:26 AM · Restricted Project
jhenderson added inline comments to D60250: [llvm-objdump] Allow -dynamic-reloc on ET_EXEC files.
Tue, Apr 16, 2:56 AM · Restricted Project
jhenderson added a comment to D60250: [llvm-objdump] Allow -dynamic-reloc on ET_EXEC files.

Looks good, aside from one comment about the tests.

Tue, Apr 16, 2:16 AM · Restricted Project

Mon, Apr 15

jhenderson updated the summary of D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Mon, Apr 15, 9:00 AM · Restricted Project
jhenderson added a comment to D60067: [llvm-symbolizer] Add llvm-addr2line.

Could we have a test case that shows that llvm-symbolizer --output-style=GNU produces the same as llvm-addr2line for the inlining stuff, please?

Mon, Apr 15, 7:38 AM · Restricted Project
jhenderson requested changes to D60067: [llvm-symbolizer] Add llvm-addr2line.
Mon, Apr 15, 4:51 AM · Restricted Project
jhenderson accepted D60376: [llvm-objdump] Align instructions to a tab stop in disassembly output.
Mon, Apr 15, 4:51 AM · Restricted Project
jhenderson added inline comments to D60382: FileCheck [2/12]: Stricter parsing of -D option.
Mon, Apr 15, 4:34 AM · Restricted Project
jhenderson accepted D60684: [llvm-readobj] Reapply: Improve error message for --string-dump.

LGTM. I'm slightly surprised the latter checks needed modifying (it's okay to though), but aside from that, this is fine.

Mon, Apr 15, 3:59 AM · Restricted Project
jhenderson added a comment to D60067: [llvm-symbolizer] Add llvm-addr2line.

But I'm fine checking this in now as .rst for consistency. Worst case scenario, it doesn't render, and someone can commit a patch to fix it. Nothing will stop compiling :)

FWIW, there is actually a build bot that does the conversion, and it fails if the syntax is bad, so we should aim to get it right first time if possible.

Mon, Apr 15, 3:18 AM · Restricted Project
jhenderson accepted D60381: FileCheck [1/12]: Move variable table in new object.

LGTM, with one nit.

Mon, Apr 15, 1:58 AM · Restricted Project

Fri, Apr 12

jhenderson accepted D60614: [llvm-readelf] Fix dumping of SHN_XINDEX symbols.

LGTM, with nit. Might want to get @MaskRay to give thumbs up on the macro stuff, but it's probably fine.

Fri, Apr 12, 8:48 AM · Restricted Project
jhenderson added a comment to D60614: [llvm-readelf] Fix dumping of SHN_XINDEX symbols.

Okay, basically looks fine. Just a couple of small comments.

Fri, Apr 12, 7:43 AM · Restricted Project
jhenderson added inline comments to D60376: [llvm-objdump] Align instructions to a tab stop in disassembly output.
Fri, Apr 12, 7:43 AM · Restricted Project
jhenderson added inline comments to D60614: [llvm-readelf] Fix dumping of SHN_XINDEX symbols.
Fri, Apr 12, 7:15 AM · Restricted Project
jhenderson added a comment to D60614: [llvm-readelf] Fix dumping of SHN_XINDEX symbols.

There is no test for PRC, OS and RSV kind of symbols. Does anyone know how to generate them?

Use yaml2obj, with an explicit section index, but actually you don't need to test them as they are already tested by test/tools/llvm-readobj/elf-symbol-shndx.test.

Fri, Apr 12, 7:01 AM · Restricted Project
jhenderson added inline comments to D60381: FileCheck [1/12]: Move variable table in new object.
Fri, Apr 12, 6:42 AM · Restricted Project
jhenderson added inline comments to D60381: FileCheck [1/12]: Move variable table in new object.
Fri, Apr 12, 6:32 AM · Restricted Project
jhenderson accepted D60067: [llvm-symbolizer] Add llvm-addr2line.

LGTM, but it would be worth getting a second pair of eyes on the documentation who is more familiar with how the markup works, to make sure it is correct.

Fri, Apr 12, 6:32 AM · Restricted Project
jhenderson added a comment to D60555: [llvm-objcopy] Fill .symtab_shndx section correctly.

Looks to me like you've hit a bug here

Are you sure it's a bug? According to sources it looks like some extension to original readelf output:

// Find if:
// Processor specific
if (SectionIndex >= ELF::SHN_LOPROC && SectionIndex <= ELF::SHN_HIPROC)
  return std::string("PRC[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";
// OS specific
if (SectionIndex >= ELF::SHN_LOOS && SectionIndex <= ELF::SHN_HIOS)
  return std::string("OS[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";
// Architecture reserved:
if (SectionIndex >= ELF::SHN_LORESERVE &&
    SectionIndex <= ELF::SHN_HIRESERVE)
  return std::string("RSV[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";

Anyway I'll switch to llvm-readobj in the test

Yes, it's a bug. Where the original st_shndx field is between SHN_LORESERVE and SHN_HIRESERVE (and not SHN_XINDEX), it should be decorated in this way. In all other cases it shouldn't be. It's a little complicated to explain in detail, but I'll do my best. If we look at the section header table, there are sections with index 0xff00, 0xff01, etc. Thus when we generally refer to sections with index 0xff00 etc we are talking about those sections. The symbol st_shndx field (i.e. the field indicating the symbol's section) however is only 16 bits, and the upper few values are reserved for various purposes. The SHN_XINDEX value (0xffff) in this context means look up the section index in the SHT_SYMTAB_SHNDX section instead. Other values in this range have other meanings (e.g. absolute and common symbols). If the real section index for the symbol's section is >= SHN_LORESERVE, it is impossible to represent it in directly in st_shndx, so it will be SHN_XINDEX.

Fri, Apr 12, 5:34 AM · Restricted Project
jhenderson added a comment to D60603: Make llvm-as --help great again.

I don't really know much about llvm-as. Are all the filtered-out options irrelevant? In particular -color seems like it might be relevant.

Fri, Apr 12, 3:03 AM · Restricted Project
jhenderson added a comment to D60270: [llvm-objcopy] Add support for Intel HEX input/output format.

Ping

Fri, Apr 12, 2:57 AM
jhenderson added inline comments to D60376: [llvm-objdump] Align instructions to a tab stop in disassembly output.
Fri, Apr 12, 2:57 AM · Restricted Project
jhenderson added a comment to D60067: [llvm-symbolizer] Add llvm-addr2line.

I guess it's not considered a common practice, but i think it would be a really good idea to add a docs page for the new tool at the same time. Else it will be forgotten, much like for half of other tools..

I agree, although it's worth noting that llvm-symbolizer DOES have this documentation, so it may be worth piggy-backing on that rather than writing an entirely new doc.

Fri, Apr 12, 2:46 AM · Restricted Project
jhenderson added inline comments to D60502: [llvm-nm] Add --special-syms.
Fri, Apr 12, 2:40 AM
jhenderson accepted D60555: [llvm-objcopy] Fill .symtab_shndx section correctly.

Could we use llvm-mc and some loop macros to generate a binary with the required amount of sections?
I did that for a testcase that needed 32768 symbols: https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/mips-out-of-bounds-call16-reloc.s

Good tip, thanks. It's probably worth bearing in mind in case we have any more tests along these lines that the existing pre-canned binary can't be used for.

Fri, Apr 12, 2:34 AM · Restricted Project
jhenderson added a comment to D60381: FileCheck [1/12]: Move variable table in new object.

Basically looks good to me. Just a few small comments remaining.

Fri, Apr 12, 2:17 AM · Restricted Project

Thu, Apr 11

jhenderson accepted D60042: [llvm-objcopy] Add --prefix-alloc-sections.

LGTM. As I mentioned earlier, @jakehehrlich should review this before it gets committed. However, he may be away, so you might have to wait a few days.

Thu, Apr 11, 6:33 AM · Restricted Project
jhenderson accepted D60411: Filter out irrelevant llvm-nm option.

Great, LGTM.

Thu, Apr 11, 6:16 AM · Restricted Project
jhenderson added inline comments to D60042: [llvm-objcopy] Add --prefix-alloc-sections.
Thu, Apr 11, 6:14 AM · Restricted Project
jhenderson added inline comments to D60411: Filter out irrelevant llvm-nm option.
Thu, Apr 11, 5:20 AM · Restricted Project
jhenderson added inline comments to D60502: [llvm-nm] Add --special-syms.
Thu, Apr 11, 3:05 AM
jhenderson added inline comments to D60376: [llvm-objdump] Align instructions to a tab stop in disassembly output.
Thu, Apr 11, 2:56 AM · Restricted Project
jhenderson added a comment to D60250: [llvm-objdump] Allow -dynamic-reloc on ET_EXEC files.

Yeah the rule is a bit weird but I think ET_DYN without a .dynamic section is really a corner case. I did try to craft one by hex-editing and ran objdump -R on it, which did successfully *workaround* this condition but then it hit another error in another place.

You can use objcopy -R .dynamic to remove the .dynamic without resorting to hex-editing. I've seen developers do some pretty weird "corner case" things and still expect things to work, so it would be good if we can achieve it, assuming the logic isn't too hard. Since it just makes sense to ignore the ELF type entirely, I think this resolves the concern anyway.

Thu, Apr 11, 2:53 AM · Restricted Project
jhenderson added a comment to D60555: [llvm-objcopy] Fill .symtab_shndx section correctly.

Test case?

Have you seen bug description?
The problem with test case is that it requires file with large number of sections of which at least some should have relocation sections.
I have 19Mb object file generated by clang, but I'm not sure if using it as test case is a good idea ...

Sorry, I skipped over that somehow (I think I jumped straight from the bug). We already have at least one test that uses many sections (see many-sections.test). That unzips a pre-built object to achieve this. One option might be to update that object file (and possibly the test), if it can satisfy the prerequisites you need.

Thu, Apr 11, 2:46 AM · Restricted Project
jhenderson added a comment to D60411: Filter out irrelevant llvm-nm option.

Question: instead of having -s require two argument, we could require one argument and split on space character, or comma?

Splitting on a character would make a lot of sense. Most tools use '=' or similar for multi-string arguments. However, if there is a Mach-O version of llvm-nm that inspired this option, we should be compatible with that if at all possible.

Thu, Apr 11, 2:43 AM · Restricted Project
jhenderson added a comment to D60381: FileCheck [1/12]: Move variable table in new object.

Have you looked at unit-testing the new code? It might be nice if you can.

Thu, Apr 11, 2:38 AM · Restricted Project
jhenderson added a comment to D60555: [llvm-objcopy] Fill .symtab_shndx section correctly.

Test case?

Thu, Apr 11, 2:26 AM · Restricted Project
jhenderson added a comment to D60470: [DWARF] Change ambiguity resolution from smallest CUOffset to largest (LowPC, CUOffset).

What does addr2line (or any other symbolizers, not that I know of others) do in these cases?

addr2line doesn't use binary search. It iterates all ELF sections, finds the first section containing the address, then iterates DWARF CUs and finds the associated line table. It has the same problem

Any idea why this problem hasn't been much of an issue until now/recently (both your change here, and the LLD change being reviewed)?

It's not even a new issue. We've had this problem for years in the PlayStation proprietary linker, because ASLR-enabled executables have a 0 base address. We have special handling of discarded functions to work around this in the linker.

Thu, Apr 11, 2:20 AM · Restricted Project
jhenderson added a comment to D60042: [llvm-objcopy] Add --prefix-alloc-sections.

Having looked at this a bit more, I wonder if this should be part of the existing SectionsToRename loop. After all, there isn't much difference between the two behaviours, apart from the fact that this new switch doesn't change the flags. Would you mind folding the two loops together? As a side effect of this, I expect you might want to rename relocation sections when --rename-section alone is used, which I don't think is currently happening.

Thu, Apr 11, 2:11 AM · Restricted Project
jhenderson added a comment to D59746: [CommandLineParser] Add DefaultOption flag.

I've not looked at the implementation in depth, but cl::DefaultOption seems like a nice way of handling this. Please make sure that there is testing in llvm-readobj and llvm-objdump that test their own short alias interpretation somewhere though.

Thu, Apr 11, 2:03 AM · Restricted Project, Restricted Project
jhenderson added a comment to D60189: [llvm-objcopy] Simplify SHT_NOBITS -> SHT_PROGBITS promotion.

I'm trying to think where a NOBITS/PROGBITS distinction actually matters. Some relevant thoughts come to my mind:

  1. Where sections are in the middle of segments, they should always be PROGBITS, not NOBITS, otherwise addresses will get messed up (there is a special exception for .tbss and nested PT_TLS segments).
  2. If a section is PROGBITS when it doesn't need to be, then it will take up file space. Assuming it is filled with zeroes, this is harmless in terms of how the ELF behaves, and just impacts things like file size and probably load times.
  3. Converting a PROGBITS section to NOBITS can be harmful if the contents are important, as those contents will be lost.
  4. Non-alloc NOBITS sections are meaningless. Promoting them to PROGBITS will bloat the file size but otherwise have no effect.
  5. If there is a GNU binutils test for it, there's probably some good reason for any behaviour that I haven't thought of, so we should match GNU's behaviour, unless it is clearly dodgy.
Thu, Apr 11, 2:00 AM · Restricted Project

Wed, Apr 10

jhenderson added a comment to D59746: [CommandLineParser] Add DefaultOption flag.

Just chiming in from a LLVM binutils developer perspective. Some of the binutils are missing -h as an alias, when they really should have it for compatibility (specifically I checked llvm-nm and llvm-symbolizer). As a result, a solution that auto-adds -h as an alias would be good, as long as we have the ability to override it somehow. I wouldn't mind having logic in llvm-objdump/llvm-readobj to explicitly override the short alias if it is required.

Wed, Apr 10, 8:36 AM · Restricted Project, Restricted Project
jhenderson added a comment to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.

A couple of examples where this might be useful:

  1. Removing just the dynamic symbols. This would cause an error if there are any dynamic relocation sections. However, not all dynamic relocations require symbols, so potentially NO dynamic relocations need symbols. The .dynsym is then unnecessary.
  2. Removing the .dynstr. This would cause an error due to .dynamic referencing it. This section doesn't necessarily have tags that reference strings. Additionally, users might want to strip the information, but still provide some of the ELF file for example to aid debugging an issue.
Wed, Apr 10, 7:19 AM · Restricted Project
jhenderson added inline comments to D60381: FileCheck [1/12]: Move variable table in new object.
Wed, Apr 10, 7:08 AM · Restricted Project
jhenderson added inline comments to D60376: [llvm-objdump] Align instructions to a tab stop in disassembly output.
Wed, Apr 10, 7:01 AM · Restricted Project
jhenderson added a comment to D60411: Filter out irrelevant llvm-nm option.

Thanks for the help text. I think we could add a simple test actually. It would test that "Generic Options:" and "llvm-nm Options:" are printed, but not "General Options:" (I think that's the name given to the irrelevant LLVM options, but I could be wrong).

Wed, Apr 10, 5:46 AM · Restricted Project
jhenderson added inline comments to D60381: FileCheck [1/12]: Move variable table in new object.
Wed, Apr 10, 5:37 AM · Restricted Project
jhenderson accepted D60439: [llvm-objcopy] Accept --long-option but not -long-option.

The behaviour change looks good, in the sense that it does what you set out to do. I don't have any real objections to it aside from the fact that it is different from how LLVM tools typically work, but I personally also find it odd the LLVM tools work with a single-dash! I'd like others to comment on this though before giving the thumbs up, particularly @jakehehrlich as the code owner.

Wed, Apr 10, 4:16 AM · Restricted Project
jhenderson added a comment to D60411: Filter out irrelevant llvm-nm option.

I don't think a test for every option is wise, but could you paste the output of the help text with your change applied, please, so I can review it easily?

Wed, Apr 10, 4:09 AM · Restricted Project
jhenderson added inline comments to D60250: [llvm-objdump] Allow -dynamic-reloc on ET_EXEC files.
Wed, Apr 10, 3:58 AM · Restricted Project

Fri, Apr 5

jhenderson created D60324: [llvm-objcopy] Add switch to allow removing referenced sections.
Fri, Apr 5, 9:27 AM · Restricted Project
jhenderson added inline comments to D60312: [llvm-readobj] Add GNU style dumper for .gnu.version_d section.
Fri, Apr 5, 3:09 AM · Restricted Project
jhenderson added inline comments to D60312: [llvm-readobj] Add GNU style dumper for .gnu.version_d section.
Fri, Apr 5, 2:49 AM · Restricted Project
jhenderson added inline comments to D60042: [llvm-objcopy] Add --prefix-alloc-sections.
Fri, Apr 5, 1:15 AM · Restricted Project

Thu, Apr 4

jhenderson accepted D60121: Object/Minidump: Add support for reading the ModuleList stream.

Okay, no more comments from me.

Thu, Apr 4, 8:08 AM · Restricted Project
jhenderson accepted D59775: Minidump: Add support for reading/writing strings.

LGTM.

Thu, Apr 4, 7:50 AM · Restricted Project
jhenderson added a comment to D60121: Object/Minidump: Add support for reading the ModuleList stream.

Code basically looks fine, but somebody else should confirm that the format is correct.

Thu, Apr 4, 6:46 AM · Restricted Project
jhenderson added a comment to D60189: [llvm-objcopy] Simplify SHT_NOBITS -> SHT_PROGBITS promotion.

I don't feel strongly about this change either way. I think it is correct in the sense that it does what it sets out to achieve, but I have no opinion on whether we should aim for compatibility here. I do agree that non-alloc nobits sections makes no sense, but could there be a crazy build system where somebody modifies the flags in different runs, relying on the later run to add the SHF_ALLOC section?

Thu, Apr 4, 6:37 AM · Restricted Project
jhenderson added inline comments to D59775: Minidump: Add support for reading/writing strings.
Thu, Apr 4, 6:27 AM · Restricted Project
jhenderson added inline comments to D60042: [llvm-objcopy] Add --prefix-alloc-sections.
Thu, Apr 4, 5:39 AM · Restricted Project
jhenderson accepted D60042: [llvm-objcopy] Add --prefix-alloc-sections.

LGTM, but please wait for one of the other reviewers too.

Thu, Apr 4, 4:45 AM · Restricted Project
jhenderson added a comment to D60250: [llvm-objdump] Allow -dynamic-reloc on ET_EXEC files.

Test cases?

Thu, Apr 4, 1:49 AM · Restricted Project
jhenderson added a comment to D60189: [llvm-objcopy] Simplify SHT_NOBITS -> SHT_PROGBITS promotion.

Since llvm-objcopy contributors are adding more and more GNU objcopy options to llvm-objcopy, I feel people care about its compatibility with GNU objcopy. My preference is actually to keep just SectionFlag::SecContents and drop the other two, if I'm allowed to do so. This would certainly harm compatibility with GNU objcopy. What do people think?

Do you mean removing for e.g. --set-section-flags=data or whatever? I'm against dropping support for any flags that were added to be compatible with GNU, as most of them have been added with good reason, or are being relied on by end users to do what is expected. Certainly, I've run into some users of these switches, though I don't remember exactly which values.

Thu, Apr 4, 1:40 AM · Restricted Project
jhenderson added inline comments to D60042: [llvm-objcopy] Add --prefix-alloc-sections.
Thu, Apr 4, 1:37 AM · Restricted Project
jhenderson added inline comments to D60170: [llvm-objcopy] [llvm-symbolizer] Fix failing tests.
Thu, Apr 4, 1:32 AM · Restricted Project
jhenderson accepted D60170: [llvm-objcopy] [llvm-symbolizer] Fix failing tests.

LGTM.

Thu, Apr 4, 1:26 AM · Restricted Project

Wed, Apr 3

jhenderson added inline comments to D60170: [llvm-objcopy] [llvm-symbolizer] Fix failing tests.
Wed, Apr 3, 9:56 AM · Restricted Project
jhenderson added inline comments to D60170: [llvm-objcopy] [llvm-symbolizer] Fix failing tests.
Wed, Apr 3, 9:53 AM · Restricted Project