[PATCH] D119296: KCFI sanitizer

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.

I see you modified the mir parser & printer; consider adding a .mir test.

Still LGTM. Might be nice to document the generated asm more for other compiler vendors to better understand the implementation, rather than having to read the tests added here.



================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4604
+  void EmitKCFIOperandBundle(const CGCallee &Callee,
+                             SmallVectorImpl<llvm::OperandBundleDef> &Bundles);
+
----------------
Should this parameter be an `ArrayRef`? This might be a more precise type, but seeing an impl type kind of breaks the whole pImpl idiom; I'm pretty sure that's what `ArrayRef` is for.
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-arrayref-h
That said, this header is pretty inconsistent in the use of `SmallVectorImp` vs `ArrayRef`, but I *think* `ArrayRef` is strongly preferred.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:1770-1771
+  if (uint32_t CFIType = getCFIType()) {
+    if (!FirstOp) {
+      FirstOp = false;
+      OS << ',';
----------------
If `FirstOp` is `false`, reassign it `false`? Is that right?  I see that's what's done on L1763, but..."what?"


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7821-7830
 
+  ConstantInt *CFIType = nullptr;
+  auto Bundle = CB.getOperandBundle(LLVMContext::OB_kcfi);
+  if (Bundle && CB.isIndirectCall()) {
+    if (!TLI.supportKCFIBundles())
+      report_fatal_error(
+          "Target doesn't support calls with kcfi operand bundles.");
----------------
Can we check whether the `CallBase` is an indirect call first before bothering with getting the operand bundle?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7822-7840
+  ConstantInt *CFIType = nullptr;
+  auto Bundle = CB.getOperandBundle(LLVMContext::OB_kcfi);
+  if (Bundle && CB.isIndirectCall()) {
+    if (!TLI.supportKCFIBundles())
+      report_fatal_error(
+          "Target doesn't support calls with kcfi operand bundles.");
+    CFIType = cast<ConstantInt>(Bundle->Inputs[0]);
----------------
Are we able to reorder these?

```
CLI.setDebugLoc ...
  ...
if (Bundle && CB.isIndirectCall) {
  ...
  CLI.setCFIType ...
```


================
Comment at: llvm/lib/Target/AArch64/AArch64KCFI.cpp:106
+
+  const AArch64Subtarget &SubTarget = MF.getSubtarget<AArch64Subtarget>();
+  TII = SubTarget.getInstrInfo();
----------------
`const auto &SubTarget = ...`


================
Comment at: llvm/lib/Target/X86/X86KCFI.cpp:110
+
+  const X86Subtarget &SubTarget = MF.getSubtarget<X86Subtarget>();
+  TII = SubTarget.getInstrInfo();
----------------
`const auto &SubTarget = ...`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296