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

[llvm][ARM][AArch64] Add attributes to synthetic functions. #83153

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

DanielKristofKiss
Copy link
Member

@DanielKristofKiss DanielKristofKiss commented Feb 27, 2024

Module flags represent the original intention.

Depends on #82819

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 27, 2024

@llvm/pr-subscribers-llvm-ir

Author: Dani (DanielKristofKiss)

Changes

Module flags represent the original intention.


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

1 Files Affected:

  • (modified) llvm/lib/IR/Function.cpp (+36)
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index 056e4f31981a72..5e217cc5680186 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -65,6 +65,8 @@
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ModRef.h"
+#include "llvm/TargetParser/ARMTargetParser.h"
+#include "llvm/TargetParser/Triple.h"
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
@@ -378,6 +380,40 @@ Function *Function::createWithDefaultAttr(FunctionType *Ty,
   }
   if (M->getModuleFlag("function_return_thunk_extern"))
     B.addAttribute(Attribute::FnRetThunkExtern);
+
+  Triple T(M->getTargetTriple());
+
+  auto Profile = llvm::ARM::parseArchProfile(T.getArchName());
+  if ((T.isArmT32() && Profile == llvm::ARM::ProfileKind::M) || T.isAArch64()) {
+    // Check if the module attribute is present and not zero.
+    auto isModuleAttributeSet = [&](const StringRef &ModAttr) -> bool {
+      const auto *Attr =
+          mdconst::extract_or_null<ConstantInt>(M->getModuleFlag(ModAttr));
+      return Attr && Attr->getZExtValue();
+    };
+
+    auto AddAttributeIfSet = [&](const StringRef &ModAttr) {
+      if (isModuleAttributeSet(ModAttr))
+        B.addAttribute(ModAttr, "true");
+    };
+
+    StringRef SignType = "none";
+    if (isModuleAttributeSet("sign-return-address"))
+      SignType = "non-leaf";
+    if (isModuleAttributeSet("sign-return-address-all"))
+      SignType = "all";
+    if (SignType != "none") {
+      B.addAttribute("sign-return-address", SignType);
+      if (T.isAArch64())
+        B.addAttribute("sign-return-address-key",
+                       isModuleAttributeSet("sign-return-address-with-bkey")
+                           ? "b_key"
+                           : "a_key");
+    }
+    AddAttributeIfSet("branch-target-enforcement");
+    AddAttributeIfSet("branch-protection-pauth-lr");
+    AddAttributeIfSet("guarded-control-stack");
+  }
   F->addFnAttrs(B);
   return F;
 }

@MaskRay
Copy link
Member

MaskRay commented Feb 28, 2024

test?

Comment on lines 401 to 404
if (isModuleAttributeSet("sign-return-address"))
SignType = "non-leaf";
if (isModuleAttributeSet("sign-return-address-all"))
SignType = "all";
Copy link
Member

Choose a reason for hiding this comment

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

Can both be set at the same time? Is there a verifier check to enforce that if not?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, only one of them should present.
There is no check there in the verifier. I didn't see any obvious spot for it, I'll create something for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

#83635 is about the verification.

Comment on lines 386 to 387
auto Profile = llvm::ARM::parseArchProfile(T.getArchName());
if ((T.isArmT32() && Profile == llvm::ARM::ProfileKind::M) || T.isAArch64()) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can avoid bringing in Triple.h and ARMTargetParser if we just always check for these attributes (even if they are arch specific? I guess I'm curious about the M profile check here. If we ran this on non-m-profile arm32, would the module attributes even ever exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

That won't happen because the fronted won't add the flag, even it they present the backend won't do anything with them.

@DanielKristofKiss
Copy link
Member Author

Depends on #82819

Module flags represent the original intention.
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. Please avoid force pushing, it makes reviewing more difficult because we can't see what changed since the last time.

@DanielKristofKiss DanielKristofKiss merged commit 4c7fd7e into llvm:main Jul 10, 2024
4 of 7 checks passed
DanielKristofKiss added a commit that referenced this pull request Jul 10, 2024
Module flags represent the original intention.

Depends on #82819

Reland as it was but reland needed as dependent patch relanded.
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
Module flags represent the original intention.

Depends on llvm#82819
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
Module flags represent the original intention.

Depends on llvm#82819

Reland as it was but reland needed as dependent patch relanded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants