[PATCH] D71333: [llvm-readobj][test] - Add a test for testing regular section flags and cleanup flags testing.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:1
+## Check how we dump arch specific ELF section flags.
+
----------------
Probably this should be deferred to a later change, but I think this test should be extended to
1) cover the remaining arch-specific flags defined in BinaryFormat/ELF.h.
2) show that arch-specific flags not known for the current file format (e.g. SHF_MIPS_MERGE for an X86_64 ELF) are handled appropriately.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:3
+
+# RUN: yaml2obj -docnum 1 %s > %t-hex.o
+# RUN: llvm-readobj -S %t-hex.o | FileCheck -check-prefix=HEX-LLVM %s
----------------
jhenderson wrote:
> Here and elsewhere in this test whilst you're moving it, could you change single dash long-options to double-dash ones please (--docnum, --check-prefix etc).
Maybe '>' -> '-o'?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:3-5
+# RUN: yaml2obj -docnum 1 %s > %t-hex.o
+# RUN: llvm-readobj -S %t-hex.o | FileCheck -check-prefix=HEX-LLVM %s
+# RUN: llvm-readelf -S %t-hex.o | FileCheck -check-prefix=HEX-GNU %s
----------------
Here and elsewhere in this test whilst you're moving it, could you change single dash long-options to double-dash ones please (--docnum, --check-prefix etc).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:20
+  Machine:  EM_HEXAGON
+  Flags:    []
+Sections:
----------------
I believe this line can be deleted?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:25
+    Flags:  [SHF_HEX_GPREL]
+    Size:   4
+
----------------
You could delete the `Size` parameter to slightly simplify ever test case here, I think.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:76
+    Size:   4
+...
----------------
Nit: unnecessary `...`


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-flags.test:3
+
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-readobj -S %t.o | FileCheck -check-prefix=LLVM %s
----------------
`-o`?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-flags.test:4-5
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-readobj -S %t.o | FileCheck -check-prefix=LLVM %s
+# RUN: llvm-readelf -S %t.o | FileCheck -check-prefix=GNU %s
+
----------------
--check-prefix


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-flags.test:21
+# LLVM-NEXT:   SHF_TLS (0x400)
+# LLVM-NEXT:   SHF_WRITE (0x1)
+# LLVM-NEXT: ]
----------------
Don't know if this should be in this or another test, but we should also test:
1. Unknown flags outside any reserved range.
2. Flags in the OS-specific range.


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

https://reviews.llvm.org/D71333