[PATCH] D75181: [AArch64] Handle BTI/PAC in case of generated functions.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

That said, my comments are not of the "over my dead body" kind ;)



================
Comment at: clang/lib/CodeGen/CGCall.cpp:1828
+  if (CodeGenOpts.BranchTargetEnforcement) {
+    FuncAttrs.addAttribute("branch-target-enforcement", "true");
+  }
----------------
I would really prefer to not set values "true" or "false" for the attribute: we don't really have tri-state logic there (absent/present-true/present-false), and those values just add some not-very useful string processing.



================
Comment at: clang/lib/CodeGen/CGCall.cpp:1831
+
+  auto RASignKind = CodeGenOpts.getSignReturnAddress();
+  if (RASignKind != CodeGenOptions::SignReturnAddressScope::None) {
----------------
What do we get from setting the PACBTI state in the  default function attributes? We still have 
to do a per function processing, we can just as well avoid repeating the logic, and spare us some
adding and potentially removing attributes churn.



================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:200
+        if (!F.hasFnAttribute("branch-target-enforcement"))
+          return false;
+        Attribute A = F.getFnAttribute("branch-target-enforcement");
----------------
This should be "true", although the comment might turn out moot.

If we somehow end up with a function, that does not have that attribute, we should clear the
ELF flag.



================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:201-202
+          return false;
+        Attribute A = F.getFnAttribute("branch-target-enforcement");
+        return !A.isStringAttribute() || A.getValueAsString() == "false";
       })) {
----------------
... that kind of string processing, here and elsewhere.


================
Comment at: llvm/lib/Target/AArch64/AArch64BranchTargets.cpp:62-66
+  Attribute A = F.getFnAttribute("branch-target-enforcement");
+  if (A.isStringAttribute() && A.getValueAsString() == "false")
+    return false;
+
+  if (!F.hasFnAttribute("branch-target-enforcement") &&
----------------
Isn't there some redundancy with the two calls to `getFnAttribute` and to `hasFnAttribute` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75181