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

[George Rimar via Phabricator via llvm-commits]

grimar 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.
+
----------------
jhenderson wrote:
> 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.
> cover the remaining arch-specific flags defined in BinaryFormat/ELF.h.
This test case originally covered `EM_HEXAGON`, `EM_MIPS` and `EM_X86_64`. It did not cover `EM_ARM` and `EM_XCORE`.
I've added a case for `EM_ARM`. I had to skip `EM_XCORE` for now because yaml2obj does not seem to support this machine type yet.
(I'll add a case for `EM_XCORE` in a follow-up change after teaching the yaml2obj tool if you do not mind).

I've also added missing MIPS flags.

> 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.
I think it is not possible atm, because yaml2obj handles flags as a bit set and report when sees an unknown used bit:
`YAML:117:14: error: unknown bit value`
I.e. I can't just use `0x20000000` (which is a `SHF_ARM_PURECODE/SHF_MIPS_MERGE`) inside`Flags []`, for example.

Seems we might want to implement a `ShFlags` property, similar to other `Sh*` stuff we have.
What about I do this in a follow-up too?



================
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
----------------
jhenderson wrote:
> 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'?
I'll try to pay more attention to such things during test cases moving next time.


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


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


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-flags.test:21
+# LLVM-NEXT:   SHF_TLS (0x400)
+# LLVM-NEXT:   SHF_WRITE (0x1)
+# LLVM-NEXT: ]
----------------
jhenderson wrote:
> 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.
This also needs something like a `ShFlags` property to write a test. Follow-up?



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

https://reviews.llvm.org/D71333