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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:545
+    // Assemble feature flags that may require creation of a note section.
+    char FeatureFlagsAnd = 0;
+    if (M.getModuleFlag("cf-protection-branch"))
----------------
This could be `uint8_t`, though I think this better be just `unsigned`.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:556
+      MCSection *Cur = OutStreamer->getCurrentSectionOnly();
+      MCSection *Nt = (MCSection *)MMI->getContext().getELFSection(
+          ".note.gnu.property", ELF::SHT_NOTE, ELF::SHF_ALLOC);
----------------
Do you need this cast?


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:558
+          ".note.gnu.property", ELF::SHT_NOTE, ELF::SHF_ALLOC);
+      int WordSize = TT.isArch64Bit() ? 8 : 4;
+      OutStreamer->SwitchSection(Nt);
----------------
This better be moved closer to the location of using

```
// Emitting note header.
int WordSize = TT.isArch64Bit() ? 8 : 4;
EmitAlignment(WordSize == 4 ? 2 : 3);
```


================
Comment at: llvm/test/CodeGen/X86/note-cet-property.ll:1
+; RUN: llc -mtriple x86_64-pc-linux < %s | FileCheck %s
+
----------------
Can you add a check for the 32-bit target too? (Since you implement both in patch).


https://reviews.llvm.org/D47145