If stack protection is enabled, the @llvm.ppc.kill.canary() intrinsic will load the stack guard canary word, corrupt it, and store it back. This feature exists for debugging/testing stack protection.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| llvm/include/llvm/Debuginfod/Debuginfod.h | ||
|---|---|---|
| 34 ↗ | (On Diff #442678) | Im really unsure how this change got in here, this is not added by me and on a freshly created branch and pull Any ideas? I'll look into this tomorrow |
Comment Actions
Seems current revision is not right. You select the wrong base commit when you create the diff?
| llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll | ||
|---|---|---|
| 5 | missing LE run test line? | |
| clang/test/CodeGen/PowerPC/builtins-ppc-stackprotect.c | ||
|---|---|---|
| 12 | nit: do we need -O2? | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
| 10702 | 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? | |
| 10718 | nit: we may don't need this magic number here, we can use DAG.getAllOnesConstant() when we generate the XOR below? | |
| llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll | ||
| 6 | +1, we should add a RUN line for 64bit Linux LE for -mtriple=powerpc64le-unknown-linux-gnu and for 32-bit AIX too. | |
| 22 | nit: we may can add nounwind attribute to the function to remove the unnecessary cfi related instructions in the test. | |
| 35 | 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. | |
| 60 | Linux seems right... | |
| llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll | ||
|---|---|---|
| 12 | How many test cases should I include? I tested all these machine triples on Power 7-10, bought I thought having 16 run lines may be too much, so I only included the power 7 and 8 run lines in the review. | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 10702 | 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. | |
| 10720–10749 | 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) | |
| llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll | ||
| 3 | Please add one RUN line with -O0 to ensure that this works as expected with Fast ISel. | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 10744 | Nit: Maybe we need to set the chain input for store as the chain output of the load. Op->getOperand(0) -> Load->getValue(1) There is data dependency between the load and the store, so above change will not impact the correctness. But that makes more sense from memory access order part. | |
| llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll | ||
| 191 | We may need to check why we are not storing not r3, r3 directly to 120(r1) at -O0, like std r3, 120(r1). Seems now we are storing to 119(r1) | |