Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang][ARM][AArch64] Add branch protection attributes to the defaults. #83277

Merged
merged 9 commits into from
Jul 12, 2024

Conversation

DanielKristofKiss
Copy link
Member

These attributes are no longer inherited from the module flags, therefore need to be added for synthetic functions.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels Feb 28, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 28, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Dani (DanielKristofKiss)

Changes

These attributes are no longer inherited from the module flags, therefore need to be added for synthetic functions.


Full diff: https://github.com/llvm/llvm-project/pull/83277.diff

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGCall.cpp (+16)
  • (added) clang/test/CodeGenCXX/arm64-generated-fn-attr.cpp (+30)
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index d05cf1c6e1814e..d677a2e64be77e 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2022,6 +2022,22 @@ static void getTrivialDefaultFunctionAttributes(
     std::tie(Var, Value) = Attr.split('=');
     FuncAttrs.addAttribute(Var, Value);
   }
+
+  TargetInfo::BranchProtectionInfo BPI(LangOpts);
+
+  if (BPI.SignReturnAddr != LangOptions::SignReturnAddressScopeKind::None) {
+    FuncAttrs.addAttribute("sign-return-address", BPI.getSignReturnAddrStr());
+    FuncAttrs.addAttribute(
+        "sign-return-address-key",
+        BPI.SignKey == LangOptions::SignReturnAddressKeyKind::AKey ? "a_key"
+                                                                   : "b_key");
+  }
+  if (BPI.BranchTargetEnforcement)
+    FuncAttrs.addAttribute("branch-target-enforcement", "true");
+  if (BPI.BranchProtectionPAuthLR)
+    FuncAttrs.addAttribute("branch-protection-pauth-lr", "true");
+  if (BPI.GuardedControlStack)
+    FuncAttrs.addAttribute("guarded-control-stack", "true");
 }
 
 /// Merges `target-features` from \TargetOpts and \F, and sets the result in
diff --git a/clang/test/CodeGenCXX/arm64-generated-fn-attr.cpp b/clang/test/CodeGenCXX/arm64-generated-fn-attr.cpp
new file mode 100644
index 00000000000000..8daf44abd4f91c
--- /dev/null
+++ b/clang/test/CodeGenCXX/arm64-generated-fn-attr.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple aarch64-none-none -mbranch-target-enforce -msign-return-address=all -fcxx-exceptions -fexceptions -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK
+
+// Check that functions generated by clang have the correct attributes
+
+class Example {
+public:
+  Example();
+  int fn();
+};
+
+// Initialization of var1 causes __cxx_global_var_init and __tls_init to be generated
+thread_local Example var1;
+extern thread_local Example var2;
+extern void fn();
+
+int testfn() noexcept {
+  // Calling fn in a noexcept function causes __clang_call_terminate to be generated
+  fn();
+  // Use of var1 and var2 causes TLS wrapper functions to be generated
+  return var1.fn() + var2.fn();
+}
+
+// CHECK: define {{.*}} @__cxx_global_var_init() [[ATTR1:#[0-9]+]]
+// CHECK: define {{.*}} @__clang_call_terminate({{.*}}) [[ATTR2:#[0-9]+]]
+// CHECK: define {{.*}} @_ZTW4var1() [[ATTR1]]
+// CHECK: define {{.*}} @_ZTW4var2() [[ATTR1]]
+// CHECK: define {{.*}} @__tls_init() [[ATTR1]]
+
+// CHECK: attributes [[ATTR1]] = { {{.*}}"branch-target-enforcement"="true"{{.*}}"sign-return-address"="all" "sign-return-address-key"="a_key"
+// CHECK: attributes [[ATTR2]] = { {{.*}}"branch-target-enforcement"="true"{{.*}}"sign-return-address"="all" "sign-return-address-key"="a_key"

Copy link
Contributor

@tmatheson-arm tmatheson-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DanielKristofKiss
Copy link
Member Author

Thanks.
I'm planning to land once all related patches are ready to land and in this order:
#82819 #83277 #83153 #83154

@@ -2022,6 +2022,19 @@ static void getTrivialDefaultFunctionAttributes(
std::tie(Var, Value) = Attr.split('=');
FuncAttrs.addAttribute(Var, Value);
}

TargetInfo::BranchProtectionInfo BPI(LangOpts);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after #82819 lands, this can be 2 lines

Suggested change
TargetInfo::BranchProtectionInfo BPI(LangOpts);
BPI.setFnAttributes(FuncAttrs);

@@ -0,0 +1,30 @@
// RUN: %clang_cc1 -triple aarch64-none-none -mbranch-target-enforce -msign-return-address=all -fcxx-exceptions -fexceptions -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it's time to add CodeGenCXX/AArch64/ ? We have more target-specific directories under CodeGen.

Then the weird arm64- prefix (not recommended for ELF/Arm official) can be removed.

You can remove -fcxx-exceptions -fexceptions since they are unused. -triple aarch64-none-nonecan just be-triple aarch64`.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it's time to add CodeGenCXX/AArch64/ ? We have more target-specific directories under CodeGen.
Then the weird arm64- prefix (not recommended for ELF/Arm official) can be removed.

I'll create a NFC patch that cleans all of these up. (also in CodeGen where we have directories and prefixed files too.)

You can remove -fcxx-exceptions -fexceptions since they are unused

__clang_call_terminate is only generated when the exceptions are enabled. Clang generates the definition for it too and the test checks the right attributes for it.

-triple aarch64-none-nonecan just be-triple aarch64`.

ack

These attributes are no longer inherited from the module flags,
therefore need to be added for synthetic functions.
Dropping restrictions on the member functions.
@DanielKristofKiss
Copy link
Member Author

Sorry for the force push, need to be rebased. NFC since last review round.

@DanielKristofKiss DanielKristofKiss merged commit 7d1b6b2 into llvm:main Jul 12, 2024
7 checks passed
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…s. (llvm#83277)

These attributes are no longer inherited from the module flags,
therefore need to be added for synthetic functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants