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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss abandoned this revision.
danielkiss added a comment.

Abandoning in favour of D85649 .


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

https://reviews.llvm.org/D75181

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D75181#2193447 , @danielkiss wrote:

> I don't see any other alternative option, I'm open to any other idea.

My original idea was to pass options to LLVM. I'll come up with a patch in a 
day or two (if it works) and then we'll see.


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

https://reviews.llvm.org/D75181

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

> FWIW GCC has a "sign-return-address" function attribute with a default value 
> of "none". It is considered deprecated, however, in favour of 
> "branch-protection"

This is just the internal representation, the function attribute in C/C++ 
source is the "branch-protection".

Old version of the patch used attribute value to represent the 
"ignore"/disabled state https://reviews.llvm.org/D75181?id=247201 so I have no 
idea what would be the right solution.


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

https://reviews.llvm.org/D75181

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kyrill Tkachov via Phabricator via cfe-commits]

ktkachov added a comment.

In D75181#2193447 , @danielkiss wrote:

> Would it be better to add a new value to `"sign-return-address"` as `"none"`? 
> I don't see any other alternative option, I'm open to any other idea.

FWIW GCC has a "sign-return-address" function attribute with a default value of 
"none". It is considered deprecated, however, in favour of "branch-protection"


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

https://reviews.llvm.org/D75181

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

Would it be better to add a new value to `"sign-return-address"` as `"none"`? I 
don't see any other alternative option, I'm open to any other idea.


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

https://reviews.llvm.org/D75181

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

This approach looks way too hackish to me with multiple opposing attributes 
("sign-return-address" vs. "ignore-sign-return-address")
and some convoluted logic to resolve the contradiction.


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

https://reviews.llvm.org/D75181

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

depends on D80791 


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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss updated this revision to Diff 268383.
danielkiss edited the summary of this revision.
danielkiss added a comment.

Patch is refactored, the new pass smooth out the attribute issues.
This version fixes issues with _clang_call_terminate, sanitisers and the CFI. 
New pass might need a new file, I'm not yet convinced myself.


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

https://reviews.llvm.org/D75181

Files:
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGen/aarch64-sign-return-address.c
  clang/test/CodeGenCXX/aarch64-branch-target_clang_call_terminate.cpp
  clang/test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/test/CodeGen/AArch64/O0-pipeline.ll
  llvm/test/CodeGen/AArch64/O3-pipeline.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll

Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
@@ -8,10 +8,11 @@
   ret i32 0
 }
 
-!llvm.module.flags = !{!0, !1}
+!llvm.module.flags = !{!0, !1, !2}
 
 !0 = !{i32 4, !"branch-target-enforcement", i32 1}
 !1 = !{i32 4, !"sign-return-address", !"non-leaf"}
+!2 = !{i32 4, !"sign-return-address-key", !"a_key"}
 
 ; Both attribute present
 ; ASM:	.word	3221225472
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
@@ -8,9 +8,10 @@
   ret i32 0
 }
 
-!llvm.module.flags = !{!0}
+!llvm.module.flags = !{!0, !1}
 
 !0 = !{i32 4, !"sign-return-address", !"all"}
+!1 = !{i32 4, !"sign-return-address-key", !"a_key"}
 
 ; PAC attribute present
 ; ASM:	.word	3221225472
Index: llvm/test/CodeGen/AArch64/O3-pipeline.ll
===
--- llvm/test/CodeGen/AArch64/O3-pipeline.ll
+++ llvm/test/CodeGen/AArch64/O3-pipeline.ll
@@ -23,6 +23,8 @@
 ; CHECK-NEXT: Dominator Tree Construction
 ; CHECK-NEXT: FunctionPass Manager
 ; CHECK-NEXT:   Simplify the CFG
+; CHECK-NEXT: AArch64 Branch Protection
+; CHECK-NEXT: FunctionPass Manager
 ; CHECK-NEXT:   Dominator Tree Construction
 ; CHECK-NEXT:   Natural Loop Information
 ; CHECK-NEXT:   Lazy Branch Probability Analysis
