[PATCH] D47145: [X86][ELF][CET] Adding the .note.gnu.property ELF section in X86

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I think this should be splitted into 2 patches: one for codegen and one for llvm-readobj tool.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3476
+    if (DataSize == sizeof(typename ELFT::uint) ||
+        DataSize == sizeof(Elf_Word)) {
+      uint64_t CFProtection =
----------------
That looks odd. `ELFT::uint` can be 4 or 8 and `Elf_Word` is 4, so It is the same as

`if (DataSize == 4 || DataSize == 8)`

I would go with that then.

It is also probably better to return early:

```
if (DataSize != 4 && DataSize != 8) {
 OS << format("<corrupt length: 0x%x>\n", DataSize);
 break;
}
...
```


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3480
+              ? (uint64_t)(*(const typename ELFT::Addr *)Data.data())
+              : (uint64_t)(*reinterpret_cast<const Elf_Word *>(Data.data()));
+      bool First = true;
----------------
And here then you can use something like following then
`support::endian::read32<ELFT::TargetEndianness>(Data.data())` and `support::endian::read64<ELFT::TargetEndianness>(Data.data())`


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3481
+              : (uint64_t)(*reinterpret_cast<const Elf_Word *>(Data.data()));
+      bool First = true;
+      if (CFProtection == 0) {
----------------
You can move it below the `if (CFProtection == 0)` since it is only used later.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3490
+        else
+          OS << ", ";
+        OS << "IBT";
----------------
I think you can avoid using `if (First)` here, because it is always true here, no?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3498
+        else
+          OS << ", ";
+        OS << "SHSTK";
----------------
Actually, it seems you can get rid of the `First` variable and just check the flags:

```
if (CFProtection & GNU_PROPERTY_X86_FEATURE_1_IBT) {
  CFProtection &= ~GNU_PROPERTY_X86_FEATURE_1_IBT;
  OS << "IBT";
  if (CFProtection)
    OS << ", ";
}

if (CFProtection & GNU_PROPERTY_X86_FEATURE_1_SHSTK) {
  CFProtection &= ~GNU_PROPERTY_X86_FEATURE_1_SHSTK;
  OS << "SHSTK";
  if (CFProtection)
    OS << ", ";
}

if (CFProtection)
  OS << format("<unknown flags: 0x%x>", CFProtection);
```


https://reviews.llvm.org/D47145