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

[mike.dvoretsky]

Summary

In preparation for the proposed linker ABI changes (https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf, https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-cet.pdf), this patch enables emission of the .note.gnu.property section to ELF object files when building CET-enabled modules.

[mike.dvoretsky]

Uploaded the version without the test for the section emission. Corrected.

[mike.dvoretsky]

Updated to use full 8/4 byte integers to store the flags rather than using 1 byte and padding.

[mike.dvoretsky]

Enabled readobj to parse sections that use 32 bits for the data on 64-bit targets (like gcc currently does) and added explicit alignment to the section generation.

[craig.topper]

llvm/tools/llvm-readobj/ELFDumper.cpp

3504 ↗

(On Diff #148223)

This format string is wrong for a uint64_t. If you set a bad bit in the upper 32-bits this justs prints <unknown flags: 0x0>

[grimar]

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

llvm/tools/llvm-readobj/ELFDumper.cpp

3476 ↗

(On Diff #148606)

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;
}
...

3480 ↗

(On Diff #148606)

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

3481 ↗

(On Diff #148606)

You can move it below the if (CFProtection == 0) since it is only used later.

3490 ↗

(On Diff #148606)

I think you can avoid using if (First) here, because it is always true here, no?

3498 ↗

(On Diff #148606)

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

[mike.dvoretsky]

Made changes to the readobj part per comments. Will wait on comment from @craig.topper before splitting the patch.

[craig.topper]

Go ahead and split the patch.

[mike.dvoretsky]

Removed the llvm-readobj part to release as a separate patch.

[jhenderson]

llvm/lib/Target/X86/X86AsmPrinter.cpp

566–569

Can't you use EmitBytes to achieve this?

[grimar]

llvm/lib/Target/X86/X86AsmPrinter.cpp

557

I would move this assert to the beginning of the scope
and also use isArch64Bit for consistency:

if (FeatureFlagsAnd) {
  assert((TT.isArch32Bit() || TT.isArch64Bit()) && ...

But actually.. is it really possible to arch be 16 bits in X86AsmPrinter?
It seems this assert is just redundant. And if that would be possible, it should be
an explicit error probably.

[mike.dvoretsky]

Replaced assertion with a more explicit error at the start of the code path. Changed the emission of the "GNU\0" byte sequence to use EmitBytes with the StringRef constructor specifically to include the trailing zero byte.

The ELF.h file is no longer affected by this patch since D47473 was commited.

[grimar]

llvm/lib/Target/X86/X86AsmPrinter.cpp

545

This could be uint8_t, though I think this better be just unsigned.

556

Do you need this cast?

558

This better be moved closer to the location of using

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

llvm/test/CodeGen/X86/note-cet-property.ll

1

Can you add a check for the 32-bit target too? (Since you implement both in patch).

[mike.dvoretsky]

Changes made per comments.

llvm/lib/Target/X86/X86AsmPrinter.cpp

556

Unfortunately, yes. The compilation fails without it.

[grimar]

llvm/lib/Target/X86/X86AsmPrinter.cpp

556

I think you just need to add
#include "llvm/MC/MCSectionELF.h"
to resolve the failure.

[grimar]

llvm/lib/Target/X86/X86AsmPrinter.cpp

547

This if looks not checked by the test case?

[mike.dvoretsky]

Added an extra flag to the test, so that both flag-reading paths are covered. Since flags are module-wide, if there need to be tests covering them separately they would have to be put in new files.

[grimar]

LGTM.

llvm/lib/Target/X86/X86AsmPrinter.cpp

 
 #include "X86AsmPrinter.h"
 #include "InstPrinter/X86ATTInstPrinter.h"
 #include "MCTargetDesc/X86BaseInfo.h"
 #include "MCTargetDesc/X86TargetStreamer.h"
 #include "X86InstrInfo.h"
 #include "X86MachineFunctionInfo.h"
 #include "llvm/BinaryFormat/COFF.h"
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/CodeGen/MachineConstantPool.h"
 #include "llvm/CodeGen/MachineModuleInfoImpls.h"
 #include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/MC/MCCodeEmitter.h"
bool X86AsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
   }
   printMemReference(*this, MI, OpNo, O);
   return false;
 }
 
 void X86AsmPrinter::EmitStartOfAsmFile(Module &M) {
   const Triple &TT = TM.getTargetTriple();
 
+  if (TT.isOSBinFormatELF()) {
+    // Assemble feature flags that may require creation of a note section.
+    char FeatureFlagsAnd = 0;
+    if (M.getModuleFlag("cf-protection-branch"))
+      FeatureFlagsAnd |= ELF::GNU_PROPERTY_X86_FEATURE_1_IBT;
+    if (M.getModuleFlag("cf-protection-return"))
+      FeatureFlagsAnd |= ELF::GNU_PROPERTY_X86_FEATURE_1_SHSTK;
+
+    if (FeatureFlagsAnd) {
+      // Emit a .note.gnu.property section with the flags.
+      if (!TT.isArch32Bit() && !TT.isArch64Bit())
+        llvm_unreachable("CFProtection used on invalid architecture!");
+      MCSection *Cur = OutStreamer->getCurrentSectionOnly();
+      MCSection *Nt = (MCSection *)MMI->getContext().getELFSection(
+          ".note.gnu.property", ELF::SHT_NOTE, ELF::SHF_ALLOC);
+      int WordSize = TT.isArch64Bit() ? 8 : 4;
+      OutStreamer->SwitchSection(Nt);
+
+      // Emitting note header.
+      EmitAlignment(WordSize == 4 ? 2 : 3);
+      OutStreamer->EmitIntValue(4, 4 /*size*/); // data size for "GNU\0"
+      OutStreamer->EmitIntValue(8 + WordSize, 4 /*size*/); // Elf_Prop size
+      OutStreamer->EmitIntValue(ELF::NT_GNU_PROPERTY_TYPE_0, 4 /*size*/);
+      OutStreamer->EmitBytes(StringRef("GNU", 4)); // note name
+
+      // Emitting an Elf_Prop for the CET properties.
+      OutStreamer->EmitIntValue(ELF::GNU_PROPERTY_X86_FEATURE_1_AND, 4);
+      OutStreamer->EmitIntValue(WordSize, 4);               // data size
+      OutStreamer->EmitIntValue(FeatureFlagsAnd, WordSize); // data
+      EmitAlignment(WordSize == 4 ? 2 : 3);                 // padding
+
+      OutStreamer->endSection(Nt);
+      OutStreamer->SwitchSection(Cur);
+    }
+  }
+
   if (TT.isOSBinFormatMachO())
     OutStreamer->SwitchSection(getObjFileLowering().getTextSection());
 
   if (TT.isOSBinFormatCOFF()) {
     // Emit an absolute @feat.00 symbol.  This appears to be some kind of
     // compiler features bitfield read by link.exe.
     if (TT.getArch() == Triple::x86) {
       MCSymbol *S = MMI->getContext().getOrCreateSymbol(StringRef("@feat.00"));

llvm/test/CodeGen/X86/note-cet-property.ll

+; RUN: llc -mtriple x86_64-pc-linux < %s | FileCheck %s
+
+; This test checks that the compiler emits a .note.gnu.property section for
+; modules with "cf-protection" module flags.
+
+; CHECK:      .section        .note.gnu.property,"a",@note
+; CHECK-NEXT: .p2align 3
+; CHECK-NEXT: .long    4
+; CHECK-NEXT: .long    16
+; CHECK-NEXT: .long    5
+; CHECK-NEXT: .asciz   "GNU"
+; CHECK-NEXT: .long    3221225474
+; CHECK-NEXT: .long    8
+; CHECK-NEXT: .quad    2
+; CHECK-NEXT: .p2align 3
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"cf-protection-return", i32 1}