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

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen added a comment.

> Needs some thoughts to merge the above paragraphs with the existing one

Great points. I updated the commit message and tried to capture most of these, PTAL.



================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:332
+      continue;
+    while (!isRegAvailable(NextReg))
+      ++NextReg;
----------------
MaskRay wrote:
> As a minor optimization to skip one redundant `isRegAvailable` call: `while (!isRegAvailable(++NextReg));`
`Reg` != `NextReg` here, and using `while (!isRegAvailable(++NextReg))` would always skip the first alternative register by first incrementing `NextReg` before calling `isRegsAvailable`. What am I missing?


================
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;
----------------
MaskRay wrote:
> `int64_t` is the return type of `getImm`, not the original `KCFI_CHECK` operand type. Casting this to a `uint32_t` is perhaps clearer.
I don't think casting this to `uint32_t` make sense since we'd have to cast it back to a larger type immediately below (to prevent `+ 0x800` from overflowing). I changed the comment to indicate this is a 32-bit value though.


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


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:14427
   SDValue Callee = CLI.Callee;
+  bool IsCFICall = CLI.CB && CLI.CB->isIndirectCall() && CLI.CFIType;
   bool &IsTailCall = CLI.IsTailCall;
----------------
MaskRay wrote:
> If `CLI.CFIType != 0`, is `CLI.CB->isIndirectCall()` possible? If not, add an assert?
> This is  an opportunity to save a redundant check.
Sure. `CFIType` should be set only for indirect calls when we reach this point.


================
Comment at: llvm/test/CodeGen/RISCV/kcfi-patchable-function-prefix.ll:4
+
+; NOC:            .p2align 2
+; C:              .p2align 1
----------------
MaskRay wrote:
> 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...
There are four functions because we're testing type hash locations in relation to patchable-function-prefix nops in four different cases, in addition to the code generated for KCFI checks. I'm not sure there's a way to do this with fewer functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148385