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

[George Rimar via Phabricator via llvm-commits]

grimar 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: ]
----------------
jhenderson wrote:
> 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).
I've used "low" and "high" suffixes instead.


================
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: ]
----------------
jhenderson wrote:
> 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)?
> This should probably be in the regular section-flags.test.
OK.

> 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)?

OK, I've also added one more test (`.all.possible`) there too.


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

https://reviews.llvm.org/D71462