[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via llvm-commits]

samitolvanen added a comment.

In D119296#3623059 <https://reviews.llvm.org/D119296#3623059>, @nickdesaulniers wrote:

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

I added a .mir test for parsing the cfi-type. The machine instructions generated for the various call types are covered in the .ll tests.

> 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.

The exact instruction sequence is also documented in the kernel patches in case other compiler vendors want to implement this, but sure, that makes sense. I assume comments in the various `AsmPrinter` functions would be sufficient?



================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4604
+  void EmitKCFIOperandBundle(const CGCallee &Callee,
+                             SmallVectorImpl<llvm::OperandBundleDef> &Bundles);
+
----------------
nickdesaulniers wrote:
> 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.
I don't believe I can use `ArrayRef` when I need to append to the list, it appears to be read-only.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:1770-1771
+  if (uint32_t CFIType = getCFIType()) {
+    if (!FirstOp) {
+      FirstOp = false;
+      OS << ',';
----------------
nickdesaulniers wrote:
> If `FirstOp` is `false`, reassign it `false`? Is that right?  I see that's what's done on L1763, but..."what?"
Ah yes, it looks like this has been copied a few times earlier already. I'll stop the cycle.


================
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]);
----------------
nickdesaulniers wrote:
> Are we able to reorder these?
> 
> ```
> CLI.setDebugLoc ...
>   ...
> if (Bundle && CB.isIndirectCall) {
>   ...
>   CLI.setCFIType ...
> ```
We can reorder these, but this does follow the same convention as the rest of the function. I assume `CallLoweringInfo` was specifically designed so that the arguments can all be initialized in one statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296