This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj][test] - Stop using Inputs/trivial.obj.elf-x86-64.
ClosedPublic

Authored by grimar on Dec 18 2019, 7:03 AM.

Details

Summary

This rewrites a few tests to stop using the
trivial.obj.elf-x86-64 precompiled object
and removes it.

Diff Detail

Event Timeline

grimar created this revision.Dec 18 2019, 7:03 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar retitled this revision from [llvm-readobj] - Stop using Inputs/trivial.obj.elf-x86-64. to [llvm-readobj][test] - Stop using Inputs/trivial.obj.elf-x86-64..Dec 18 2019, 7:04 AM
jhenderson added inline comments.Dec 19 2019, 1:23 AM
llvm/test/tools/llvm-readobj/ELF/Inputs/trivial.ll
4–5

Is the EM_386 ELF still used anywhere?

llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test
1

Delete "a"

6–7

You should probably either add {{$}} to the end of each line or use --match-full-lines to show there's no more data after that being checked on each line. If there were, it might not get caught by this test.

14

Maybe a separate change, but we should have a big-endian ELF tested in here too.

16–17

We should have at least one test case with non-zero OS/ABI and ABI Version fields. Possibly we could test those separately (we should for example probably enumerate the OS/ABI values in a separate test to show that they are all GNU compatible).

21

Non-zero entry point?

24

Non-zero flags?

44–45

For the purpose of this test, you don't need to have any sections in the segment.

47

Similar comments to above apply to this and the MIPS case, but you don't need to necessarily test everything.

89

Any particular reason you felt the need to test EM_MIPS?

llvm/test/tools/llvm-readobj/ELF/gnu-section-mapping-no-phdrs.test
1–18

in an object

5

I think this line is also important to this test.

7

This needs a {{$}} or --match-full-lines to prevent there being any other data on the same line.

llvm/test/tools/llvm-readobj/ELF/hex-dump.test
4

Don't we need a check somewhere that shows that dumping .strtab produces expected output?

24–25

These two RUN lines need an associated comment to say what they're testing.

25–30

This should be moved up to its associated RUN line and a comment added explaining what its purpose is.

llvm/test/tools/llvm-readobj/ELF/thin-archive-paths.test
1

(Aside: I'm not sure this test should be in the ELF directory. It is testing generic functionality)

grimar updated this revision to Diff 234699.Dec 19 2019, 4:30 AM
grimar marked 25 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/Inputs/trivial.ll
4–5

Yes, in a few tests.

llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test
14

I am trying just to get rid of precompiled object for now. Such improvements should be done in a new separate test.

16–17

I agree that these fields should be tested and that we should do this separatelly in a more specific tests.

24

There are no specific flags for x64 and i386 and yaml2obj does not allow to use an arbitrary value.
I think we should test this separatelly anyways though (but see my comment about MIPS below).

(funny that current logic of obj2yaml crashes with llvm-unreachable if the description has EM_386
or other unsupported machine type and a Flags property:
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L426)

89

I just tried to convert the original test and for some reason it had a test for MIPS.
I supposed it just was testing the possibility to dump MIPS headers,
but it turns out that it was introduced to check we can dump the Flags in rL345238...
I've updated the comment and added original flags here for now.

llvm/test/tools/llvm-readobj/ELF/hex-dump.test
4

I've changed .strtab to .shstrtab, which is dumped below.

24–25

It had the original comment saying "Test we dump the section only once.". I've expanded it.

llvm/test/tools/llvm-readobj/ELF/thin-archive-paths.test
1

I'll move it out.

jhenderson added inline comments.Dec 19 2019, 5:34 AM
llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test
16–17

Okay, that's fine, but I do want to make sure it doesn't get overlooked, because at least in my most common usage, we use non-zero values for these two fields.

45

Can't you just delete the line entirely?

llvm/test/tools/llvm-readobj/ELF/hex-dump.test
25–30

The "Both 2..." original comment is probably a bit redundant now that you have the sanity check comment above.

grimar updated this revision to Diff 234891.Dec 20 2019, 6:48 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test
16–17
This revision is now accepted and ready to land.Dec 20 2019, 7:11 AM
This revision was automatically updated to reflect the committed changes.