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
1–3

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).

44–45

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?

287

Let's assign these some (arbitrary) values.

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

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
44–45

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