Index: llvm/test/CodeGen/AArch64/O0-pipeline.ll
===
--- llvm/test/CodeGen/AArch64/O0-pipeline.ll
+++ llvm/test/CodeGen/AArch64/O0-pipeline.ll
@@ -16,6 +16,8 @@
 ; CHECK-NEXT: Pre-ISel Intrinsic Lowering
 ; CHECK-NEXT: FunctionPass Manager
 ; CHECK-NEXT:   Expand Atomic instructions
+; CHECK-NEXT: AArch64 Branch Protection
+; CHECK-NEXT: FunctionPass Manager
 ; CHECK-NEXT:   Module Verifier
 ; CHECK-NEXT:   Lower Garbage Collection Instructions
 ; CHECK-NEXT:   Shadow Stack GC Lowering
Index: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
===
--- llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
+++ llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
@@ -33,6 +33,7 @@
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/Function.h"
+#include "llvm/IR/Metadata.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
@@ -454,6 +455,9 @@
   if (TM->getOptLevel() != CodeGenOpt::None && EnableAtomicTidy)
 addPass(createCFGSimplificationPass(1, true, true, false, true));
 
+  // Sets up the branch protection attributes for functions that added so far.
+  addPass(createAArch64BranchProtectionPass());
+
   // Run LoopDataPrefetch
   //
   // Run this before LSR to remove the multiplies involved in computing the
Index: llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
===
--- llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
+++ llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
@@ -26,9 +26,57 @@
 using namespace llvm;
 
 #define DEBUG_TYPE "aarch64-branch-targets"
