[PATCH] D125916: [PowerPC] Defined and lowered llvm.ppc.kill.canary intrinsic

[ChenZheng via Phabricator via cfe-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11136
+    MachineFrameInfo &MFI = MF.getFrameInfo();
+    int SPI = MFI.getStackProtectorIndex(); // should return  -1
+    
----------------
Why should return -1?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11144
+
+    unsigned int deadBird = 0x4C6C566D; // replaces canary word
+
----------------
nit: the first letter should be upper for `deadBird` according to LLVM coding style.

And how can we make sure `0x4C6C566D` is not the same with the canary word load with `TargetOpcode::LOAD_STACK_GUARD`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11146
+
+    SDValue Store = DAG.getStore( // create store instr, stores (deadBird + 0) into addr (frame index + stack protector)
+            Op->getOperand(0), 
----------------
Need to run clang-format for the new codes.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11160
+    return Store;
+    break;
+  }
----------------
nit: no need for the break after a return


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:3
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s | \
+; RUN:   FileCheck %s
+
----------------
We may also need to add test cases for AIX 32/64 bit too as stack protector is also supported on AIX.


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:13
+; CHECK-NEXT:    stw 3, -17(1)
+; CHECK-NEXT:    blr
+entry:
----------------
This seems wrong. When there is no stack protector related instructions in the function, we should not generate the killed store to the stack slot for canary word.

The store instruction now will store a word to the caller unexpectedly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125916