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

Oliver Stannard via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 25 08:28:31 PDT 2018


olista01 added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:285
+  // - sign-return-address=all
+  // - sign-return-address=non-leaf and the functions spills the LR
+
----------------
This comment and the gcc/clang command line options use "non-leaf", but you check for "partial" in the function attribute. I think it would be better to use "non-leaf" everywhere, unless there's a good reason to switch to "partial".


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:292
+  StringRef Scope = F.getFnAttribute("sign-return-address").getValueAsString();
+  if(Scope.equals_lower("none"))
+    return false;
----------------
I can't find any other places where we do case-insensitive checks of function attributes, so I think we should stick to doing an exact string-compare here.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:879
+
+  if (Subtarget.hasV8_3aOps() && MBBI != MBB.end() &&
+      MBBI->getOpcode() == AArch64::RET_ReallyLR) {
----------------
I think this could do with a comment explaining the different pre-v8.3 and post-v8.3 code.


================
Comment at: test/CodeGen/AArch64/sign-return-address.ll:6
+entry:
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
----------------
Most of the code in these tests can be removed (e.g. this one could be "ret i32 %x", or just "ret void").


================
Comment at: test/CodeGen/AArch64/sign-return-address.ll:68
+  %0 = load i32, i32* %x.addr, align 4
+  %call = call i32 @leaf_sign_non_leaf(i32 %0)
+  %1 = load i32, i32* %x.addr, align 4
----------------
It would be clearer to add an external function to call, rather than having the tests call each other.


================
Comment at: test/CodeGen/AArch64/sign-return-address.ll:112
+
+attributes #0 = { "sign-return-address"="all" }
+
----------------
Putting these attributes directly on the function definitions (in place of the #<n> reference) would make the tests a bit easier to read.


https://reviews.llvm.org/D49791





More information about the llvm-commits mailing list