This is an archive of the discontinued LLVM Phabricator instance.

[llvm\test\Object] - An initial step to cleanup the test cases.
ClosedPublic

Authored by grimar on Jul 4 2019, 6:42 AM.

Details

Summary

This patch removes trivial-object-test.elf-i386,
trivial-object-test.elf-x86-64 and trivial-object-test2.elf-x86-64
precompiled objects from test/Object/Inputs folder.

I adjusted the existent test cases to use YAML instead.

Notes:

  1. I tried not to change the check lines and sections/symbol names.

But I reduced the YAMLs produced from these objects significantly.

  1. It was not my intention to add any comments, so I did not do that.

(see 3)

  1. Most of these tests are not on their places. I think many of them

should be moved to test/tools/llvm-nm, test/tools/objdump and so on.
Perhaps some of them can be removed, I guess they are duplicating the existent
tests in the folders mentioned.

I did not try to resolve 1-3 here, because this patch is already probably
too hard to read/review and such cleanups should be done in a follow-ups.

I would like to get rid of precompiled objects at first.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 4 2019, 6:42 AM
grimar edited the summary of this revision. (Show Details)Jul 4 2019, 6:43 AM

Thanks for working on this! I agree with your assessment that some of this testing is probably redundant. It's a little less clear in some cases whether the testing belongs in the Object or tool test directories, because it might be that the test is exercising a part of the Object library really rather than the tool. However, as you say, that is a different change.

