This is an archive of the discontinued LLVM Phabricator instance.

[NFC][XCOFF] [llvm-readobj] replace binaries with YAMLs (only tests for Symbols)
ClosedPublic

Authored by Esme on Nov 23 2021, 4:10 AM.

Diff Detail

Event Timeline

Esme created this revision.Nov 23 2021, 4:10 AM
Esme requested review of this revision.Nov 23 2021, 4:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 4:10 AM

A patch like this should be marked with "NFC" in the title

llvm/test/tools/llvm-readobj/XCOFF/symbols64.test
94

Maybe use a meaningful date here?

100

We don't need to mention IBM XLC V16.1 now. Please use IBM Open XL instead.

107

whitespace at the end?

115

The length seems not right to me.

134

Same as above

153

Same as above

Mostly looks good, but I think we can simplify the test a little whilst we're here, as suggested in a couple of places inline.

My comments in the two tests apply to both tests, so please address in both cases/respond for both cases.

llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
4–5

I'm not necessarily opposed to this being a separate file, although I think it's more common in newer tests to keep invalid behaviour checks in the same files as their corresponding regular behaviour.

Also the %t5 needs changing to just %t.

llvm/test/tools/llvm-readobj/XCOFF/symbols.test
30

Should this be CSECT rather than SECT?

90

The addition of the # characters has made this a little hard to review. Are there any actual changes in the checks performed, or is the output semantically identical?

llvm/test/tools/llvm-readobj/XCOFF/symbols64.test
88

The 8 sounds important. I'd change to just test.c

94

More preceisely, as this is just a free string in the YAML, let's just change it to something more obviously arbitrary e.g. "foo"

100

Does the test actually need this many auxiliary entries to exercise the llvm-readobj behaviour appropriately? If so, as above, just use an arbitrary string here e.g. "bar".

Esme updated this revision to Diff 389934.Nov 26 2021, 1:15 AM
Esme marked 6 inline comments as done.
Esme retitled this revision from [XCOFF] [llvm-readobj] replace binaries with YAMLs (only tests for Symbols) to [NFC][XCOFF] [llvm-readobj] replace binaries with YAMLs (only tests for Symbols).

Address comments.

llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
4–5

Thanks for your comments!
I prefer to separate invalid behavior checks, because it's convenient to add other invalid checks in follow-up works.

llvm/test/tools/llvm-readobj/XCOFF/symbols.test
30

It should be Section auxiliary entry, and abbreviated as SECT auxiliary entry.

90

I removed duplicated checks for a given symbol type and added a check for C_WEAKEXT that were not tested.

llvm/test/tools/llvm-readobj/XCOFF/symbols64.test
115

The length value is correct in fact.
The section length value in the CSECT Auxiliary Entry for XCOFF64 is composed of two fields, i.e. x_scnlen_lo and x_scnlen_hi, that are SectionOrLengthLo and SectionOrLengthHi in YAML.
I set SectionOrLengthLo: 4 and SectionOrLengthHi: 5 in YAML, and 21474836484 equals to 0x500000004, so the value is right.

jhenderson accepted this revision.Nov 30 2021, 1:14 AM

LGTM, but please wait for @shchenz too.

This revision is now accepted and ready to land.Nov 30 2021, 1:14 AM
shchenz accepted this revision.Nov 30 2021, 10:46 PM

LGTM too. Thanks for cleaning up.

llvm/test/tools/llvm-readobj/XCOFF/symbols64.test
115

Ah, OK. I didn't notice that this big length is coming from the YAML. Thanks for the explanation.

This revision was landed with ongoing or failed builds.Jan 10 2022, 10:18 PM
This revision was automatically updated to reflect the committed changes.