[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Thanks for the description. The code generally looks good.

If the kernel wants a specific code sequence, implementing conditions in the LLVMCodeGen looks good to me. Consider adding a link to the discussion.

I wonder whether it will be more useful to give more context to those who are unaware of -fsanitize=kcfi:

This patch implements KCFI operand bundle lowering to generate contiguous code sequences and produce `.kcfi_traps` sections.

- clang emits "kcfi" operand bundles
- `SelectionDAGBuilder::visitCall` visits the indirect `CallInst` and builds a `SDNode` using `XXXTargetLowering::LowerCall`.
- In `RISCVPassConfig::addPreSched`, add a KCFI pass to emit a `KCFI_CHECK` target-specific pseudo instruction before an indirect call instruction.
- In `RISCVAsmPrinter`, lower the `KCFI_CHECK` pseudo instructions to a contiguous code sequence that crash if the type hashes don't match.

Without the patch, there is no `.kcfi_traps` and the type hash comparison code sequence may not be contiguous due to basic block placement.



================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:327
+  auto isRegAvailable = [&](unsigned Reg) {
+    return (Reg != AddrReg && !STI->isRegisterReservedByUser(Reg));
+  };
----------------
The prevailing style doesn't add parens for `return`.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:332
+      continue;
+    while (!isRegAvailable(NextReg))
+      ++NextReg;
----------------
As a minor optimization to skip one redundant `isRegAvailable` call: `while (!isRegAvailable(++NextReg));`


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:338
+  }
+  assert(ScratchRegs[0] != AddrReg && ScratchRegs[1] != AddrReg &&
+         "Invalid scratch registers for KCFI_CHECK");
----------------
This assert seems redundant as `Reg != AddrReg` is very close.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:352
+    int NopSize =
+        STI->getInstrInfo()->getNop().getOpcode() == RISCV::C_NOP ? 2 : 4;
+    int64_t PrefixNops = 0;
----------------
Just check `STI.hasStdExtCOrZca()`


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:368
+  // Load the expected type hash.
+  const int64_t Type = MI.getOperand(1).getImm();
+  const int64_t Hi20 = ((Type + 0x800) >> 12) & 0xFFFFF;
----------------
`int64_t` is the return type of `getImm`, not the original `KCFI_CHECK` operand type. Casting this to a `uint32_t` is perhaps clearer.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:371
+  const int64_t Lo12 = SignExtend64<12>(Type);
+  if (Hi20)
+    EmitToStreamer(
----------------
Add `{}` if the then body has more than one lines.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:375
+        MCInstBuilder(RISCV::LUI).addReg(ScratchRegs[1]).addImm(Hi20));
+  if (Lo12 || Hi20 == 0)
+    EmitToStreamer(*OutStreamer,
----------------
Is `Hi20 == 0` tested?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:16122
+
+  switch (MBBI->getOpcode()) {
+  case RISCV::PseudoCALLIndirect:
----------------
This switch should probably be turned to an assert. You can use `llvm::is_contained` if you don't want to write `MBBI->getOpcode()` twice.

`assert(is_contained({RISCV::PseudoCALLIndirect, RISCV::PseudoTAILIndirect}, MBBI->getOpcode()));`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:16131
+  MachineOperand &Target = MBBI->getOperand(0);
+  assert(Target.isReg() && "Invalid target operand for an indirect call");
+  Target.setIsRenamable(false);
----------------
This assert seems redundant since we immediately call `getReg` with an internal assert.


================
Comment at: llvm/test/CodeGen/RISCV/kcfi-patchable-function-prefix.ll:4
+
+; NOC:            .p2align 2
+; C:              .p2align 1
----------------
f1/f2/f3/f4 have the same shape. Without increasing the number of functions, we can make some disturbance (e.g. add one extra indirect call, use inline asm to clobber some registers) to check various cases without using .mir tests. `.mir` tests are fine, but the signal-to-noise ratio is not great and readers can be lost in the paramemters...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148385