+#define AARCH64_BRANCH_PROTECTION_NAME "AArch64 Branch Protection"
 #define AARCH64_BRANCH_TARGETS_NAME "AArch64 Branch Targets"
 
 namespace {
+
+struct AArch64BranchProtection : public ModulePass {
+  static char ID;
+  AArch64BranchProtection() : ModulePass(ID) {
+initializeAArch64BranchProtectionPass(*PassRegistry::getPassRegistry());
+  }
+
+  bool runOnModule(Module ) override {
+bool changed = false;
+const 

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5149-5152
+  if (BPI.BranchTargetEnforcement)
+Fn->addFnAttr("branch-target-enforcement", "true");
+  else
+Fn->addFnAttr("branch-target-enforcement", "false");

tamas.petz wrote:
> chill wrote:
> > danielkiss wrote:
> > > I'm going to rebase the patch. I add there a new attribute here 
> > > "ignore-branch-target-enforcement"
> > > so then the "branch-target-enforcement"="true"/"false" could be just 
> > > "branch-target-enforcement".
> > > 
> > > 
> > TBH, that's worse, IMHO.
> > 
> > Ideally, I *think* we'd like *every* LLVM IR function that the backend sees,
> > regardless of how, why and by whom it is created, to have (or not have)
> > the three existing PACBTI attributes "sign-return-address", 
> > "sign-return-address-key", and "branch-target-enforcement", so the backend 
> > can generate code accordingly.
> > 
> > The module attributes are LLVM IR metadata,  and  AFAIK LLVM IR metadata is 
> > an optional extra, 
> > it should not affect correctness.
> > Indeed, *module* metadata is a somwhat grey area,  better not use it if 
> > there a way around it.
> > 
> > 
> > 
> Which case are you trying to handle here?
> Is this the case, for example, when -mbranch-protection=standard is set but a 
> function has _attribute _((target("branch-protection=none")))?
Chill: I think I have code that solve that for clang created functions, but 
functions created in llvm I don't have any idea. 

Tamas: yes, that is the case when module is compiled with bti but the function 
is not ( "none" or "pac-ret" and so on. )


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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5149-5152
+  if (BPI.BranchTargetEnforcement)
+Fn->addFnAttr("branch-target-enforcement", "true");
+  else
+Fn->addFnAttr("branch-target-enforcement", "false");

chill wrote:
> danielkiss wrote:
> > I'm going to rebase the patch. I add there a new attribute here 
> > "ignore-branch-target-enforcement"
> > so then the "branch-target-enforcement"="true"/"false" could be just 
> > "branch-target-enforcement".
> > 
> > 
> TBH, that's worse, IMHO.
> 
> Ideally, I *think* we'd like *every* LLVM IR function that the backend sees,
> regardless of how, why and by whom it is created, to have (or not have)
> the three existing PACBTI attributes "sign-return-address", 
> "sign-return-address-key", and "branch-target-enforcement", so the backend 
> can generate code accordingly.
> 
> The module attributes are LLVM IR metadata,  and  AFAIK LLVM IR metadata is 
> an optional extra, 
> it should not affect correctness.
> Indeed, *module* metadata is a somwhat grey area,  better not use it if there 
> a way around it.
> 
> 
> 
Which case are you trying to handle here?
Is this the case, for example, when -mbranch-protection=standard is set but a 
function has _attribute _((target("branch-protection=none")))?


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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5149-5152
+  if (BPI.BranchTargetEnforcement)
+Fn->addFnAttr("branch-target-enforcement", "true");
+  else
+Fn->addFnAttr("branch-target-enforcement", "false");

danielkiss wrote:
> I'm going to rebase the patch. I add there a new attribute here 
> "ignore-branch-target-enforcement"
> so then the "branch-target-enforcement"="true"/"false" could be just 
> "branch-target-enforcement".
> 
> 
TBH, that's worse, IMHO.

Ideally, I *think* we'd like *every* LLVM IR function that the backend sees,
regardless of how, why and by whom it is created, to have (or not have)
the three existing PACBTI attributes "sign-return-address", 
"sign-return-address-key", and "branch-target-enforcement", so the backend can 
generate code accordingly.

The module attributes are LLVM IR metadata,  and  AFAIK LLVM IR metadata is an 
optional extra, 
it should not affect correctness.
Indeed, *module* metadata is a somwhat grey area,  better not use it if there a 
way around it.





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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss updated this revision to Diff 255347.
danielkiss added a reviewer: tamas.petz.
danielkiss added a comment.

Fix review comments from Tamas.


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

https://reviews.llvm.org/D75181

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGenCXX/aarch64-branch-target_clang_call_terminate.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll

Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
@@ -0,0 +1,27 @@
+; RUN: llc -mtriple=aarch64-linux %s   -o - | \
+; RUN:   FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
+; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
+
+define dso_local i32 @f() #0 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 @g() {
+entry:
+  ret i32 0
+}
+
+attributes #0 = { "branch-target-enforcement" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
+; Only the common atttribute (BTI)
+; ASM:	.word	3221225472
+; ASM-NEXT:	.word	4
+; ASM-NEXT	.word	1
+
+; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
@@ -13,6 +13,10 @@
 
 attributes #0 = { "branch-target-enforcement" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
 ; Declarations don't prevent setting BTI
 ; ASM:	.word	3221225472
 ; ASM-NEXT:	.word	4
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
@@ -13,10 +13,14 @@
   ret i32 0
 }
 
-attributes #0 = { "sign-return-address"="non-leaf" }
+attributes #0 = { "ignore-branch-target-enforcement" "sign-return-address"="non-leaf" }
 
 attributes #1 = { "branch-target-enforcement" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
 ; No common attribute, no note section
 ; ASM: warning: not setting BTI in feature flags
 ; ASM-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
@@ -17,6 +17,11 @@
 
 attributes #1 = { "sign-return-address"="none" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"sign-return-address", !"non-leaf"}
+
+
 ; No common attribute, no note section
 ; ASM-NOT: .note.gnu.property
 ; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
@@ -15,7 +15,13 @@
 
 attributes #0 = { "branch-target-enforcement" "sign-return-address"="non-leaf" }
 
-attributes #1 = { "sign-return-address"="all" }
+attributes #1 = { "ignore-branch-target-enforcement" "sign-return-address"="all" }
+
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+!1 = !{i32 4, !"sign-return-address", !"all"}
+
 
 ; Only the common atttribute (PAC)
 ; ASM: warning: not setting BTI in feature flags
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
@@ -17,6 +17,10 @@
 
 attributes #1 = { "branch-target-enforcement" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
 ; Only the common atttribute (BTI)
 ; ASM:	.word	3221225472
 ; ASM-NEXT:	.word	4
Index: 

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

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1827-1828
+
+  auto RASignKind = getLangOpts().getSignReturnAddressScope();
+  if (RASignKind != LangOptions::SignReturnAddressScopeKind::None) {
+FuncAttrs.addAttribute("sign-return-address",

LangOptions has the following functions:

 - hasSignReturnAddress()
 - isSignReturnAddressWithAKey()
 - isSignReturnAddressScopeAll()

With these functions some of these lines can be significantly reduced.



Comment at: clang/lib/CodeGen/CGCall.cpp:1837
+"sign-return-address-key",
+RASignKey == LangOptions::SignReturnAddressKeyKind::AKey ? "a_key"
+ : "b_key");

Use getLangOpts(). isSignReturnAddressWithAKey()



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:596
+
+  LangOptions::SignReturnAddressScopeKind Scope =
+  Context.getLangOpts().getSignReturnAddressScope();

Use new functions please.


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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss updated this revision to Diff 254761.
danielkiss added a comment.

The patched is rebased and the hopefully the logic is now simpler.


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

https://reviews.llvm.org/D75181

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGenCXX/aarch64-branch-target_clang_call_terminate.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll

Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
@@ -0,0 +1,27 @@
+; RUN: llc -mtriple=aarch64-linux %s   -o - | \
+; RUN:   FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
+; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
+
+define dso_local i32 @f() #0 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 @g() {
+entry:
+  ret i32 0
+}
+
+attributes #0 = { "branch-target-enforcement" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
+; Only the common atttribute (BTI)
+; ASM:	.word	3221225472
+; ASM-NEXT:	.word	4
+; ASM-NEXT	.word	1
+
+; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
@@ -13,6 +13,10 @@
 
 attributes #0 = { "branch-target-enforcement" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
 ; Declarations don't prevent setting BTI
 ; ASM:	.word	3221225472
 ; ASM-NEXT:	.word	4
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
@@ -13,10 +13,14 @@
   ret i32 0
 }
 
