Page MenuHomePhabricator

[AArch64][PAC] Select XPAC for ptrauth.strip intrinsic.
ClosedPublic

Authored by ab on Aug 22 2022, 8:56 AM.

Details

Summary

This is mostly straightforward, with an additional optimization: XPACIuntied allows strips of LR (which we emit in __builtin_return_address) to avoid clobbering LR. (This can be useful, e.g., for a certain kind of wrapper functions that just forward the caller address, for logging and such)

Diff Detail

Event Timeline

ab created this revision.Aug 22 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald Transcript
ab requested review of this revision.Aug 22 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 8:56 AM
tschuett added inline comments.Aug 22 2022, 12:09 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
5873

Can the Key be 2? Is an if-statement easier to comprehend than a ternary operator?

apazos added inline comments.Aug 23 2022, 11:24 AM
llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
1227 ↗(On Diff #454524)

there is no unit test case for this scenario

kristof.beyls added inline comments.Aug 24 2022, 8:53 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
5873

I guess having an enum for the valid key descriptors for AArch64 would make this and other code working with keys more readable. I'm not sure if that's typically done or where to define that enum?
Than in this location, maybe a switch on that enum would be clearer?

I see these constants 0,1,2,3 are also needed in tablegen files. Not sure if something similar to an enum exists in Tablegen?

ab updated this revision to Diff 456771.Aug 30 2022, 1:18 PM
ab marked 3 inline comments as done.
  • Extract out XPACIuntied, leaving it for ptrauth-returns.
  • Pull in key/opcode helper functions from later patch in the series.
ab added inline comments.Aug 30 2022, 1:21 PM
llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
1227 ↗(On Diff #454524)

Yeah, XPACIuntied is really intended to be used in @llvm.returnaddress when return address signing (/PAC-RET) is enabled. I pulled it out of this patch into that one, where it actually gets exercised.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
5873

A later patch in the series actually does introduce helper functions in AArch64BaseInfo.h/AArch64InstrInfo.h, so it's a simple matter of using them here! I combined the two in this review, to be committed separately.

kristof.beyls accepted this revision.Aug 31 2022, 2:36 AM

LGTM!
I just added a few minor nitpicks - I'll leave it to you to decide if following those suggestions would result in an improvement or not.

llvm/lib/Target/AArch64/AArch64InstrInfo.h
514 ↗(On Diff #456771)

nitpick, maybe do s/ptrauth auth/pointer authentication/?

526 ↗(On Diff #456771)

nitpick, maybe do s/ptrauth auth/pointer signing/?

llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
798 ↗(On Diff #456771)

I guess that if we added "LAST = 3" here, the somewhat cryptic if statement in AArch64InstructionSelector.cpp that says

if (Key > 3)
      return false;

could instead say something like

if (AArch64PACKey::LAST > 3)
      return false;

It seems the word "LAST" is already used in a few other places in LLVM to indicate the highest defined value in an enum. Maybe doing this change would be a minor improvement in readability/maintainability?

This revision is now accepted and ready to land.Aug 31 2022, 2:36 AM
This revision was landed with ongoing or failed builds.Oct 24 2022, 8:16 AM
This revision was automatically updated to reflect the committed changes.