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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/Inputs/trivial.ll:4
 ; llc -mtriple=x86_64-pc-win32 trivial.ll -filetype=obj -o trivial.obj.coff-x86-64
 ; llc -mtriple=i386-linux-gnu trivial.ll -filetype=obj -o trivial.obj.elf-i386 -relocation-model=pic
 ; llc -mtriple=i386-apple-darwin10 trivial.ll -filetype=obj -o trivial.obj.macho-i386 -relocation-model=pic
----------------
jhenderson wrote:
> Is the EM_386 ELF still used anywhere?
Yes, in a few tests.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test:12
+# I386-NEXT:  Class:                             ELF32
+# I386-NEXT:  Data:                              2's complement, little endian
+# I386-NEXT:  Version:                           1 (current)
----------------
jhenderson wrote:
> Maybe a separate change, but we should have a big-endian ELF tested in here too.
I am trying just to get rid of precompiled object for now. Such improvements should be done in a new separate test.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test:14-15
+# I386-NEXT:  Version:                           1 (current)
+# I386-NEXT:  OS/ABI:                            UNIX - System V
+# I386-NEXT:  ABI Version:                       0x0
+# I386-NEXT:  Type:                              REL (Relocatable file)
----------------
jhenderson wrote:
> 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).
I agree that these fields should be tested and that we should do this separatelly in a more specific tests.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test:22
+# I386-NEXT:  Start of section headers:          112 (bytes into file)
+# I386-NEXT:  Flags:                             0x0
+# I386-NEXT:  Size of this header:               52 (bytes)
----------------
jhenderson wrote:
> Non-zero flags?
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)


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test:87
+
+## Case 3: Test EM_MIPS object.
+# RUN: yaml2obj %s --docnum=3 -o %t3
----------------
jhenderson wrote:
> Any particular reason you felt the need to test EM_MIPS?
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.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/hex-dump.test:4
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --file-header --hex-dump=.strtab %t > %t.hexdump.out
+# RUN: llvm-readelf -h --hex-dump .strtab %t > %t.hexdump.1
----------------
jhenderson wrote:
> Don't we need a check somewhere that shows that dumping .strtab produces expected output?
I've changed `.strtab` to `.shstrtab`, which is dumped below.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/hex-dump.test:26-27
+# RUN:   | FileCheck %s --check-prefix=ELF
+# RUN: llvm-readobj -x 2 -x .shstrtab -x 3 -x not_exist %t 2>&1 \
+# RUN:   | FileCheck %s -DFILE=%t --check-prefixes=ELF-WARN,ELF
 
----------------
jhenderson wrote:
> These two RUN lines need an associated comment to say what they're testing.
It had the original comment saying "Test we dump the section only once.". I've expanded it.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/thin-archive-paths.test:1
 # RUN: rm -rf %t
 # RUN: mkdir -p %t/a/b
----------------
jhenderson wrote:
> (Aside: I'm not sure this test should be in the ELF directory. It is testing generic functionality)
I'll move it out.


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

https://reviews.llvm.org/D71662