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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: test/Object/X86/objdump-disassembly-inline-relocations.test:106
+    Address: 0x0000000000000034
+    Link:    .symtab
+    Info:    .text
----------------
This line can be deleted.


================
Comment at: test/Object/X86/objdump-disassembly-inline-relocations.test:150
+    Address: 0x0000000000000038
+    Link:    .symtab
+    Info:    .text
----------------
Ditto.


================
Comment at: test/Object/archive-symtab.test:70
+# THIN: Archive map
+# THIN-NEXT: main in {{.*}}.elf-x86-64
+# THIN-NEXT: foo in {{.*}}2.elf-x86-64
----------------
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.


================
Comment at: test/Object/archive-symtab.test:118
+
+## Repeate the test with llvm-ranlib
+
----------------
Could you fix this comment line please (repeate -> repeat, and trailing full stop).


================
Comment at: test/Object/nm-trivial-object.test:65
+# ELF-o-NOT: U
+# ELF-o: {{.*.elf-i386}}:          U SomeOtherFunction
+# ELF-o: {{.*.elf-i386}}: 00000000 T main
----------------
Consider using FileCheck -D here too.


================
Comment at: test/Object/nm-trivial-object.test:69
+
+# RUN: llvm-nm %t.elf-i386 -S | FileCheck %s -check-prefix ELF-SIZE
+
----------------
It's probably worth putting this test case back before the above one, to match its original ordering.


================
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
----------------
I assume you're holding off changing this to YAML for now?


================
Comment at: test/Object/nm-trivial-object.test:152
+
+# ELF64:                  U SomeOtherFunction
+# ELF64: 0000000000000000 T main
----------------
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.


================
Comment at: test/Object/objdump-file-header.test:5
+# COFF-i386: architecture: i386
+# COFF-i386: start address: 0x00000000
 
----------------
Here and the ELF one, you should probably write {{$}} at the end, to show that there are no more 0s than expected.


================
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
----------------
Any particular reason you've changed the check lines in this test?


================
Comment at: test/Object/objdump-section-content.test:32
+
+# COFF-i386: file format
+# COFF-i386: Contents of section .text:
----------------
Probably worth moving these check lines up to near their use point.


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


================
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
----------------
Perhaps this needs to be a "does nothing if no operations requested" test, if it doesn't exist already?


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

https://reviews.llvm.org/D64206