Page MenuHomePhabricator

[Object/llvm-readobj] - Cleanup testing of the dynamic objects.
ClosedPublic

Authored by grimar on Jul 22 2019, 2:36 AM.

Details

Summary

This patch touches a few test cases:

  1. It removes dtflags.elf-x86-64 binary and elf-dtflags.test.

elf-dtflags.test is excessive because we have the
elf-dynamic-tags.test which test all non-machine specific tags.

  1. It removes testing of --dynamic-table from test\Object\readobj-shared-object.test

(we have the elf-dynamic-tags.test for that), and simplifies this test case.

  1. It moves testing of the headers from readobj-shared-object.test

to elf-file-headers.test.

  1. Adds test/tools/llvm-readobj/elf-file-types.test and test/tools/llvm-readobj/elf-loadname.test.

It opens road for removing the readobj-shared-object.test completely soon.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 22 2019, 2:36 AM
grimar edited the summary of this revision. (Show Details)Jul 22 2019, 2:36 AM
grimar edited the summary of this revision. (Show Details)Jul 22 2019, 2:38 AM
jhenderson added inline comments.Jul 22 2019, 10:00 AM
test/Object/readobj-shared-object.test
15–18 ↗(On Diff #211031)

Do we have testing elsewhere for this part of the printing?

test/tools/llvm-readobj/elf-dynamic-tags.test
1 ↗(On Diff #211031)

Seems like this should be a separate NFC commit? This file is otherwise unchanged.

test/tools/llvm-readobj/elf-file-headers.test
1 ↗(On Diff #211031)

It feels like the machine type and ELF type are two orthoganol test cases that don't need combinatorially testing. Why not also test ET_EXEC and ET_CORE in this manner after all?

Perhaps a separate test file would be sensible, with tests for each different ELF type, focused just on testing that field?

grimar updated this revision to Diff 211262.Jul 23 2019, 2:09 AM
grimar marked 4 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
test/Object/readobj-shared-object.test
15–18 ↗(On Diff #211031)

Probably not. I Added elf-loadname.test.

test/tools/llvm-readobj/elf-dynamic-tags.test
1 ↗(On Diff #211031)

I just tried to show this test case in the commit somehow since I am referring to it.
Reverted.

test/tools/llvm-readobj/elf-file-headers.test
1 ↗(On Diff #211031)

I added a separate test.

jhenderson added inline comments.Jul 23 2019, 3:28 AM
test/tools/llvm-readobj/elf-file-types.test
88 ↗(On Diff #211262)

You should probably also test something in the ET_LOOS -> ET_HIOS range too.

grimar marked an inline comment as done.Jul 23 2019, 3:33 AM
grimar added inline comments.
test/tools/llvm-readobj/elf-file-types.test
88 ↗(On Diff #211262)

Interesting that ELF.h doesn't define ET_LOOS/ET_HIOS:

// File types
enum {
  ET_NONE = 0,        // No file type
  ET_REL = 1,         // Relocatable file
  ET_EXEC = 2,        // Executable file
  ET_DYN = 3,         // Shared object file
  ET_CORE = 4,        // Core file
  ET_LOPROC = 0xff00, // Beginning of processor-specific codes
  ET_HIPROC = 0xffff  // Processor-specific
};
grimar updated this revision to Diff 211275.Jul 23 2019, 4:17 AM
  • Addressed review comment.
This revision is now accepted and ready to land.Jul 23 2019, 4:55 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2019, 5:21 AM