[PATCH] D64206: [llvm\test\Object] - An initial step to cleanup the test cases.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/Object/nm-trivial-object.test:104
+
+# RUN: llvm-nm %p/Inputs/weak.elf-x86-64 \
+# RUN:         | FileCheck %s -check-prefix WEAK-ELF64
----------------
jhenderson wrote:
> I assume you're holding off changing this to YAML for now?
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.


================
Comment at: test/Object/objdump-relocations.test:9
 
-COFF-i386: .text
-COFF-i386: IMAGE_REL_I386_DIR32 L_.str
-COFF-i386: IMAGE_REL_I386_REL32 _puts
-COFF-i386: IMAGE_REL_I386_REL32 _SomeOtherFunction
+# ELF-i386: .text
+# ELF-i386: R_386_32 .section
----------------
jhenderson wrote:
> Any particular reason you've changed the check lines in this test?
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?


================
Comment at: test/Object/objdump-section-content.test:3
 
-COFF-i386: file format
-COFF-i386: Contents of section .text:
-COFF-i386:  0000 83ec0cc7 44240800 000000c7 04240000  ....D$.......$..
-COFF-i386:  0010 0000e800 000000e8 00000000 8b442408  .............D$.
-COFF-i386:  0020 83c40cc3                             ....
-COFF-i386: Contents of section .data:
-COFF-i386:  0000 48656c6c 6f20576f 726c6421 00        Hello World!.
+# RUN: yaml2obj %s > %t-i386
+# RUN: llvm-objdump -s %t-i386 | FileCheck %s -check-prefix ELF-i386
----------------
MaskRay wrote:
> Delete the suffix or use `.coff-i386`?
I assume you meant `.elf-i386`. Done.


================
Comment at: test/Object/objdump-sectionheaders.test:14
+# CHECK:   4 .rela.text    00000000 0000000000000038
+# CHECK:   5 .symtab       00000018 0000000000000000
+# CHECK:   6 .strtab       00000001 0000000000000000
----------------
jhenderson wrote:
> Can you find any reasoning in history as to why the .symtab etc had a VMA before?
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.


================
Comment at: test/Object/readobj.test:2
-// Don't crash while reading non-dynamic files.
-RUN: llvm-readobj %p/Inputs/trivial-object-test.elf-x86-64
----------------
MaskRay wrote:
> jhenderson wrote:
> > Perhaps this needs to be a "does nothing if no operations requested" test, if it doesn't exist already?
> `test/tools/llvm-readobj/elf-dynamic-*` test various .dynamic features. This file can be moved there if you feel strong about keeping it.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64206/new/

https://reviews.llvm.org/D64206