This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Verify that sections are sorted by their file offsets when creating segments.
ClosedPublic

Authored by grimar on Apr 17 2020, 5:30 AM.

Details

Summary

This validates that sections listed for a segment in the YAML
declaration are ordered by their file offsets.

It might help to simplify the file size computation, but also
is useful by itself as helps to avoid issues in test cases and
to maintain their readability.

Diff Detail

Event Timeline

grimar created this revision.Apr 17 2020, 5:30 AM
grimar updated this revision to Diff 258297.Apr 17 2020, 5:37 AM
  • Add the forgotten test case.
grimar updated this revision to Diff 258299.Apr 17 2020, 5:49 AM
  • Last minute fixes.

Can the tool sort program headers by p_offset so that reordered user input can be accepted? It may sometimes be useful to check how other tools behave with reordered input. This patch is dropping this feature.

Can the tool sort program headers by p_offset so that reordered user input can be accepted? It may sometimes be useful to check how other tools behave with reordered input. This patch is dropping this feature.

This patch fixed issues in 5 of test cases, I believe they did not expect to have reordered sections. This is what is really useful.

It may sometimes be useful to check how other tools behave with reordered input.

I am not sure what do you mean, could you elaborate? Are you going to use YAMLs as inputs for another tools? Which tools? Are them a part of LLVM? Do we have any test cases?

jhenderson added inline comments.Apr 20 2020, 12:57 AM
llvm/test/tools/obj2yaml/program-headers.yaml
419 ↗(On Diff #258299)

the file offset -> their offsets

(sections only have one kind of offset field defined, so I don't think you need to say "file")

441 ↗(On Diff #258299)

This isn't really a "Case", as that implies it's actually something that is testing functionality itself. I'd just get rid of the Case labels in this test.

grimar updated this revision to Diff 258699.Apr 20 2020, 4:07 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
MaskRay accepted this revision.Apr 20 2020, 12:04 PM

Can the tool sort program headers by p_offset so that reordered user input can be accepted? It may sometimes be useful to check how other tools behave with reordered input. This patch is dropping this feature.

This patch fixed issues in 5 of test cases, I believe they did not expect to have reordered sections. This is what is really useful.

It may sometimes be useful to check how other tools behave with reordered input.

I am not sure what do you mean, could you elaborate? Are you going to use YAMLs as inputs for another tools? Which tools? Are them a part of LLVM? Do we have any test cases?

Ah, I misread the patch. The check does not prevent any feature.

This revision is now accepted and ready to land.Apr 20 2020, 12:04 PM
jhenderson accepted this revision.Apr 21 2020, 12:28 AM

LGTM.

llvm/test/tools/obj2yaml/program-headers.yaml
419 ↗(On Diff #258699)

offset -> offsets

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 5:54 AM