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
Comment Actions
Sorry about making a separate review for this again, I encountered some troubles with git and decided that moving my code to a fresh branch would be quicker than figuring out how to fix the problem, the previous review will be abandoned.
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 11150 | To address a comment on the previous review, I fixed the XOR to do what I intended. When you XOR bits against 1, you are guaranteed to not return the same bit because of exclusivity. Therefore XORing the canary word against 0xFFFFFFFF (0b1111...111) guarantees that the corrupted canary word is never the same as the original | |
| 11157 | Addressing a comment from the previous review, GV != nullptr can not be an assert because linux implements stack guard loading differently than aix. This review now also supports linux as well | |
Comment Actions
Did you consider to make this more generalized? From skimming through the change, I see only 2 ppc-specific things:
- Loading of the canary word
- Value of XORWord
The first bullet is solved by the the other inline comment I made.
The second bullet can be solved by using all 64 bits. If a 32 bit value is used, the upper bits are ignored because the VT should be MVT::i32 in this case.
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 11154–11156 | I think this can be simplified using getSDagStackGuard(). | |
| 11157 | You can simplify the code by reordering the conditions: if (useLoadStackGuardNode()) {
// LOAD_STACK_GUARD
} else if (Value *GV = getSDagStackGuard(M)) {
// AIX implementation
} else
llvm_unreachable("Unhandled stack guard case"); | |
| 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 | ||
| 11126 | 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? | |
| 11142 | 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 | ||
|---|---|---|
| 11126 | 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. | |
| 11144–11173 | 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 | ||
|---|---|---|
| 11168 | 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) | |
nit: do we need -O2?