Page MenuHomePhabricator

[AArch64][GlobalISel] Implement __builtin_return_address for PAC-RET
ClosedPublic

Authored by chill on Jul 24 2020, 2:42 AM.

Details

Summary

This patch implements stripping of the PAC in the return address for GlobalISel.

Implementation for when not using GLobalISel is in https://reviews.llvm.org/D75044
The analogous GCC patch is https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=a70d5d81c41048556fd86eaa1036018a6bfba115

Diff Detail

Event Timeline

chill created this revision.Jul 24 2020, 2:42 AM
arsenm added a subscriber: arsenm.Jul 24 2020, 6:33 AM

Merge conflict warning with D84199

sammccall resigned from this revision.Jul 24 2020, 12:58 PM

(Sorry, I don't know anything about this code)

aemerson added a reviewer: ab.Jul 24 2020, 1:07 PM
ab accepted this revision.Jul 27 2020, 10:44 AM

This looks sensible, modulo a couple inline comments

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4891–4892

I don't know what model you have for the attribute and the command line options, and I guess you're probably aware, but this seems unsafe for depths > 0. I'm not sure there's a better way to deal with this, short of just forcing the strips in any aarch64 code, which is not an option. Maybe emitting strips when +pa is available? I imagine that's not at all reliable either.

4892

-> hasPA() ?

llvm/test/CodeGen/AArch64/GlobalISel/builtin-return-address-pacret.ll
2

can you explicitly pass -global-isel ?

This revision is now accepted and ready to land.Jul 27 2020, 10:44 AM
chill updated this revision to Diff 281612.Jul 29 2020, 8:35 AM
chill marked 3 inline comments as done.Jul 29 2020, 8:38 AM
chill added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4891–4892

... I guess you're probably aware, but this seems unsafe for depths > 0.

In what sense is it unsafe? The clear instructions do not require a valid PAC
to be present, so even if a caller is not using PAC-RET we still get the return
address.

4892

I'll make a note to prepare a follow-up patch to use hasPA across the board.
There are a few more pending patches that likely use hasV8_3aOps, I want to fix them in a single patch.

llvm/test/CodeGen/AArch64/GlobalISel/builtin-return-address-pacret.ll
2

Kept -O0 so scheduling does not introduced uninteresting changes in the output.

ab added inline comments.Jul 30 2020, 11:21 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4891–4892

I'm thinking of the opposite scenario, where a caller in an unrelated library does have a signed return address, but this function wasn't compiled with that support: it wouldn't know to strip.

4892

If you have more patches please CC me, we have very extensive codegen support downstream, I can help with reviews until we upstream our changes ;) In the meantime you can take a look at our partial upstreaming in github apple/llvm-project

llvm/test/CodeGen/AArch64/GlobalISel/builtin-return-address-pacret.ll
2

Makes sense, that's fine. Can you also add -global-isel? ;) I'd rather not tie something as general as the optimization level with the specific path being tested: that can change over time, across cherry-picks, in different release branches, etc..

chill marked 6 inline comments as done.Jul 31 2020, 3:23 AM
chill added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4891–4892

I see. Looks like checking for +pa, while not perfect, will work in more cases.

llvm/test/CodeGen/AArch64/GlobalISel/builtin-return-address-pacret.ll
2

Right, I meant I added -global-isel, but also kept the -O0.

This comment was removed by danielkiss.
danielkiss added inline comments.Jul 31 2020, 12:30 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4891–4892

XPACLRI can always be emitted because it is in the NOP space. Besides it is not nice to emit always one extra presumably unnecessary instruction but I don't see why it won't work.

chill marked 2 inline comments as done.Aug 5 2020, 8:40 AM

I'm in favour of changing these patches to unconditionally emit XPACLRI (i.e. HINT #whatever).

XPACI could be emit when it is available, only the if (MF.getFunction().hasFnAttribute("sign-return-address")) { need do be dropped from the patches IMHO.

chill updated this revision to Diff 284471.Aug 10 2020, 12:19 PM

Updated the patch to unconditionally include PAC stripping instructions, NOP-space ones to non-NON-space ones, depending on
whether v8.3-a (soo to be '+pa') feature.

chill requested review of this revision.Aug 10 2020, 12:21 PM
arsenm added inline comments.Aug 10 2020, 12:23 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4894

Why do you need this temporary copy?

chill planned changes to this revision.Aug 11 2020, 3:02 AM
chill marked an inline comment as done.
chill added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4894

Nevermind, the patch is completely broken, I'll work on a new version.

chill updated this revision to Diff 284664.Aug 11 2020, 4:58 AM
chill marked an inline comment as done.

Well, it wasn't completely broken, just a little bit :D

We have virtual register that keeps LR, I would like for that vreg to keep the stripped version of LR, but
couldn't figure out how to get the proper place to insert the clearing insn, so instead there's a one clearing insn
per __builtin_fucntion_address "invocation".

ab accepted this revision.Aug 20 2020, 12:33 AM
ab added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4891–4892

Yeah, it should work to always emit it. On darwin we know we don't need to, but I imagine it's the exception rather than the rule. I think we'll address that when merging this into our ptrauth changes: either emit XPACI (as a strip intrinsic) or nothing, based on the target architecture. Either way this LGTM.

This revision is now accepted and ready to land.Aug 20 2020, 12:33 AM

Did you commit this?

chill added a comment.Sep 2 2020, 1:06 AM

No, not yet.

chill marked 2 inline comments as done.Thu, Sep 24, 6:02 AM
chill updated this revision to Diff 294076.Thu, Sep 24, 8:39 AM

Rebased.