In D95341#2524716, @jhenderson wrote:

LGTM.

In D95341#2524529, @grimar wrote:

Hi guys.
I am sorry for pinging this too early, but I am planning a ~6 weeks vacation starting from 1st of february
and I wonder if there is something I can do for this to land it or if there are any concerns about this change?

No problem. Somehow this slipped off my review pile, so I hadn't seen it. Let me know if there are any other reviews I haven't done by the end of the day.

Hi guys.
I am sorry for pinging this too early, but I am planning a ~6 weeks vacation starting from 1st of february
and I wonder if there is something I can do for this to land it or if there are any concerns about this change?

In D95354#2522343, @jhenderson wrote:

In D95354#2522323, @grimar wrote:

It turns out this wasn't NFC, one of LLDB test cases started to crash.
I've fixed it in rG2a33b092f5b175a21680b3980ff2019bc1c65b12.

The difference was that before this patch we set the sh_link for symbol tables basing on their names (E.g ".symtab"),
now we set this field basing on section types (e.g SHT_SYMTAB).
The mentioned test case has a .text section of type SHT_SYMTAB and it did not expect to see a sh_link field set it seeems.

Based on my casual reading of that test, I wonder if you found a bug in the tool there - it looks like the intent is for section types to trump section names, which surely should mean the use of sh_link would be correctin this case? Perhaps we should highlight this to the LLDB developers (pinging @JDevlieghere for example).

E.g. it seems that the expected behavior of llvm-objcopy would be to drop the sh_link for symbol table in this case. Because it is of SHT_NOBITS type.
Perhaps I'd wait from some input from @MaskRay as he was the author of the test case I think.

In D95364#2522324, @jhenderson wrote:

Looks reasonable, but perhaps fixing the llvm-objcopy issue first would be best. What do you think?

It turns out this wasn't NFC, one of LLDB test cases started to crash.
I've fixed it in rG2a33b092f5b175a21680b3980ff2019bc1c65b12.

I like this approach.

• Addressed review comment.

As far I understand, looking on the description of D94239, the message on z/OS looks like "EDC5129I No such file or directory.".
I guess the EDC5129I is a stable error code? So why not to check for a possible EDC5129I prefix word instead of .*?

A minor nit from me, the rest looks fine.

This looks to me it solves a specific corner case, but I really don't feel I have enough knowledges about embedded world/scripts to say whether it is a good or bad thing to support it.
It seems matches to what GNU ld/gold do. I'll add Peter who I guess might have an opinion about this.

• Addressed review comments.

D95140 was posted instead.

• Addressed review comments.

In D95140#2514914, @jhenderson wrote:

It feels like there needs to be testing there for writing the section header table in the right place in the YAML (with Offset if needed), but I don't see that yet.

LGTM with a nit, please wait for others.

In D95124#2512145, @jhenderson wrote:

missing check-prefix. Not sure how I didn't spot the latter...

This LGTM, but please wait for others.

@MaskRay, are you OK with it?

Ping.

• Added test cases.

• Addressed review comment.

• Addressed review comments.

• Format.

• Addressed review comments.

In D94560#2502703, @inglorion wrote:

Thanks again.

I chose DAG on purpose because I think it more accurately expresses what we're really asserting.

First, we since we removed the sorting, we're not guaranteeing that these will be in any particular order. Given that, I would rather not make the test depend on the order.

An alternative to this could probably be changing the Writer<ELFT>::assignFileOffsets()
It has the following loop:

In D94560#2499076, @inglorion wrote:

Thanks for the comments!

In D94560#2497393, @grimar wrote:

E.g. I think it can be just a 2 lines loop which would iterate over all output sections and print their names ans sizes.
No need to sort them or to calculate fields width.

The reason I do the sorting is because there can potentially be very many sections. Imagine a large binary with -fdata-sections -ffunction-settings.

In D94659#2498626, @MaskRay wrote:

But I think using 0 in obj2yaml is fine.

In D93362#2499834, @haowei wrote:

For the case to test error message when sh_size % sh_entsize is not 0, I didn't find any good examples that I can manipulate the sh_entsize through yaml2obj. Do you have good suggestions to write unit tests for these cases?

In D93362#2499834, @haowei wrote:

The reason is that .strtab and .shstrtab are placed at the end of elf file automatically by yaml2obj and there will almost always be cases to make (*It) & 1 != 0, preventing the code from reach the error message. I think the only way would be placing .gnu.hash at the end of the ELF, and remove the terminator, but that seems to be not possible with yaml2obj. For the case to test error message when sh_size % sh_entsize is not 0, I didn't find any good examples that I can manipulate the sh_entsize through yaml2obj. Do you have good suggestions to write unit tests for these cases?

In D94667#2498005, @tschuett wrote:

Does it have to stay in llvm-nm.cpp? A different file for MachO stuff?

• Addressed review comments.

FWIW, I think we could support this, but I'd implement it in a much more trivial way.
E.g. I think it can be just a 2 lines loop which would iterate over all output sections and print their names ans sizes.
No need to sort them or to calculate fields width.

I am not the right person to review this, I don't work on MC or MC/ARM. I think you should add people who mantains MC/ARM and/or may be authors of these tests.
The change looks good to me though.

Ok. I've used the following piece of code to construct a 1Gb of random compressed data and estimated
the difference between the old and the new code in Release.

In D93678#2476962, @jhenderson wrote:

An alternative would be to move the Offset aspect of the sections into the Layout table only, and possibly moving Fills into that Layout block too out of the Sections list. This has the advantage that layout information is generally kept in the Layout table, whilst the details of what a thing actually is (e.g. section type, size, address etc) are kept in appropriate parts of the document. This is closer to my original option 3:

FileHeader:
  ...
Sections:
  - Name: .foo
    Type: SHT_PROGBITS
  - Name: .bar
    Type: SHT_PROGBITS
SectionHeaderTable:
  ...
ProgramHeaders:
  ...
Layout:
   - Type: Section
    Name: .foo
    Offset: 0x1000
  - Type: Fill
    # Offset could be implicitly calculated here; fills move out of Sections to Layout.
    Size: 0x100
  - Type: SectionHeaderTable
    Offset: 0x2000
  - Type: ProgramHeaderTable
    Offset: 0x3000
  - Type: Section
    Name: .bar
    Offset: 0x3500

I think I mildly prefer the second one, as it'll be easier to retrofit and feels a little cleaner. We'd need some implicit rules that the program header table appears at the front and the section header table at the back if they aren't specifically mentioned in either case.