[PATCH] D71462: [llvm-readelf][llvm-readobj] - Reimplement the logic of section flags dumping.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:120
+# OS-PROC-LLVM-NEXT: Type: SHT_PROGBITS
+# OS-PROC-LLVM-NEXT: Flags [ (0xFE00000)
+# OS-PROC-LLVM-NEXT: ]
----------------
grimar wrote:
> jhenderson wrote:
> > What is this test case trying to show?
> I wanted to show how we handle the value that is (`SHF_MASKOS` - 1).
> I supposed it is reasonable, because we might have some logic (and we actually have it)
> that works with `SHF_MASKOS` in the code. Hence I tried to check the bounds: a minimal possible value,
> the mask itself and something that is almost large as `SHF_MASKOS`, but `!= SHF_MASKOS`.
> 
> Do you think it is excessive?
I think it's okay to have one not on the boundaries, but I'd change the naming of this and the similar issues elsewhere, since "max" implies it is trying to be the maximum OS-specific flag value, which is not the case (that would be 0xff00000).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:56
 
-# RUN: yaml2obj --docnum 3 %s -o %t-x86_64.o
+## Test what we print when an MIPS object when it has SHF_MIPS_STRING flag.
+## SHF_MIPS_STRING has a value of 0x80000000, which matches the value of SHF_EXCLUDE.
----------------
an MIPS -> a MIPS
has the SHF_MIPS_STRING


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:132
+## the SHF_MASKPROC mask first. It only prints "E" when there are no other
+## proccessor flags set. Check llvm-readelf output matches GNU.
+
----------------
proccessor -> processor


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:183-186
+# OS-PROC-LLVM:      Name: .unknown
+# OS-PROC-LLVM-NEXT: Type: SHT_PROGBITS
+# OS-PROC-LLVM-NEXT: Flags [ (0xF0000)
+# OS-PROC-LLVM-NEXT: ]
----------------
This should probably be in the regular section-flags.test.

I'm not actually sure if any of this test case belongs in this test, as it's not about the architecture really. How about moving it into a new test (e.g. section-flags-os-proc.test)?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:203
+
+## Use an arbitraty EM_* machine type that does not have specific SHF_* OS/Processor
+## flags to test what we dump when bits in the OS and processor specific ranges are set.
----------------
arbitraty -> arbitrary


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

https://reviews.llvm.org/D71462