[PATCH] D59780: Support Intel Control-flow Enforcement Technology

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I'm happy with this approach and I think it could be extended to handle the AArch64 BTI/PAC features very simply. I think it will be worth adding some tests for the new PLT sequences at some point.

Although not relevant to this patch, I'm a bit concerned about how CET will propagate through LTO. For example is it a module attribute that the LTO code-generator must merge so that the output ELF file has a single correct property, or is it expressed in the bitcode file and the linker has to read that? I'm assuming the former.



================
Comment at: lld/ELF/Driver.cpp:1449
 }
 
+// In order to enable CET (x86's hardware-assited control flow enforcement),
----------------
In theory you could update Config->X86Features incrementally as you read them in rather than storing them in InputFiles and processing them later. Although I think doing all the logic here makes it easier to understand.


================
Comment at: lld/ELF/Driver.cpp:1457
+// This function returns a merged feature flags. If 0, we cannot enable CET.
+template <class ELFT> static uint32_t getX86Features() {
+  if (Config->EMachine != EM_386 && Config->EMachine != EM_X86_64)
----------------
I've made a few English suggestions below. No functional changes.
```
// To enable CET (x86's hardware-assited control flow enforcement),
// each source file must be compiled by passing --xxx to the compiler
// [what is the compiler flag to enable CET?]. Object files compiled with the
// flag contain feature flags indicating that they are compatible with CET. We
// enable the feature only when all object files are compatible with CET.
//
// This function returns the merged feature flags. If 0, we cannot enable CET.
```



================
Comment at: lld/ELF/SyntheticSections.cpp:297
+// If x86, each object file may contain feature flags indicating what features
+// that object file is compatible with. The flags are stored in a
+// .note.gnu.property section.
----------------
I think it would be better to say
"In x86, object files may contain feature flags indicating the features that they have used."
The x86-64-psABI has:
```
GNU_PROPERTY_X86_FEATURE_1_AND The x86 processor features indicated by
the corresponding bits are used in program.
```


================
Comment at: lld/ELF/SyntheticSections.cpp:2429
+// Note that the processors in the market as of early 2019 don't actually
+// support the feature. The only spec is available at the moment.
+//
----------------
Only the spec is available at the moment.


================
Comment at: lld/ELF/SyntheticSections.cpp:2436
+// byte long), as the PLT entry is only 16 byte long and all bytes are already
+// used.
+//
----------------
"4 bytes long" and "16 bytes long" 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59780