[PATCH] D129016: [PowerPC] implemented @llvm.ppc.kill.canary to corrupt stack guard

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10702
+
+    // If SafeStack or !StackProtector, kill_canary is not supported.
+    if (MF.getFunction().hasFnAttribute(Attribute::SafeStack) ||
----------------
Again, this comment is nothing more than a re-reading of the code. As such, it is not useful. The comment should say what is happening and why. Something along the lines of:
```
// The kill_canary intrinsic only makes sense when the Stack Protector
// feature is on in the function. It can also not be used in conjunction
// with safe stack because the latter splits the stack and the canary
// value isn't used (i.e. safe stack supersedes stack protector).
// In situations where the kill_canary intrinsic is not supported,
// we simply replace uses of its chain with its input chain, causing
// the SDAG CSE to remove the node.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10720-10749
+    if (useLoadStackGuardNode()) {
+      MachineSDNode *LSG =
+          DAG.getMachineNode(PPC::LOAD_STACK_GUARD, DL, VT, Op->getOperand(0));
+      Load = SDValue(LSG, 0);
+
+      // Frame index used to determine stack guard location if
+      // LOAD_STACK_GUARD is used.
----------------
The common code should just be common. Seems like the only thing that changes is how we load the value. Please refactor this to something like:
```
if (useLoadStackGuardNode()) {
  ...
  Load = ... // stack guard load
} else if (Value *GV = getSDagStackGuard(*M)) {
  ...
  Load = ... // canary word global load
} else
  llvm_unreachable("Unhandled stack guard case");

Store = ... // common store
return Store; // (or you can just create the store in-line and return it directly)
```


================
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=powerpc-unknown-aix -ppc-vsr-nums-as-vr \
+; RUN:   -mcpu=pwr7 --ppc-asm-full-reg-names < %s | FileCheck %s -check-prefix=CHECK-AIX
----------------
Please add one RUN line with `-O0` to ensure that this works as expected with Fast ISel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129016