-attributes #0 = { "sign-return-address"="non-leaf" }
+attributes #0 = { "ignore-branch-target-enforcement" "sign-return-address"="non-leaf" }
 
 attributes #1 = { "branch-target-enforcement" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
 ; No common attribute, no note section
 ; ASM: warning: not setting BTI in feature flags
 ; ASM-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
@@ -17,6 +17,11 @@
 
 attributes #1 = { "sign-return-address"="none" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"sign-return-address", !"non-leaf"}
+
+
 ; No common attribute, no note section
 ; ASM-NOT: .note.gnu.property
 ; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
@@ -15,7 +15,13 @@
 
 attributes #0 = { "branch-target-enforcement" "sign-return-address"="non-leaf" }
 
-attributes #1 = { "sign-return-address"="all" }
+attributes #1 = { "ignore-branch-target-enforcement" "sign-return-address"="all" }
+
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+!1 = !{i32 4, !"sign-return-address", !"all"}
+
 
 ; Only the common atttribute (PAC)
 ; ASM: warning: not setting BTI in feature flags
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
@@ -17,6 +17,10 @@
 
 attributes #1 = { "branch-target-enforcement" }
 
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
 ; Only the common atttribute (BTI)
 ; ASM:	.word	3221225472
 ; ASM-NEXT:	.word	4
Index: 

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss marked an inline comment as done.
danielkiss added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5149-5152
+  if (BPI.BranchTargetEnforcement)
+Fn->addFnAttr("branch-target-enforcement", "true");
+  else
+Fn->addFnAttr("branch-target-enforcement", "false");

I'm going to rebase the patch. I add there a new attribute here 
"ignore-branch-target-enforcement"
so then the "branch-target-enforcement"="true"/"false" could be just 
"branch-target-enforcement".




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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1828
+  if (CodeGenOpts.BranchTargetEnforcement) {
+FuncAttrs.addAttribute("branch-target-enforcement", "true");
+  }

