Page MenuHomePhabricator

[PowerPC] Finished kill_canary implementation and debugging
AbandonedPublic

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

Details

Reviewers
shchenz
rzurob
nemanjai
Group Reviewers
Restricted Project
Summary

Made a new phabricator review because of git issues

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.CodeGen/PowerPC::kill-canary-intrinsic.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix --ppc-asm-full-reg-names < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll -check-prefix=AIX

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 github.ibm.com:compiler/llvm-project 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.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
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.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10699

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

10705

Do we use this?

10711

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

10723–10735

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?

llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll
3

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 : (

llvm/include/llvm/IR/IntrinsicsPowerPC.td
198

Need rebase your code. GCCBuiltin is not a valid class anymore. See https://reviews.llvm.org/D127460

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5013 ↗(On Diff #441374)

Nit: avoid this unnecessary change

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
58

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

10715

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. https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

canaryLoc does not follow the style, should be CanaryLoc.

10725

All these new added codes need clang-format

llvm/test/CodeGen/PowerPC/kill-canary-intrinsic-aix.ll
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