[PATCH] D49791: [AArch64] - Generate pointer authentication instructions

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

A few minor comments inline, and one question: it sounds like this is based on a GCC feature;  what's the expected behavior for `__builtin_returnaddress` ?  Shouldn't it `xpaci` the result?

Also, that's one of the reasons why I'm not sure it's wise to emit the instructions always, even if we're not explicitly targeting v8.3a.  You run the risk of having some parts of the code (__builtin_returnaddress, backtrace, unwind, ...) be built for v8, not knowing that they need to strip, even though some other part might have enabled this LR signing.  I'm not sure there's much to be done about that, though ;)



================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:296
+  if(Scope.equals("all"))
+    return true;
+
----------------
Is this actually necessary?  "all" is still only functions that actually save LR, no?  Why sign it if it's not saved?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1007
                     MachineInstr::FrameDestroy);
+    InsertReturnAddressAuth(MF, MBB);
     return;
----------------
One trick to make sure this covers all 'return' paths (even when the code changes) is to use 'make_scope_exit' to guarantee the code is run.


================
Comment at: test/CodeGen/AArch64/sign-return-address.ll:5
+define dso_local i32 @leaf(i32 %x) {
+entry:
+  ret i32 %x
----------------
To expand on Oliver's comments, you can also remove the basic block names, and 'dso_local' (unless its semantics matter, which doesn't seem to be the case here)

Also, another nit: having the check lines away from the check-label is kinda confusing, maybe put it all before the IR?


================
Comment at: test/CodeGen/AArch64/sign-return-address.ll:42
+}
+; CHECK: paciasp
+; CHECK: autiasp
----------------
The ordering of the instructions is critical here, no?  Whether the PAC/AUT occur before (resp. after) SP updates (or, for that matter, the LR save) is crucial to the feature.  Maybe have explicit CHECK-NEXT for the rest of the prologue/epilogue?


https://reviews.llvm.org/D49791