This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj][llvm-objdump][test] - Cleanup testing of dynamic tags dumping.
ClosedPublic

Authored by grimar on Dec 17 2019, 5:17 AM.

Details

Summary

We have the elf-dynamic-tags-machine-specific.yaml input shared
between the llvm-readobj and llvm-objdump test.
It looks strange, because tools usually does not share inputs.

Also there are following problems related:

  1. elf-dynamic-tags-machine-specific.yaml input contains excessive YAML parts.
  2. objdump's test case never test AARCH64 tags.
  3. There are unknown tags in the elf-dynamic-tags-machine-specific.yaml and dynamic-tags-machine-specific.test, though we already testing unknown tags in llvm-readobj\ELF\dynamic-tags.test and llvm-objdump\elf-dynamic-section.test tests.

This patch removes the shared input and refines the test cases to resolve
issues mentioned.

Diff Detail

Event Timeline

grimar created this revision.Dec 17 2019, 5:17 AM
jhenderson added inline comments.Dec 18 2019, 1:19 AM
llvm/test/tools/llvm-objdump/elf-dynamic-section-machine-specific.test
42–43

I'd move this comment to the first individual line that this applies to. Are you planning on fixing this soon? If not, I'd file a bug and include a link to that bug with this comment.

66

You've removed testing for these unknown values, but is there any testing for them anywhere else?

205–207

I'd suggest some wording changes:

  1. Top-level comment should be something like "In this test we test..."
  2. Individual case comments could be simplified to "Case 1: Hexagon" or "Hexagon tags"

And I wouldn't put a blank line after the comments at the start of each machine case, because the comment is tied to the respective RUN lines (just like you wouldn't put a comment for an if statement in C++ with a blank line between it and the if).

282

Let's assign these some (arbitrary) values.

llvm/test/tools/llvm-readobj/ELF/dynamic-tags-machine-specific.test
1–4

Basically all my coments for the objdump test also apply here too.

grimar edited the summary of this revision. (Show Details)Dec 18 2019, 4:43 AM
grimar updated this revision to Diff 234517.Dec 18 2019, 5:01 AM
grimar marked 7 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-objdump/elf-dynamic-section-machine-specific.test
42–43

Are you planning on fixing this soon?

Yes, I think it worth fixing. Plan to do that in a follow-up for both llvm-objdump and llvm-readelf.

66

Yep. I've mentioned this in the patch description:
"There are unknown tags in the elf-dynamic-tags-machine-specific.yaml, though we already testing unknown tags in \llvm-readobj\ELF\dynamic-tags.test and llvm-objdump\elf-dynamic-section.test tests."

grimar edited the summary of this revision. (Show Details)Dec 18 2019, 5:02 AM
grimar edited the summary of this revision. (Show Details)
jhenderson accepted this revision.Dec 18 2019, 5:25 AM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 18 2019, 5:25 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/tools/llvm-objdump/elf-dynamic-section-machine-specific.test