This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add support for ROP protection for 32 bit.
ClosedPublic

Authored by stefanp on Oct 7 2021, 3:16 PM.

Details

Summary

Add support for Return Oriented Programming (ROP) protection for 32 bit.
This patch also adds a testing for AIX on both 64 and 32 bit.

Diff Detail

Event Timeline

stefanp created this revision.Oct 7 2021, 3:16 PM
stefanp requested review of this revision.Oct 7 2021, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2021, 3:16 PM
stefanp updated this revision to Diff 378031.Oct 7 2021, 3:19 PM

Updated spacing.

stefanp added a reviewer: Restricted Project.Oct 7 2021, 3:20 PM
amyk added a subscriber: amyk.Nov 1 2021, 2:49 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.td
5548

Just wanted to double check that isCodeGenOnly is intended for these, right?
I don't have a strong opinion either way but since they're not on the 64-bit version and since hasSideEffects is documented, it may not hurt to document why isCodeGenOnly is added here.

llvm/test/CodeGen/PowerPC/ppc64-rop-protection-aix.ll
278

Would it be helpful to add a TODO to remove this comment when the next ABI revision is released?

stefanp added inline comments.Nov 9 2021, 12:22 PM
llvm/lib/Target/PowerPC/PPCInstrInfo.td
5548

Sure. I can add that.
The idea is that we don't want the llvm to get confused between HASHCHK and HASHCHK8 because they are both encoded the same way. I'll add a comment with an explanation.

llvm/test/CodeGen/PowerPC/ppc64-rop-protection-aix.ll
278

Sure. That's a good idea. I will add that.

nemanjai added inline comments.Nov 12 2021, 5:36 AM
llvm/lib/Target/PowerPC/PPCInstrInfo.td
5548

I think Amy's question is why this one parts with the convention to make the 32-bit versions the real instructions and the 64-bit version the isCodeGenOnly instruction (along with being marked with Interpretation64Bit). And I think the question is perfectly reasonable.

stefanp marked an inline comment as not done.Nov 15 2021, 7:28 AM
stefanp added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.td
5548

Oh I see what you mean. I'm sorry I think I misunderstood initially. There is no reason why I did it this way. I will switch the flags so that the isCodeGenOnly flag is on the 64 bit version.

stefanp updated this revision to Diff 387382.Nov 15 2021, 1:20 PM

Added a TODO to the test file.
Moved the isCodeGenOnly to the 64 bit version.
Added the Interpretation64Bit flag to the 64 bit version.

amyk accepted this revision as: amyk.Nov 23 2021, 5:59 AM

Thanks for addressing the comments I had Stefan.
Overall this LGTM.

This revision is now accepted and ready to land.Nov 23 2021, 5:59 AM
stefanp updated this revision to Diff 397385.Jan 4 2022, 1:11 PM

Rebased the patch to get ready for commit.

stefanp updated this revision to Diff 397610.Jan 5 2022, 9:05 AM

Rebased this patch again as it was not rebased correctly the last time.
Should be ready for commit.

This revision was landed with ongoing or failed builds.Jan 5 2022, 1:16 PM
This revision was automatically updated to reflect the committed changes.