Page MenuHomePhabricator

[AArch64] Emit .cfi_negate_ra_state for PAC-auth instructions.
ClosedPublic

Authored by danielkiss on Oct 14 2021, 1:13 AM.

Details

Summary

autiasp, autibsp instructions are the counterpart of paciasp/pacibsp instructions
therefore let's emit .cfi_negate_ra_state for these too.
In case of Armv8.3 instruction set the retaa/retbb will do the return and authentication
in one step here we can't emit the . cfi_negate_ra_state because that would be point after
the ret* instruction.

Diff Detail

Event Timeline

danielkiss created this revision.Oct 14 2021, 1:13 AM
danielkiss requested review of this revision.Oct 14 2021, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2021, 1:13 AM

(Slightly OT:
As my draft D109253 and D109254 show, aarch64 CFI is currently quite bad in prologue and epilogue code. Do we know how to fix that? :)
I realized that we have another issue: the uwtable IR attribute isn't sufficient to encode the different degree of unwind tables: (as an size optimization omitting most callee-saved-registers), -funwind-tables, -fasynchronous-unwind-tables.
)

Does this handle -homogeneous-prolog-epilog or adaptForLdStOpt correctly? If yes, is it necessary to improve test coverage when they are used together?

ardb added a comment.Oct 14 2021, 10:39 AM

Note that v8.3 retaa performs the autiasp internally, but does not write back the resulting value to x30, so even if we could emit the CFI directive here, doing so would not be correct.

D111411 is also in the queue to improve the situation of the prologues.

Does this handle -homogeneous-prolog-epilog or adaptForLdStOpt correctly?

I don't know, need to check.

danielkiss added inline comments.Oct 19 2021, 11:18 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
7617

fix a typo

Does this handle -homogeneous-prolog-epilog or adaptForLdStOpt correctly? If yes, is it necessary to improve test coverage when they are used together?

Looks okay to me, based on a few tests that I modified to run with PAUTH.

If yes, is it necessary to improve test coverage when they are used together?

It is a bit messy to run all tests with PAUTH, I can push a patch with additional test.

nickdesaulniers accepted this revision.Oct 19 2021, 12:13 PM

Consider adding @ardb 's note to your comments. Thank you very much for the patch!

This revision is now accepted and ready to land.Oct 19 2021, 12:13 PM
MaskRay accepted this revision.Oct 19 2021, 3:46 PM
This revision was landed with ongoing or failed builds.Oct 20 2021, 2:04 AM
This revision was automatically updated to reflect the committed changes.

This causes unwind problems when landing pads of the exception handlers involved, because for those the .cfi_negate_ra_state is processed twice and the signature from the return address is not stripped but should be.
Let's reland after D114545.

ardb added a comment.Apr 21 2022, 10:44 AM

Can this change be taken into consideration again now that D114545 has been merged?

Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 10:44 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
MaskRay reopened this revision.Apr 21 2022, 10:52 AM
MaskRay added a reviewer: chill.

Reopen to reflect the status that this has not been relanded.

I agree that we should re-consider this.

This revision is now accepted and ready to land.Apr 21 2022, 10:53 AM

Can this change be taken into consideration again now that D114545 has been merged?

Yes, I'm running a few tests, will land later today.