[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:33-34
+# MIPS-LLVM:      Flags [ (0xFF000000)
+# MIPS-LLVM-NEXT:   SHF_EXCLUDE (0x80000000)
+# MIPS-LLVM-NEXT:   SHF_MASKPROC (0xF0000000)
+# MIPS-LLVM-NEXT:   SHF_MIPS_ADDR (0x40000000)
----------------
Where have these two flags come from?

SHF_MASKPROC in particular is concerning, since it's a range delimiter, not an actual value... That's probably a bug.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:54-55
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_MIPS_NODUPES, SHF_MIPS_NAMES, SHF_MIPS_LOCAL, SHF_MIPS_NOSTRIP,
+              SHF_MIPS_GPREL, SHF_MIPS_MERGE, SHF_MIPS_ADDR, SHF_MIPS_STRING]
+
----------------
Spacing here isn't quite consistent at the start and end of the list.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:1
+## Check how we dump arch specific ELF section flags.
+
----------------
grimar wrote:
> 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?
> 
Sounds good. I wasn't expecting you to extend this test right now!


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-flags.test:21
+# LLVM-NEXT:   SHF_TLS (0x400)
+# LLVM-NEXT:   SHF_WRITE (0x1)
+# LLVM-NEXT: ]
----------------
grimar wrote:
> 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?
> 
Yup, sounds good.


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

https://reviews.llvm.org/D71333