This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj/llvm-readelf][test] - Add testing for EI_OSABI and EI_ABIVERSION fields of an ELF header.
ClosedPublic

Authored by grimar on Dec 20 2019, 6:41 AM.

Diff Detail

Event Timeline

grimar created this revision.Dec 20 2019, 6:41 AM

I wouldn't go as far as to say the fields were never tested - they are partially, just not exhaustively tested in ELF/file-headers.test.

llvm/test/tools/llvm-readobj/ELF/file-header-os-abi-version.test
14 ↗(On Diff #234889)

a maximum -> the maximum

52 ↗(On Diff #234889)

I think OS/ABI testing should be split into a separate file from ABIVersion, as the two are separate fields.

60 ↗(On Diff #234889)

Nit: here and throughout, it would be nice if the "OS/ABI" bit lined up:

# OSABI-NONE-LLVM: OS/ABI: SystemV (0x0){{$}}
# OSABI-NONE-GNU:  OS/ABI: UNIX - System V{{$}}
grimar edited the summary of this revision. (Show Details)Dec 23 2019, 2:39 AM
grimar updated this revision to Diff 235123.Dec 23 2019, 3:29 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
MaskRay added inline comments.Dec 23 2019, 11:21 AM
llvm/test/tools/llvm-readobj/ELF/file-header-abi-version.test
39

The comment may be redundant... As 52 and 0x34 appear on adjacent lines above. It should be obvious.

llvm/test/tools/llvm-readobj/ELF/file-header-os-abi.test
8

It looks like {{$}} can be avoided by using FileCheck --match-full-line.

grimar updated this revision to Diff 235208.Dec 24 2019, 1:19 AM
grimar marked 2 inline comments as done.
  • Addressed comments.
MaskRay accepted this revision.Dec 24 2019, 2:36 PM
This revision is now accepted and ready to land.Dec 24 2019, 2:36 PM
This revision was automatically updated to reflect the committed changes.

Two post-commit nits, otherwise this was fine, thanks!

llvm/test/tools/llvm-readobj/ELF/file-header-abi-version.test
2

how the ABI version field

llvm/test/tools/llvm-readobj/ELF/file-header-os-abi.test
2

how the OS/ABI

Two post-commit nits, otherwise this was fine, thanks!

I've addressed them in https://reviews.llvm.org/rGec6579fc047f, thanks!