This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] [llvm-readobj] replace tests using binary as input with tests generated by yaml2obj.
ClosedPublic

Authored by Esme on Oct 13 2021, 2:45 AM.

Diff Detail

Event Timeline

Esme created this revision.Oct 13 2021, 2:45 AM
Esme requested review of this revision.Oct 13 2021, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2021, 2:45 AM
shchenz accepted this revision.Oct 13 2021, 7:19 PM

LGTM with one small nit.

llvm/test/tools/llvm-readobj/XCOFF/relocations.test
63

nit: This is not a good symbol name that the relocation entries will point to. Maybe we need another name. Like foo bar and so on.

This revision is now accepted and ready to land.Oct 13 2021, 7:19 PM
Esme updated this revision to Diff 379600.Oct 13 2021, 8:31 PM

address comment

jhenderson added inline comments.Oct 14 2021, 12:33 AM
llvm/test/tools/llvm-readobj/XCOFF/file-header.test
2

With these basic tests, it's often a good idea to add --strict-whitespace and --match-full-lines to show that the formatting is as desired.

5

Let's make the check explicitly clear.

21–24

The previous version of this test had non zero values for the fields. I think it's important we do the same, because a 0 value could indicate an unset value in the tool code, rather than a value that has been read.

There should probably be a special case for 0 for some fields, where 0 has a special meaning (I'm looking at the TimeStamp specifically).

llvm/test/tools/llvm-readobj/XCOFF/relocations.test
7

Consider adding --strict-whitespace and --match-full-lines to highlight this.

llvm/test/tools/llvm-readobj/XCOFF/sections.test
1
4

As above, consider adding --strict-whitespace and --match-full-lines.

14

As with the file headers case, consider setting non-zero values for all the fields, so that we can exercise dumping properly.

Esme updated this revision to Diff 380272.Oct 17 2021, 7:49 PM
Esme marked 8 inline comments as done.

Thanks! @shchenz and @jhenderson

Esme added inline comments.Oct 17 2021, 7:49 PM
llvm/test/tools/llvm-readobj/XCOFF/file-header.test
21–24

Set non-zero values for all fields (except for the aux file header which is not yet supported).

llvm/test/tools/llvm-readobj/XCOFF/sections.test
14

Set non-zero values for all fields (except for the line number which is not yet supported) in the second section.

This revision was landed with ongoing or failed builds.Nov 1 2021, 1:45 AM
This revision was automatically updated to reflect the committed changes.
jhenderson added inline comments.Nov 2 2021, 1:49 AM
llvm/test/tools/llvm-readobj/XCOFF/sections.test
7

See inline edit, for a tip for the future (no need to change this now unless you want to). This ensures the : characters all line up, meaning your text is all lined up.

llvm/include/llvm/ObjectYAML/XCOFFYAML.h