This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Refine program headers testing (`-l`, `--program-headers` and `--segments`).
ClosedPublic

Authored by grimar on Aug 7 2020, 7:27 AM.

Details

Summary

We have gnu-phdrs.test that tests
-l, --program-headers and --segments options for llvm-readelf.

We also have:

  1. program-headers.test that tests --program-headers and -l for llvm-readobj. It doesn't test --segments and also uses lots of precompiled objects. It is very incomplete in compare with the gnu-phdrs.test.
  1. pt-gnu-property.test that contains a simple test for the PT_GNU_PROPERTY segment. There is no reason to have it in a separate file.

This patch:

  1. Merges program-headers.test and pt-gnu-property.test to gnu-phdrs.test.
  2. Removes 2 precompiled binaries used by program-headers.test (other ones are still used by another tests).

Diff Detail

Event Timeline

grimar created this revision.Aug 7 2020, 7:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar requested review of this revision.Aug 7 2020, 7:27 AM

There are a few typos in the description: --segment -> --segments

Merges program-headers.test and pt-gnu-property.test to gnu-phdrs.test.

Removes 2 precompiled binaries used by program-headers.test (other ones are still used by another tests).

Makes sense. Thanks!

Except that.... program-headers.test has some llvm-readobj RUN lines. Note that we still need some representative tests for the LLVM style.

I am also not sure if we want to rename gnu-phdrs.test to something (e.g to program-headers.test) in this patch or is it better to do in a "NFC" follow-up? (for better commit history and to avoid confusion).

Do you want to add some llvm-readobj RUN lines to gnu-phdrs.test so that it will become a misnomer to call the test gnu-*?

Except that.... program-headers.test has some llvm-readobj RUN lines. Note that we still need some representative tests for the LLVM style.

Yeah, that is why I've added llvm-readobj testing to gnu-phdrs.test

I am also not sure if we want to rename gnu-phdrs.test to something (e.g to program-headers.test) in this patch or is it better to do in a "NFC" follow-up? (for better commit history and to avoid confusion).

Do you want to add some llvm-readobj RUN lines to gnu-phdrs.test so that it will become a misnomer to call the test gnu-*?

I've already did it in this patch. So yes, currently it's name is gnu-*, though it tests LLVM style too now.

grimar retitled this revision from [llvm-readobj] - Refine program headers testing (`-l`, `--program-headers` and `--segment`). to [llvm-readobj] - Refine program headers testing (`-l`, `--program-headers` and `--segments`)..Aug 10 2020, 1:06 AM
grimar edited the summary of this revision. (Show Details)
MaskRay accepted this revision.Aug 10 2020, 9:06 AM

I am also not sure if we want to rename gnu-phdrs.test to something (e.g to

program-headers.test) in this patch or is it better to do in a "NFC" follow-up?
(for better commit history and to avoid confusion).

I don't have a strong opinion. Either way may be fine. program-headers.test as the final name makes sense.

This revision is now accepted and ready to land.Aug 10 2020, 9:06 AM

I think for diffing purposes, it would probably be best to rename in a separate commit.

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
21–26

Could these just change to calling FileCheck with the text file as an input file:

# RUN: FileCheck %s --check-prefixes=ELF32,MAPPING --strict-whitespace --match-full-lines --input-file=$t.readelf-l.txt
# RUN: FileCheck %s --check-prefixes=ELF-LLVM,ELF32-LLVM --input-file=%t.readobj-l.txt

Also, it might be wise to add at least --match-full-lines to all llvm-readobj checks, because it will prevent numeric values spuriously matching when there are more digits. For example, without it CHECK: Value: 0x1234 would also match Value: 0x12345678, I believe.

225

(Aside: I'm sure you've noticed there's a bug here and in similar cases below - we probably should be printing "<unknown>" or similar)

grimar updated this revision to Diff 284683.Aug 11 2020, 6:03 AM
grimar marked an inline comment as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
21–26

Done.

225

Probably we should print "Unknown". This is what we do for --sections. E.g:

Type: Unknown (0x1C7)
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test