This is an archive of the discontinued LLVM Phabricator instance.

Differential D86350

[llvm-readobj][test] - Test "Format" values.
ClosedPublic

Authored by grimar on Aug 21 2020, 7:11 AM.

Details

Reviewers
jhenderson
MaskRay
• espindola
Commits
rG4cd14256b23a: [llvm-readobj][test] - Test "Format" values.
Summary

This adds testing for the "Format" field printed with --file-headers.

llvm-readelf doesn't use them, so only llvm-readobj needs to be tested.

All possible values are defined in ELFObjectFile<ELFT>::getFileFormatName().
and tested by D86625. Here we test just a few arbitrary ones.

Diff Detail

Event Timeline

grimar created this revision.Aug 21 2020, 7:11 AM
Herald added a reviewer: • espindola. · View Herald TranscriptAug 21 2020, 7:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: s.egerton, rupprecht, simoncook and 2 others. · View Herald Transcript
grimar requested review of this revision.Aug 21 2020, 7:11 AM
grimar added a comment.Aug 26 2020, 1:51 AM

Ping.

jhenderson added a comment.Aug 26 2020, 2:59 AM

This is essentially testing the getFileFormatName function in ELFObjectFile.h, which is part of the Object library, not part of llvm-readobj. I'm not sure exhaustively testing all the possible values should be done in an llvm-readobj test. There should be an llvm-readobj test that shows that the Format is printed for one or two given values, but the getFileFormatName function should be tested in Object tests (probably best in gtest unit tests), in my opinion.

grimar planned changes to this revision.Aug 26 2020, 5:56 AM

OK. I'll prepare a different patch and update this later.

grimar updated this revision to Diff 287978.Aug 26 2020, 8:08 AM
grimar retitled this revision from [llvm-readobj][test] - Test all possible "Format" values. to [llvm-readobj][test] - Test "Format" values..
grimar edited the summary of this revision. (Show Details)
  • Addressed review comment.
jhenderson accepted this revision.Aug 27 2020, 12:05 AM

LGTM.

llvm/test/tools/llvm-readobj/ELF/file-header-format.test
1
27

If you're going to use the default for CLASS and DATA, I'd do the same for MACHINE too, with EM_NONE as the default. Up to you though.

This revision is now accepted and ready to land.Aug 27 2020, 12:05 AM
grimar mentioned this in D86625: [unittests/Object] - Add testing for missing ELF formats..Aug 27 2020, 2:36 AM
Closed by commit rG4cd14256b23a: [llvm-readobj][test] - Test "Format" values. (authored by grimar). · Explain WhyAug 28 2020, 1:33 AM
This revision was automatically updated to reflect the committed changes.
grimar added a commit: rG4cd14256b23a: [llvm-readobj][test] - Test "Format" values..

Revision Contents

PathSize
llvm/
test/
tools/
llvm-readobj/
ELF/
27 lines

Diff 287978

llvm/test/tools/llvm-readobj/ELF/file-header-format.test

  • This file was added.
## This testss "Format" values printed by llvm-readobj.
jhendersonUnsubmitted
Not Done
ReplyInline Actions
- ## This testss "Format" values printed by llvm-readobj.
+ ## This tests "Format" values printed by llvm-readobj.
## The "Format" value printed depends on values stored in e_ident[EI_CLASS],
jhenderson:
## The "Format" value printed depends on values stored in e_ident[EI_CLASS],
## e_ident[EI_DATA] and e_machine fields of an ELF file header.
## We test all possible values in unittests\Object\ELFObjectFileTest.cpp,
## here we test just a few of them to demonstrate that llvm-readobj prints the same.
# RUN: yaml2obj %s -o %t.none64.o -DMACHINE=EM_NONE
# RUN: llvm-readobj --file-headers %t.none64.o | FileCheck %s -DFMT="elf64-unknown"
# RUN: yaml2obj %s -o %t.none32.o -DMACHINE=EM_NONE -DCLASS=32
# RUN: llvm-readobj --file-headers %t.none32.o | FileCheck %s -DFMT="elf32-unknown"
# RUN: yaml2obj %s -o %t.i386-32.o -DMACHINE=EM_386 -DCLASS=32
# RUN: llvm-readobj --file-headers %t.i386-32.o | FileCheck %s -DFMT="elf32-i386"
# RUN: yaml2obj %s -o %t.x86_64.o -DMACHINE=EM_X86_64
# RUN: llvm-readobj --file-headers %t.x86_64.o | FileCheck %s -DFMT="elf64-x86-64"
# CHECK: Format: [[FMT]]
--- !ELF
FileHeader:
Class: ELFCLASS[[CLASS=64]]
Data: ELFDATA2[[DATA=LSB]]
Type: ET_REL
Machine: [[MACHINE]]
jhendersonUnsubmitted
Not Done
ReplyInline Actions

If you're going to use the default for CLASS and DATA, I'd do the same for MACHINE too, with EM_NONE as the default. Up to you though.

jhenderson: If you're going to use the default for CLASS and DATA, I'd do the same for MACHINE too, with…