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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 19 05:37:23 PST 2019


jhenderson added inline comments.


================
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)
----------------
grimar wrote:
> 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.
> 
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.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-file-headers.test:45
+    Flags: [ PF_R ]
+    Sections: []
+
----------------
Can't you just delete the line entirely?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/hex-dump.test:25
 
-# ELF-SEC: [ 9] .strtab
+## Both 2 and .shstrtab refer to .shstrtab.
+## Test we dump the section only once when the option is specified multiple times for the same section.
----------------
The "Both 2..." original comment is probably a bit redundant now that you have the sanity check comment above.


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

https://reviews.llvm.org/D71662





More information about the llvm-commits mailing list