chill wrote:
> 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?
> 
Attributes are not always set in clang as I see:
CodeGenModule::CreateRuntimeFunction() -> 
CodeGenModule::GetOrCreateLLVMFunction  
CreateRuntimeFunction could be fixed but, the common location for LLVM created 
function is the llvm::Function::Create() where the CodeGenOpts and LangOpts are 
no available.
Adding target specific code there seems not right for me.


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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Momchil Velikov via Phabricator via cfe-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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

ping


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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss updated this revision to Diff 250191.
danielkiss marked an inline comment as done.
danielkiss added a comment.

Patch is rebased, test is updated.


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

https://reviews.llvm.org/D75181

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGen/aarch64-sign-return-address.c
  clang/test/CodeGenCXX/aarch64-branch-target_clang_call_terminate.cpp
  clang/test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/lib/Target/AArch64/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/test/CodeGen/AArch64/branch-target-enforcement-indirect-calls.ll
  llvm/test/CodeGen/AArch64/branch-target-enforcement.mir
  llvm/test/CodeGen/AArch64/bti-branch-relaxation.ll
  llvm/test/CodeGen/AArch64/machine-outliner-bti.mir
  llvm/test/CodeGen/AArch64/machine-outliner-outline-bti.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
  llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll

Index: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
===
--- llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
+++ llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
@@ -1,6 +1,6 @@
 ; RUN: llc -mtriple=aarch64 %s -o - | FileCheck %s
 
-define void @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
+define void @f0() "patchable-function-entry"="0" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f0:
 ; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK:  // %bb.0:
@@ -12,7 +12,7 @@
 
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, place the label .Lpatch0 after the initial BTI.
-define void @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
+define void @f1() "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1:
 ; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK-NEXT: .cfi_startproc
@@ -28,7 +28,7 @@
 }
 
 ;; -fpatchable-function-entry=2,1 -mbranch-protection=bti
-define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement" {
+define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: .type f2_1,@function
 ; CHECK-NEXT: .Ltmp0:
 ; CHECK-NEXT:  nop
@@ -50,7 +50,7 @@
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, don't create .Lpatch0 if the initial instruction is not BTI,
 ;; even if other basic blocks may have BTI.
-define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement" {
+define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1i:
 ; CHECK-NEXT: .Lfunc_begin3:
 ; CHECK:  // %bb.0:
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
@@ -0,0 +1,27 @@
+; RUN: llc -mtriple=aarch64-linux %s   -o - | \
+; RUN:   FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
+; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
+
+define dso_local i32 @f() #0 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 @g() {
+entry:
+  ret i32 0
+}
+
+attributes #0 = { "branch-target-enforcement"="true" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
+; Only the common atttribute (BTI)
+; ASM:	.word	3221225472
+; ASM-NEXT:	.word	4
+; ASM-NEXT	.word	1
+
+; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
@@ -11,7 +11,11 @@
 
 declare dso_local i32 @g()
 
-attributes #0 = { "branch-target-enforcement" }
+attributes #0 = { "branch-target-enforcement"="true" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, 

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss updated this revision to Diff 248222.
danielkiss marked an inline comment as done.

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

https://reviews.llvm.org/D75181

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGen/aarch64-sign-return-address.c
  clang/test/CodeGenCXX/aarch64-branch-target_clang_call_terminate.cpp
  clang/test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/lib/Target/AArch64/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/test/CodeGen/AArch64/branch-target-enforcement-indirect-calls.ll
  llvm/test/CodeGen/AArch64/branch-target-enforcement.mir
  llvm/test/CodeGen/AArch64/bti-branch-relaxation.ll
  llvm/test/CodeGen/AArch64/machine-outliner-bti.mir
  llvm/test/CodeGen/AArch64/machine-outliner-outline-bti.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
  llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll

