[PATCH] D84502: [AArch64][GlobalISel] Implement __builtin_return_address for PAC-RET

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:4720
       EntryBuilder.setInstr(*EntryBlock.begin());
-      EntryBuilder.buildCopy({DstReg}, {Register(AArch64::LR)});
+      if (MF.getFunction().hasFnAttribute("sign-return-address")) {
+        if (STI.hasV8_3aOps()) {
----------------
chill wrote:
> ab wrote:
> > 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.
> > ... 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.
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.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:4721
+      if (MF.getFunction().hasFnAttribute("sign-return-address")) {
+        if (STI.hasV8_3aOps()) {
+          Register TmpReg = MRI.createVirtualRegister(&AArch64::GPR64RegClass);
----------------
chill wrote:
> ab wrote:
> > -> `hasPA()` ?
> 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.
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


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/builtin-return-address-pacret.ll:1
+;; RUN: llc -mtriple aarch64               -O0 %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-NOP
+;; RUN: llc -mtriple aarch64 -mattr=+v8.3a -O0 %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-V83
----------------
chill wrote:
> ab wrote:
> > can you explicitly pass `-global-isel` ?
> Kept -O0 so scheduling does not introduced uninteresting changes in the output.
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..


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84502/new/

https://reviews.llvm.org/D84502