This is an archive of the discontinued LLVM Phabricator instance.

[obj2yaml] - Teach tool to emit the "SectionHeaderTable" key and sort sections by file offset.
ClosedPublic

Authored by grimar on Nov 11 2020, 4:09 AM.

Details

Summary

Currently when we dump sections, we dump them in the order,
which is specified in the sections header table.

With that the order in the output might not match the order in the file.
This patch starts sorting them by by file offsets when dumping.

When the order in the section header table doesn't match the order
in the file, we should emit the "SectionHeaderTable" key. This patch does it.

Diff Detail

Event Timeline

grimar created this revision.Nov 11 2020, 4:09 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: emaste. · View Herald Transcript
grimar requested review of this revision.Nov 11 2020, 4:09 AM

"actual order" -> "file offset header.

What is the motivation behind this change?

"actual order" -> "file offset header.

What is the motivation behind this change?

There was a mini discussion about behavior of "LastSec"/"FirstSec" here:
https://reviews.llvm.org/D90458#inline-846536

In short - it is easier to assign sections to segments when they are sorted by file offsets.

Also, sorting will help to reduce the noise from D91152 patch, which starts producing the "Offset" key
and misordered sections confuses the algorithm that calculates the expected offset basing on previous section.

I.e. having a rule that sections are sorted helps a bit for the following logic and helps to keep it simpler.

I'm a little confused here. If we have an ELF with sections .foo, .bar and .zed in that order in the section header table, but with offsets 0x300, 0x200, and 0x100 respectively, I'd expect the obj2yaml output to mantain at least the offset property (the section header table order is arguably less important, although if that order can be maintained too, great). This change appears to be changing the offsets of the sections. At least in section-headers.yaml the input offsets are 0x100, 0x200, 0x300, for .foo, .bar, .zed in that order, whereas the output offsets are 0x300, 0x100, 0x200. This isn't a faithful representation of the input. The thing that could be reordered is the Sections: array. I think either it could be ordered to match the section header table order (in which case there is no need for the SectionHeaderTable key probably), or it should be ordered according to Offset (in which case a SectionHeaderTable key could be added to maintain the table order).

grimar planned changes to this revision.Nov 12 2020, 2:50 AM

I'm a little confused here. If we have an ELF with sections .foo, .bar and .zed in that order in the section header table, but with offsets 0x300, 0x200, and 0x100 respectively, I'd expect the obj2yaml output to mantain at least the offset property (the section header table order is arguably less important, although if that order can be maintained too, great). This change appears to be changing the offsets of the sections.

Actually I am confused too. It definetely was not my intention to change the offsets and I had such bug when developed this patch and was sure that fixed it.
Something is wrong here. I'll recheck after finish with D91152.

grimar updated this revision to Diff 308307.Nov 30 2020, 3:24 AM
grimar retitled this revision from [obj2yaml] - Sort sections by file offsets when dumping them. to [obj2yaml] - Teach tool to emit the "SectionHeaderTable" key and sort sections by file offset..
grimar edited the summary of this revision. (Show Details)
  • Updated implementation. Now this patch also emits the "SectionHeaderTable" key in according to the description.

I think you might need some test coverage for fills too? At least the code you're working on works with Chunk instances, so it implies that both Sections and Fills can reach this bit.

llvm/test/tools/obj2yaml/ELF/section-headers.yaml
1

I'm not sure you really need this test. offset.yaml provides the same level of coverage and actually provides better coverage (it also shows that the section header table isn't printed when the sections are already in order).

I think you might need some test coverage for fills too? At least the code you're working on works with Chunk instances, so it implies that both Sections and Fills can reach this bit.

obj2yaml doesn't emit fills. It only dumps Sections from an object .

grimar updated this revision to Diff 308576.Dec 1 2020, 1:30 AM
  • Addressed review comment (removed test case).
This revision is now accepted and ready to land.Dec 1 2020, 1:55 AM