Index: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
===
--- llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
+++ llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
@@ -1,6 +1,6 @@
 ; RUN: llc -mtriple=aarch64 %s -o - | FileCheck %s
 
-define void @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
+define void @f0() "patchable-function-entry"="0" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f0:
 ; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK:  // %bb.0:
@@ -12,7 +12,7 @@
 
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, place the label .Lpatch0 after the initial BTI.
-define void @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
+define void @f1() "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1:
 ; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK-NEXT: .cfi_startproc
@@ -28,7 +28,7 @@
 }
 
 ;; -fpatchable-function-entry=2,1 -mbranch-protection=bti
-define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement" {
+define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: .type f2_1,@function
 ; CHECK-NEXT: .Ltmp0:
 ; CHECK-NEXT:  nop
@@ -50,7 +50,7 @@
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, don't create .Lpatch0 if the initial instruction is not BTI,
 ;; even if other basic blocks may have BTI.
-define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement" {
+define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1i:
 ; CHECK-NEXT: .Lfunc_begin3:
 ; CHECK:  // %bb.0:
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
@@ -0,0 +1,27 @@
+; RUN: llc -mtriple=aarch64-linux %s   -o - | \
+; RUN:   FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
+; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
+
+define dso_local i32 @f() #0 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 @g() {
+entry:
+  ret i32 0
+}
+
+attributes #0 = { "branch-target-enforcement"="true" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
+; Only the common atttribute (BTI)
+; ASM:	.word	3221225472
+; ASM-NEXT:	.word	4
+; ASM-NEXT	.word	1
+
+; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
@@ -11,7 +11,11 @@
 
 declare dso_local i32 @g()
 
-attributes #0 = { "branch-target-enforcement" }
+attributes #0 = { "branch-target-enforcement"="true" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
 
 ; Declarations don't prevent setting BTI
 ; ASM:	  

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss updated this revision to Diff 248201.
danielkiss added a comment.

Thanks for the comments, patch is improved
isStringAttribute() check removed, the attribute is always a string in this 
case or "null" so the check is not needed.
Function level the attribute is now only change when needed, so as the function 
level attribute is expected to be rare I hope the performance won't be impacted 
by the patch.
I kept the "tri-state" logic because of the emitted functions. Introducing a 
"branch-target-enforcement-disabled" attribute seems even more confusing for me.


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

https://reviews.llvm.org/D75181

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGen/aarch64-sign-return-address.c
  clang/test/CodeGenCXX/aarch64-branch-target_clang_call_terminate.cpp
  clang/test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/lib/Target/AArch64/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/test/CodeGen/AArch64/branch-target-enforcement-indirect-calls.ll
  llvm/test/CodeGen/AArch64/branch-target-enforcement.mir
  llvm/test/CodeGen/AArch64/bti-branch-relaxation.ll
  llvm/test/CodeGen/AArch64/machine-outliner-bti.mir
  llvm/test/CodeGen/AArch64/machine-outliner-outline-bti.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
  llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll

Index: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
===
--- llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
+++ llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
@@ -1,6 +1,6 @@
 ; RUN: llc -mtriple=aarch64 %s -o - | FileCheck %s
 
-define void @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
+define void @f0() "patchable-function-entry"="0" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f0:
 ; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK:  // %bb.0:
@@ -12,7 +12,7 @@
 
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, place the label .Lpatch0 after the initial BTI.
-define void @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
+define void @f1() "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1:
 ; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK-NEXT: .cfi_startproc
@@ -28,7 +28,7 @@
 }
 
 ;; -fpatchable-function-entry=2,1 -mbranch-protection=bti
-define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement" {
+define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: .type f2_1,@function
 ; CHECK-NEXT: .Ltmp0:
 ; CHECK-NEXT:  nop
@@ -50,7 +50,7 @@
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, don't create .Lpatch0 if the initial instruction is not BTI,
 ;; even if other basic blocks may have BTI.
