Page MenuHomePhabricator

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

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

Details

Reviewers
jhenderson
shchenz
Higuoxing
DiggerLin
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

Esme created this revision.Tue, Nov 23, 4:10 AM
Esme requested review of this revision.Tue, Nov 23, 4:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 23, 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
5–6

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.Fri, Nov 26, 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
5–6

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.