This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj][test] - Remove --symbols --dyn-syms part from Object/readobj-shared-object.test.
ClosedPublic

Authored by grimar on Jan 22 2020, 4:59 AM.

Details

Summary

The intention of Object/readobj-shared-object.test was to check the
general output for shared object.

I've added a case for testing dynamic objects to ELF/symbols.test.
Also we already test dynamic symbols printing in ELF/dyn-symbols.test +
I've added a case for --dyn-syms alias in D73164.

Hence we can remove this piece from Object/readobj-shared-object.test.

Diff Detail

Event Timeline

grimar created this revision.Jan 22 2020, 4:59 AM
Herald added a project: Restricted Project. · View Herald Transcript

Can we delete the test/Object test and move the remaining --needed-libs pieces to test/tools/llvm-readobj/ELF/ ?

Can we delete the test/Object test and move the remaining --needed-libs pieces to test/tools/llvm-readobj/ELF/ ?

That is my intention, I posted D73174 to move the --needed-libs and after this patch we will be able to remove the test.
I'd probably do it in a separate commit saying that "we are testing --sections already elsewhere" though.

MaskRay accepted this revision.Jan 24 2020, 12:16 AM
This revision is now accepted and ready to land.Jan 24 2020, 12:16 AM
jhenderson added inline comments.Jan 24 2020, 1:54 AM
llvm/test/tools/llvm-readobj/ELF/symbols.test
12

It feels weird to me that we test relocatable and shared objects, but not regular ET_EXEC executables. Why do shared objects need specifically testing?

grimar marked 2 inline comments as done.Jan 24 2020, 2:05 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/symbols.test
12

I'd say it is wierd that we test relocatable object with dynamic symbols (--dyn-symbols, below) as they normally
do not have them. Perhaps having a test for both ET_DYN and ET_REL does not hurt to document the behavior?
I am not sure if we should add a test for ET_EXEC as it looks similar to ET_DYN in the context of this patch to me.
What do you think?

jhenderson added inline comments.Jan 24 2020, 4:00 AM
llvm/test/tools/llvm-readobj/ELF/symbols.test
12

I'd say it is wierd that we test relocatable object with dynamic symbols (--dyn-symbols, below) as they normally do not have them.

Fair point. I think one or more of the tools may have only allowed options like this for shared objects at one point. I don't know if that's still the case though. Perhaps a comment is needed to explain why we picked those two formats.

grimar updated this revision to Diff 240504.Jan 27 2020, 2:14 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
MaskRay added inline comments.Jan 27 2020, 10:00 AM
llvm/test/tools/llvm-readobj/ELF/symbols.test
9

dynamic libraries

-> The difference between ET_EXEC and ET_DYN

ET_DYN can also be used by position-independent executables.

MaskRay accepted this revision.Jan 27 2020, 10:00 AM
This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.