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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
