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

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1828
+  if (CodeGenOpts.BranchTargetEnforcement) {
+    FuncAttrs.addAttribute("branch-target-enforcement", "true");
+  }
----------------
danielkiss wrote:
> chill wrote:
> > 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.
> > 
> the attribute will be  "absent" for the runtime emitted function.
How about setting the attribute for LLVM created functions at the time of creation, just like Clang created functions
get their attribute at the time of creation?



================
Comment at: clang/lib/CodeGen/CGCall.cpp:1831
+
+  auto RASignKind = CodeGenOpts.getSignReturnAddress();
+  if (RASignKind != CodeGenOptions::SignReturnAddressScope::None) {
----------------
danielkiss wrote:
> chill wrote:
> > 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.
> > 
> in the new patch the per function processing will change the attribute only if really need.
Sure, but that's duplication of code/logic, it's a source of countless issues "oh, here's the place I should fix that thing ... oh noes,  turns out I have to fix ten more ... hope I've found all ..." ;)



================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:200
+        if (!F.hasFnAttribute("branch-target-enforcement"))
+          return false;
+        Attribute A = F.getFnAttribute("branch-target-enforcement");
----------------
chill wrote:
> chill wrote:
> > 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.
> > 
> Oh, I see, those are the cases of sanitizer functions, created at LLVM level, that don't have the attribute.
> Please, leave a comment in that sense.
Or, as mentioned in the other comment, check if it's possible to set the attribute at the time of creation (from module attributes?).  Tri-state logic is added complexity, if it's necessary, it's necessary, but if it's not, better make it simpler.


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

https://reviews.llvm.org/D75181