[PATCH] D128652: [PowerPC] Finished kill_canary implementation and debugging

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

"Made a new phabricator review because of git issues" is not an appropriate description of a review/revision.

Hopefully the description you add will describe what this intrinsic is supposed to do. It seems to me that this is a poorly designed feature if it is meant to work the way it was implemented. Namely, it seems like this intrinsic clobbers the stack protect global value rather than clobbering the corresponding value on the stack for the specific function it is enclosed in. I would have thought that it will clobber the stack in the function, thereby allowing stack protection to work as expected for other functions in the module.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5014
+
+    if (IntrinsicID == Intrinsic::ppc_kill_canary) {
+      CurDAG->SelectNodeTo(N, PPC::NOP, MVT::Other, N->getOperand(0));
----------------
I think it would be preferable to handle this intrinsic in one place. The `nop` is not actually necessary here. We should simply remove the intrinsic from the stream in `PPCISelLowering.cpp` and not pass it on.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11132
+  case Intrinsic::ppc_kill_canary: { 
+    MachineFunction &MF = DAG.getMachineFunction();
+    if (MF.getFunction().hasFnAttribute(Attribute::SafeStack) ||
----------------
The formatting of this entire block is quite messed up. Please run `clang-format` on this.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11138
+
+    IRBuilder<> B(&MF.getFunction().getEntryBlock().front());
+
----------------
Do we use this?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11144
+
+    if (GV == nullptr) break;
+    EVT VT = DAG.getTargetLoweringInfo().getValueType(DAG.getDataLayout(), GV->getType(), true); 
----------------
Is it ok to just ignore a failure to get the GV here? Should this not be an assert?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11156-11168
+    SDValue Store = DAG.getStore( 
+        Op->getOperand(0), 
+        DL,
+	DAG.getNode(
+            ISD::XOR,
+	    DL,
+	    VT,
----------------
What is happening here? We load the value, XOR it with itself, store it again? Isn't that just zeroing it out? Why do we even need to load it then?


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
+; RUN:   --ppc-asm-full-reg-names < %s | FileCheck %s
----------------
At the very least, this has to also include a RUN line for Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128652