Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[PowerPC] Finished kill_canary implementation and debugging

Authored by pscoro on Jun 27 2022, 8:05 AM.


Group Reviewers
Restricted Project

Made a new phabricator review because of git issues

Diff Detail

Event Timeline

pscoro created this revision.Jun 27 2022, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 8:05 AM
pscoro requested review of this revision.Jun 27 2022, 8:05 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 27 2022, 8:05 AM
pscoro retitled this revision from [PowerPC] Finished kill_canary implementation and debugging Merge branch 'Paul_3105final' of into Paul_3105final to [PowerPC] Finished kill_canary implementation and debugging.Jun 27 2022, 8:07 AM
pscoro edited the summary of this revision. (Show Details)
pscoro added reviewers: Restricted Project, shchenz, rzurob.

"Made a new phabricator review because of git issues" is not an appropriate description of a review/revision.

Hopefully the description you add will describe what this intrinsic is supposed to do. It seems to me that this is a poorly designed feature if it is meant to work the way it was implemented. Namely, it seems like this intrinsic clobbers the stack protect global value rather than clobbering the corresponding value on the stack for the specific function it is enclosed in. I would have thought that it will clobber the stack in the function, thereby allowing stack protection to work as expected for other functions in the module.

5014 ↗(On Diff #440234)

I think it would be preferable to handle this intrinsic in one place. The nop is not actually necessary here. We should simply remove the intrinsic from the stream in PPCISelLowering.cpp and not pass it on.


The formatting of this entire block is quite messed up. Please run clang-format on this.


Do we use this?


Is it ok to just ignore a failure to get the GV here? Should this not be an assert?


What is happening here? We load the value, XOR it with itself, store it again? Isn't that just zeroing it out? Why do we even need to load it then?


At the very least, this has to also include a RUN line for Linux.

nemanjai requested changes to this revision.Jun 29 2022, 1:58 AM
This revision now requires changes to proceed.Jun 29 2022, 1:58 AM
pscoro updated this revision to Diff 441374.Jun 30 2022, 6:09 AM

fixed XOR usage, added linux tests and other small fixes

lkail added a subscriber: lkail.Jun 30 2022, 6:24 AM

Summary should be updated as @nemanjai has said.

Please make sure the patch is able to compile first. I fixed two build failures, it still fails : (


Need rebase your code. GCCBuiltin is not a valid class anymore. See

5013 ↗(On Diff #441374)

Nit: avoid this unnecessary change


Do we need this header? A little strange...


Please make sure when you post the patch, it is ok to compile. DL seems undefined, we use dl here.

Also please read the coding style of llvm first.

canaryLoc does not follow the style, should be CanaryLoc.


All these new added codes need clang-format

5 ↗(On Diff #441374)

We can merge these two files kill-canary-intrinsic-aix.ll and kill-canary-intrinsic-linux.ll into one file with different runlines but with different check prefix.

pscoro abandoned this revision.Jul 1 2022, 2:42 PM
pscoro updated this revision to Diff 442682.Jul 6 2022, 1:49 PM

patch fix

pscoro abandoned this revision.Jul 6 2022, 1:49 PM