[PATCH] D51429: [AArch64] Return Address Signing B Key Support

[Oliver Stannard via Phabricator via cfe-commits]

olista01 added inline comments.


================
Comment at: lib/CodeGen/CGDeclCXX.cpp:364
   if (RASignKind != CodeGenOptions::SignReturnAddressScope::None)
+  {
     Fn->addFnAttr("sign-return-address",
----------------
LLVM style has the opening brace on the same line as the if. There's a git-clang-format script in the clang repo which can fix up style issues like this for a whole patch, changing touching lines you haven't changes.


================
Comment at: lib/CodeGen/CGDeclCXX.cpp:374
+                      : "b_key");
+  }
+
----------------
I think we need to handle branch-target-enforcement here too.


================
Comment at: lib/CodeGen/TargetInfo.cpp:4982
+    if (Kind != CodeGenOptions::SignReturnAddressScope::None)
+    {
+      Fn->addFnAttr("sign-return-address",
----------------
Style nit: brace on same line as if.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1433
 
+static std::tuple<StringRef, StringRef, bool>
+ParseAArch64BranchProtection(const Driver &D, const ArgList &Args, const Arg *A) {
----------------
Please add a comment about what the parts of the return value are.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1440
+  StringRef Value = A->getValue();
+  // This maps onto -mbranch-protection=<scope>+<key>
+
----------------
I'm not sure what this is trying to say. Is it referring to the "standard" case below?


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1448
+  } else if (!Value.equals("none")) {
+    SmallVector<StringRef, 3> BranchProtection;
+    StringRef(A->getValue()).split(BranchProtection, '+');
----------------
I'd make this 4, because that's the longest sensible argument ("pac-ret+leaf+b-key+bti").


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1460
+        Scope = "non-leaf";
+        while (++Protection != BranchProtection.end()) {
+          if (Protection->equals("leaf"))
----------------
Add a comment about the fact that "leaf" and "b-key" must follow "pac-ret", to explain why we don't just parse this in one loop.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1543
+    if (A->getOption().matches(options::OPT_msign_return_address_EQ))
+    {
+      Scope = A->getValue();
----------------
Nit: brace on same line as if.


================
Comment at: test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp:7
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ALL  --check-prefix=CHECK-A-KEY
 
 struct Foo {
----------------
This should also test branch target enforcement (it's also missing from the code).


https://reviews.llvm.org/D51429