-define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement" {
+define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1i:
 ; CHECK-NEXT: .Lfunc_begin3:
 ; CHECK:  // %bb.0:
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
@@ -0,0 +1,27 @@
+; RUN: llc -mtriple=aarch64-linux %s   -o - | \
+; RUN:   FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
+; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
+
+define dso_local i32 @f() #0 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 @g() {
+entry:
+  ret i32 0
+}
+
+attributes #0 = { "branch-target-enforcement"="true" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 4, !"branch-target-enforcement", i32 1}
+
+; Only the common atttribute (BTI)
+; ASM:	.word	3221225472
+; ASM-NEXT:	.word	4
+; ASM-NEXT	.word	1
+
+; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss marked 4 inline comments as done.
danielkiss added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1828
+  if (CodeGenOpts.BranchTargetEnforcement) {
+FuncAttrs.addAttribute("branch-target-enforcement", "true");
+  }

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.



Comment at: clang/lib/CodeGen/CGCall.cpp:1831
+
+  auto RASignKind = CodeGenOpts.getSignReturnAddress();
+  if (RASignKind != CodeGenOptions::SignReturnAddressScope::None) {

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.



Comment at: llvm/lib/Target/AArch64/AArch64BranchTargets.cpp:62
+
+  // LLVM emmited function won't have the attribute.
+  if (!F.hasFnAttribute("branch-target-enforcement")) {

emmited -> emitted


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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss updated this revision to Diff 247201.
danielkiss retitled this revision from "[Clang][AArch64] Add default arguments 
to runtime functions." to "[AArch64] Handle BTI/PAC in case of generated 
functions.".
danielkiss edited the summary of this revision.
danielkiss added a project: LLVM.
danielkiss added a comment.
Herald added subscribers: llvm-commits, hiraditya.

Previous version of the patch handled only the functions that are created in 
clang. Sanitizers can't see the codegen options therefore they also disables 
BTI.
This version of the patch is less invasive in my opinion, effects only aarch64.
branch-target-enforcement(BTI) and sign-return-address(PAC) are added to all 
function that comes from clang frontend.
If the backend encounters with the function without BTI attribute but the 
module is compiled with BTI then it assumes it should be made BTI compatible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75181

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGen/aarch64-sign-return-address.c
  clang/test/CodeGenCXX/aarch64-branch-target_clang_call_terminate.cpp
  clang/test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/lib/Target/AArch64/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/test/CodeGen/AArch64/branch-target-enforcement-indirect-calls.ll
  llvm/test/CodeGen/AArch64/branch-target-enforcement.mir
  llvm/test/CodeGen/AArch64/bti-branch-relaxation.ll
  llvm/test/CodeGen/AArch64/machine-outliner-bti.mir
  llvm/test/CodeGen/AArch64/machine-outliner-outline-bti.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
  llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll

Index: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
===
--- llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
+++ llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
@@ -1,6 +1,6 @@
 ; RUN: llc -mtriple=aarch64 %s -o - | FileCheck %s
 
-define void @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
+define void @f0() "patchable-function-entry"="0" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f0:
 ; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK:  // %bb.0:
@@ -12,7 +12,7 @@
 
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, place the label .Lpatch0 after the initial BTI.
-define void @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
+define void @f1() "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1:
 ; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK-NEXT: .cfi_startproc
@@ -28,7 +28,7 @@
 }
 
 ;; -fpatchable-function-entry=2,1 -mbranch-protection=bti
-define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement" {
+define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: .type f2_1,@function
 ; CHECK-NEXT: .Ltmp0:
 ; CHECK-NEXT:  nop
@@ -50,7 +50,7 @@
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, don't create .Lpatch0 if the initial instruction is not BTI,
 ;; even if other basic blocks may have BTI.
-define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement" {
+define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1i:
 ; CHECK-NEXT: .Lfunc_begin3:
 ; CHECK:  // %bb.0:
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-9.ll
@@ -0,0 +1,27 @@
+; RUN: llc -mtriple=aarch64-linux %s   -o - | \
+; RUN:   FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
+; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
+
+define dso_local i32 @f() #0 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 @g() {
+entry:
+  ret i32 0
+}
+