test/Object/X86/objdump-disassembly-inline-relocations.test
106 ↗(On Diff #208024)

This line can be deleted.

150 ↗(On Diff #208024)

Ditto.

test/Object/archive-symtab.test
70 ↗(On Diff #208024)

I think it might be good to check the full file path here, using FileCheck's -D option:

... FileCheck -DFILE=%/t.elf-x86-64 --check-prefix=THIN

...
# THIN-NEXT: main in [[FILE]]

Same might apply for other thin archive cases, or where a full path is being printed. Not sure how important it is though.

118 ↗(On Diff #208024)

Could you fix this comment line please (repeate -> repeat, and trailing full stop).

test/Object/nm-trivial-object.test
65 ↗(On Diff #208024)

Consider using FileCheck -D here too.

69 ↗(On Diff #208024)

It's probably worth putting this test case back before the above one, to match its original ordering.

104 ↗(On Diff #208024)

I assume you're holding off changing this to YAML for now?

152 ↗(On Diff #208024)

Should this block be moved up to immediately after the RUN, like you did with the others?

Same could probably be said about each of the other ones from this point on.

test/Object/objdump-file-header.test
5 ↗(On Diff #208024)

Here and the ELF one, you should probably write {{$}} at the end, to show that there are no more 0s than expected.

test/Object/objdump-relocations.test
9 ↗(On Diff #208024)

Any particular reason you've changed the check lines in this test?

test/Object/objdump-section-content.test
32 ↗(On Diff #208024)

Probably worth moving these check lines up to near their use point.

test/Object/objdump-sectionheaders.test
14 ↗(On Diff #208024)

Can you find any reasoning in history as to why the .symtab etc had a VMA before?

test/Object/readobj.test
2 ↗(On Diff #208024)

Perhaps this needs to be a "does nothing if no operations requested" test, if it doesn't exist already?

grimar edited the summary of this revision. (Show Details)Jul 4 2019, 9:56 AM
MaskRay added inline comments.Jul 4 2019, 8:33 PM
test/Object/X86/objdump-disassembly-inline-relocations.test
106 ↗(On Diff #208024)

I think James meant the Address: 0x0000000000000034 line.

test/Object/X86/objdump-trivial-object.test
7 ↗(On Diff #208024)

3: c7 44 24 08 00 00 00 00 movl $0, 8(%esp) then align the rest lines.

21 ↗(On Diff #208024)

4: c7 44 24 24 00 00 00 00 movl $0, 36(%rsp) then align the rest lines.

35 ↗(On Diff #208024)

3: c7 44 24 08 00 00 00 00 movl $0, 8(%esp)

55 ↗(On Diff #208024)

i386 -> x86_64

61 ↗(On Diff #208024)

4: c7 44 24 04 00 00 00 00 movl $0, 4(%rsp)

test/Object/archive-symtab.test
114 ↗(On Diff #208024)

##

test/Object/mri-addmod.test
3 ↗(On Diff #208024)

echo 'addmod "%t.elf-x86-64"'

or echo -e '..\n..' if you join the two lines

test/Object/nm-shared-object.test
33 ↗(On Diff #208024)

.elf-i386 to be consistent with previous tests in the same file.

test/Object/objdump-section-content.test
3 ↗(On Diff #208024)

Delete the suffix or use .coff-i386?

jhenderson added inline comments.Jul 5 2019, 1:52 AM
test/Object/X86/objdump-disassembly-inline-relocations.test
106 ↗(On Diff #208024)

Nope, I meant the Link line. I believe that yaml2obj automatically links the section to .symtab unless told otherwise.

grimar marked an inline comment as done.Jul 5 2019, 1:55 AM
grimar added inline comments.
test/Object/X86/objdump-disassembly-inline-relocations.test
106 ↗(On Diff #208024)

Both of these lines can be removed :)

MaskRay added inline comments.Jul 5 2019, 2:57 AM
test/Object/readobj.test
2 ↗(On Diff #208024)

test/tools/llvm-readobj/elf-dynamic-* test various .dynamic features. This file can be moved there if you feel strong about keeping it.

grimar updated this revision to Diff 208135.Jul 5 2019, 3:47 AM
grimar marked 25 inline comments as done.
  • Addressed review comments.
grimar added inline comments.Jul 5 2019, 3:47 AM
test/Object/nm-trivial-object.test
104 ↗(On Diff #208024)

Yes, I am only converting 3 objects: trivial-object-test.elf-i386,
trivial-object-test.elf-x86-64 and trivial-object-test2.elf-x86-64 in this patch.

test/Object/objdump-relocations.test
9 ↗(On Diff #208024)

No. This was the test I tried to clean up initially, I found no point to check the R_386_PC32 relocation twice
and supposed that just testing relocation against a symbol and against a section is cleaner.
But then I realized that do not want to improve/rework the YAMLs in this patch actually (at least because I guess these tests are incomplete, might duplicate the existent tests in test/tools/*, missing comments and so on, a much more work should be done here),
so I did not do any major YAML reworks in other tests. This one is still probably a bit better than it was, so I left the result.
Do you think it worth to get it back to original but reduced YAML?

test/Object/objdump-section-content.test
3 ↗(On Diff #208024)

I assume you meant .elf-i386. Done.

test/Object/objdump-sectionheaders.test
14 ↗(On Diff #208024)

No, but I have a guess: trivial-object-test.elf-x86-64 was introduced in the begining of the 2011: https://reviews.llvm.org/rL123899.
I am not sure how it was produced, but it could be just a bug, i.e. perhaps some tool, may be llvm-mc, assigned an addresses to non-allocatable sections that time.

test/Object/readobj.test
2 ↗(On Diff #208024)

It was introduced in 2013: https://reviews.llvm.org/rL174639 and its intention seems was just to test that llvm-readobj doesn't crash. I introduced test/tools/llvm-readobj/elf-no-action.test instead.

MaskRay added inline comments.Jul 5 2019, 4:16 AM
test/Object/X86/objdump-trivial-object.test
61 ↗(On Diff #208024)

00 movl

Since you are updating this test, it may be good to make the number of spaces consistent with the actual output.

grimar updated this revision to Diff 208170.Jul 5 2019, 6:48 AM
grimar marked an inline comment as done.
  • Addressed comment.
test/Object/X86/objdump-trivial-object.test
61 ↗(On Diff #208024)

Ok. I was not sure this makes much sence until -strict-whitespace is used for FileCheck invocations actually,
but I have no strong objections if you feel this is better.

MaskRay accepted this revision.Jul 5 2019, 7:03 AM

LGTM if @jhenderson's comments are addressed.

test/Object/X86/objdump-trivial-object.test
61 ↗(On Diff #208024)

I just wondered why you changed the number of spaces but didn't make it look like the actual output..

This revision is now accepted and ready to land.Jul 5 2019, 7:03 AM
jhenderson accepted this revision.Jul 8 2019, 8:13 AM

LGTM, with one nit.

test/Object/objdump-relocations.test
9 ↗(On Diff #208024)

I think it's fine as is.

test/tools/llvm-readobj/elf-no-action.test
1 ↗(On Diff #208170)

Nit: '##'.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 9:57 AM
llvm/trunk/test/Object/objdump-symbol-table.test