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

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-stackprotect.c:11
+// RUN:  FileCheck %s
+// RUN: %clang_cc1 -O2 -triple powerpc64-unknown-aix \
+// RUN:   -emit-llvm %s -o - -target-cpu pwr7 | \
----------------
nit: do we need -O2?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10702
+
+    // If SafeStack or !StackProtector, kill_canary is not supported.
+    if (MF.getFunction().hasFnAttribute(Attribute::SafeStack) ||
----------------
Is it possible to handle this when we generate `Intrinsic::ppc_kill_canary`, for example in the FE? Passing and handling this useless intrinsic in all the opt passes should be avoided?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10718
+    // before corruption, so we XOR the canary word with all 1 bits.
+    const uint64_t XORWord = 0xFFFFFFFFFFFFFFFF;
+
----------------
nit: we may don't need this magic number here, we can use `DAG.getAllOnesConstant()` when we generate the `XOR` below?


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:5
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux \
+; RUN:   --ppc-asm-full-reg-names < %s | FileCheck %s -check-prefix=LINUX
+
----------------
+1, we should add a RUN line for  64bit Linux LE for `-mtriple=powerpc64le-unknown-linux-gnu`

and for 32-bit AIX too.


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:21
+
+attributes #0 = { sspreq }
+; Function Attrs: sspreq
----------------
nit: we may can add `nounwind` attribute to the function to remove the unnecessary `cfi` related instructions in the test.


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:34
+; AIX-NEXT:    not r4, r4
+; AIX-NEXT:    std r4, 0(r3)
+; AIX-NEXT:    ld r3, 0(r3)
----------------
The functionality here seems wrong, AFAIK, we should overwrite the content in the slack slot instead of the data csect. Which means the store here should be `std r4, 120(r1)` instead of `std r4, 0(r3)`. Stack protection aims to check that the stack content is not changed unexpectedly.


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:59
+; LINUX-NEXT:    not r3, r3
+; LINUX-NEXT:    std r3, 120(r1)
+; LINUX-NEXT:    ld r3, 120(r1)
----------------
Linux seems right...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129016