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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I don't see any testing for the 'x' flag printing. I think it makes sense to add it here, since the behaviour could be impacted by this patch.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:100-101
+
+## Use an arbitraty EM_* machine type that does not have specific SHF_* OS/Processor
+## flags to test what we dump when have this bits set.
+
----------------
This comment probably belongs with the YAML below.

I'd change the last part of the sentence as follows:

"when have this bits set." -> "when bits in the OS and processor specific ranges are set."


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:105-106
+## 0x80000000. When SHF_EXCLUDE is mixed with other proccessor specific
+## flags, GNU readelf does not print "E", it drops this bits, because
+## handles SHF_MASKPROC mask first. It only prints "E" when there are
+## no other proccessor flags set.
----------------
I'd change as follows:
"GNU readelf does not print "E", it drops this bits, because handles SHF_MASKPROC mask first" -> "GNU readelf does not necessarily print "E", because it handles the SHF_MASKPROC mask first"



================
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: ]
----------------
What is this test case trying to show?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:132
+# OS-PROC-LLVM-NEXT: Type: SHT_PROGBITS
+# OS-PROC-LLVM-NEXT: Flags [ (0xE0000000)
+# OS-PROC-LLVM-NEXT:   SHF_EXCLUDE (0x80000000)
----------------
Same as above. What is this case trying to show?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:151
+# OS-PROC-LLVM-NEXT: Type: SHT_PROGBITS
+# OS-PROC-LLVM-NEXT: Flags [ (0xEFE00000)
+# OS-PROC-LLVM-NEXT:   SHF_EXCLUDE (0x80000000)
----------------
Same as above. What is this case actually trying to show?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test:172
+# OS-PROC-GNU-NEXT:  [ 9] .both.flags.maxvalues PROGBITS 0000000000000000 000040 000000 00  op 0   0  0
+# OS-PROC-GNU-NEXT:  [10] .exclude              PROGBITS 0000000000000000 000040 000000 00   E 0   0  0
+
----------------
What should GNU output do for a MIPS object with SHF_EXCLUDE set, since that value is also a used processor-specific one?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1497
+    if (I != std::end(ElfSectionFlags)) {
+      Flags &= ~I->Value;
+      Str += I->AltName;
----------------
It's entirely possible I'm misreading the code (I hate trying to follow bit-wise arithmetic), but isn't this line just a duplicate of the `Flags &= ~Flag;` above?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1502
+
+    // If we did not find a matching regular flag, then we deal with a OS
+    // specific flag, proccessor specific flag or an unknown flag.
----------------
a OS -> an OS


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1509
+      HasProcFlag = true;
+      Flags &= ~ELF::SHF_MASKPROC;
+    } else {
----------------
It took me quite a long time to identify how this algorithm achieves the stated aim in the comment, and I think it's because this block doesn't make clear in comments that the SHF_EXCLUDE flag is masked off by the masking below. Perhaps a comment here would be a good idea to clarify. Something along the lines of "Mask off all the processor-specific bits. This removes the SHF_EXCLUDE bit if set so that it doesn't also get printed."


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

https://reviews.llvm.org/D71462