[PATCH] D135411: Add generic KCFI operand bundle lowering

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:47
+  // Find call instructions with KCFI operand bundles.
+  SmallVector<CallInst *, 8> KCFICalls;
+  for (Instruction &I : instructions(F)) {
----------------
You can omit `, 8` to use the default (also 8).


================
Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:61
+  if (F.hasFnAttribute("patchable-function-prefix"))
+    report_fatal_error("-fpatchable-function-entry=N,M, where M>0 is not "
+                       "compatible with -fsanitize=kcfi on this target.",
----------------
Try avoiding report_fatal_error. Use a proper error handling mechanism like `Ctx.diagnose(...)` with a `DiagnosticInfo` subclass. Remove the trailing period for the message.


================
Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:72
+    // Get the expected hash value.
+    uint32_t ExpectedHash =
+        cast<ConstantInt>(CI->getOperandBundle(LLVMContext::OB_kcfi)->Inputs[0])
----------------
Add const if not modified


================
Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:97
+    Builder.CreateCall(Intrinsic::getDeclaration(&M, Intrinsic::trap));
+    NumKCFIChecks++;
+  }
----------------
pre-increment https://llvm.org/docs/CodingStandards.html#prefer-preincrement


================
Comment at: llvm/test/Transforms/KCFI/kcfi-supported.ll:4
+
+; If the back-end supports KCFI operand bundle lowering, KCFIPass should be a no-op.
+
----------------



================
Comment at: llvm/test/Transforms/KCFI/kcfi.ll:3
+
+; CHECK-LABEL: define void @f1
+define void @f1(ptr noundef %x) {
----------------
Appending `(` to prevent match with another function with the same prefix (not in this test, but this is a good general practice)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135411