[llvm] [clang] [llvm][AArch64] Do not inline a function with different signing scheme. (PR #80642)

via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 5 00:02:31 PST 2024


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-lto

Author: Dani (DanielKristofKiss)

<details>
<summary>Changes</summary>

If the signing scheme is different that maybe the functions assumes different behaviours and dangerous to inline them without analysing them. This should be a rare case.

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


11 Files Affected:

- (modified) clang/lib/CodeGen/CodeGenModule.cpp (+4-10) 
- (modified) llvm/include/llvm/IR/AutoUpgrade.h (+2-1) 
- (modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+1-1) 
- (modified) llvm/lib/IR/AutoUpgrade.cpp (+68-1) 
- (modified) llvm/lib/Linker/IRMover.cpp (+4) 
- (modified) llvm/lib/Transforms/Utils/InlineFunction.cpp (+15) 
- (modified) llvm/test/Bitcode/upgrade-arc-runtime-calls.ll (+2-2) 
- (modified) llvm/test/LTO/AArch64/link-branch-target-enforcement.ll (+1) 
- (added) llvm/test/LTO/AArch64/link-sign-return-address.ll (+43) 
- (modified) llvm/test/Linker/link-arm-and-thumb.ll (+4-3) 
- (added) llvm/test/Transforms/Inline/inline-sign-return-address.ll (+126) 


``````````diff
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index c63e4ecc3dcba..36b63d78b06f8 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -1044,17 +1044,14 @@ void CodeGenModule::Release() {
                               llvm::MDString::get(VMContext, "ascii"));
   }
 
-  llvm::Triple::ArchType Arch = Context.getTargetInfo().getTriple().getArch();
-  if (   Arch == llvm::Triple::arm
-      || Arch == llvm::Triple::armeb
-      || Arch == llvm::Triple::thumb
-      || Arch == llvm::Triple::thumbeb) {
+  llvm::Triple T = Context.getTargetInfo().getTriple();
+  if (T.isARM() || T.isThumb()) {
     // The minimum width of an enum in bytes
     uint64_t EnumWidth = Context.getLangOpts().ShortEnums ? 1 : 4;
     getModule().addModuleFlag(llvm::Module::Error, "min_enum_size", EnumWidth);
   }
 
-  if (Arch == llvm::Triple::riscv32 || Arch == llvm::Triple::riscv64) {
+  if (T.isRISCV()) {
     StringRef ABIStr = Target.getABI();
     llvm::LLVMContext &Ctx = TheModule.getContext();
     getModule().addModuleFlag(llvm::Module::Error, "target-abi",
@@ -1127,10 +1124,7 @@ void CodeGenModule::Release() {
     getModule().addModuleFlag(llvm::Module::Override,
                               "tag-stack-memory-buildattr", 1);
 
-  if (Arch == llvm::Triple::thumb || Arch == llvm::Triple::thumbeb ||
-      Arch == llvm::Triple::arm || Arch == llvm::Triple::armeb ||
-      Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_32 ||
-      Arch == llvm::Triple::aarch64_be) {
+  if (T.isARM() || T.isThumb() || T.isAArch64()) {
     if (LangOpts.BranchTargetEnforcement)
       getModule().addModuleFlag(llvm::Module::Min, "branch-target-enforcement",
                                 1);
diff --git a/llvm/include/llvm/IR/AutoUpgrade.h b/llvm/include/llvm/IR/AutoUpgrade.h
index 152f781ffa9b3..c0d96efc54752 100644
--- a/llvm/include/llvm/IR/AutoUpgrade.h
+++ b/llvm/include/llvm/IR/AutoUpgrade.h
@@ -67,7 +67,8 @@ namespace llvm {
   void UpgradeSectionAttributes(Module &M);
 
   /// Correct any IR that is relying on old function attribute behavior.
-  void UpgradeFunctionAttributes(Function &F);
+  void UpgradeFunctionAttributes(Function &F,
+                                 bool ModuleMetadataIsMaterialized = false);
 
   /// If the given TBAA tag uses the scalar TBAA format, create a new node
   /// corresponding to the upgrade to the struct-path aware TBAA format.
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 5b233fb365fe2..6b335dd9f1f89 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -6708,7 +6708,7 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
   }
 
   // Look for functions that rely on old function attribute behavior.
-  UpgradeFunctionAttributes(*F);
+  UpgradeFunctionAttributes(*F, true);
 
   // If we've materialized a function set up in "new" debug-info mode, the
   // contents just loaded will still be in dbg.value mode. Switch to the new
diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index 19d80eb9aec0b..e25ac46450cec 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -5155,7 +5155,39 @@ struct StrictFPUpgradeVisitor : public InstVisitor<StrictFPUpgradeVisitor> {
 };
 } // namespace
 
-void llvm::UpgradeFunctionAttributes(Function &F) {
+static void
+CopyModuleAttributeToFunction(Function &F, StringRef FnAttrName,
+                              StringRef ModAttrName,
+                              std::pair<StringRef, StringRef> Values) {
+  Module *M = F.getParent();
+  if (!M)
+    return;
+  if (F.hasFnAttribute(FnAttrName))
+    return;
+  if (const auto *MAttr = mdconst::extract_or_null<ConstantInt>(
+          M->getModuleFlag(ModAttrName))) {
+    if (MAttr->getZExtValue()) {
+      F.addFnAttr(FnAttrName, Values.first);
+      return;
+    }
+  }
+  F.addFnAttr(FnAttrName, Values.second);
+}
+
+static void CopyModuleAttributeToFunction(Function &F, StringRef AttrName) {
+  CopyModuleAttributeToFunction(
+      F, AttrName, AttrName,
+      std::make_pair<StringRef, StringRef>("true", "false"));
+}
+
+static void
+CopyModuleAttributeToFunction(Function &F, StringRef AttrName,
+                              std::pair<StringRef, StringRef> Values) {
+  CopyModuleAttributeToFunction(F, AttrName, AttrName, Values);
+}
+
+void llvm::UpgradeFunctionAttributes(Function &F,
+                                     bool ModuleMetadataIsMaterialized) {
   // If a function definition doesn't have the strictfp attribute,
   // convert any callsite strictfp attributes to nobuiltin.
   if (!F.isDeclaration() && !F.hasFnAttribute(Attribute::StrictFP)) {
@@ -5167,6 +5199,41 @@ void llvm::UpgradeFunctionAttributes(Function &F) {
   F.removeRetAttrs(AttributeFuncs::typeIncompatible(F.getReturnType()));
   for (auto &Arg : F.args())
     Arg.removeAttrs(AttributeFuncs::typeIncompatible(Arg.getType()));
+
+  if (!ModuleMetadataIsMaterialized)
+    return;
+  if (F.isDeclaration())
+    return;
+  Module *M = F.getParent();
+  if (!M)
+    return;
+
+  Triple T(M->getTargetTriple());
+  // Convert module level attributes to function level attributes because
+  // after merging modules the attributes might change and would have different
+  // effect on the functions as the original module would have.
+  if (T.isThumb() || T.isARM() || T.isAArch64()) {
+    if (!F.hasFnAttribute("sign-return-address")) {
+      StringRef SignType = "none";
+      if (const auto *Sign = mdconst::extract_or_null<ConstantInt>(
+              M->getModuleFlag("sign-return-address")))
+        if (Sign->getZExtValue())
+          SignType = "non-leaf";
+
+      if (const auto *SignAll = mdconst::extract_or_null<ConstantInt>(
+              M->getModuleFlag("sign-return-address-all")))
+        if (SignAll->getZExtValue())
+          SignType = "all";
+
+      F.addFnAttr("sign-return-address", SignType);
+    }
+    CopyModuleAttributeToFunction(F, "branch-target-enforcement");
+    CopyModuleAttributeToFunction(F, "branch-protection-pauth-lr");
+    CopyModuleAttributeToFunction(F, "guarded-control-stack");
+    CopyModuleAttributeToFunction(
+        F, "sign-return-address-key",
+        std::make_pair<StringRef, StringRef>("b_key", "a_key"));
+  }
 }
 
 static bool isOldLoopArgument(Metadata *MD) {
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 8cc0f7fb90991..47d5a39c9f746 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -1606,6 +1606,10 @@ Error IRLinker::run() {
   // Loop over all of the linked values to compute type mappings.
   computeTypeMapping();
 
+  // Update function attributes before copy them to destation module.
+  for (Function &F : SrcM->getFunctionList())
+    UpgradeFunctionAttributes(F, true);
+
   std::reverse(Worklist.begin(), Worklist.end());
   while (!Worklist.empty()) {
     GlobalValue *GV = Worklist.back();
diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index d4d4bf5ebdf36..639c77716463e 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -2110,6 +2110,21 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
     return InlineResult::failure("incompatible strictfp attributes");
   }
 
+  // Do not inline function with a different signing scheme.
+  if (CalledFunc->getFnAttribute("sign-return-address") !=
+      Caller->getFnAttribute("sign-return-address")) {
+    return InlineResult::failure("incompatible sign return address attributes");
+  }
+  if (CalledFunc->getFnAttribute("sign-return-address-key") !=
+      Caller->getFnAttribute("sign-return-address-key")) {
+    return InlineResult::failure("incompatible signing keys attributes");
+  }
+  if (CalledFunc->getFnAttribute("branch-protection-pauth-lr") !=
+      Caller->getFnAttribute("branch-protection-pauth-lr")) {
+    return InlineResult::failure(
+        "incompatible sign return address modifier attributes");
+  }
+
   // GC poses two hazards to inlining, which only occur when the callee has GC:
   //  1. If the caller has no GC, then the callee's GC must be propagated to the
   //     caller.
diff --git a/llvm/test/Bitcode/upgrade-arc-runtime-calls.ll b/llvm/test/Bitcode/upgrade-arc-runtime-calls.ll
index 19f25f98953fa..d2edec18d55e5 100644
--- a/llvm/test/Bitcode/upgrade-arc-runtime-calls.ll
+++ b/llvm/test/Bitcode/upgrade-arc-runtime-calls.ll
@@ -55,7 +55,7 @@ unwindBlock:
 // Check that auto-upgrader converts function calls to intrinsic calls. Note that
 // the auto-upgrader doesn't touch invoke instructions.
 
-// ARC: define void @testRuntimeCalls(ptr %[[A:.*]], ptr %[[B:.*]], ptr %[[C:.*]], ptr %[[D:.*]], ptr %[[E:.*]]) personality
+// ARC: define void @testRuntimeCalls(ptr %[[A:.*]], ptr %[[B:.*]], ptr %[[C:.*]], ptr %[[D:.*]], ptr %[[E:.*]]) #0 personality
 // ARC: %[[V0:.*]] = tail call ptr @llvm.objc.autorelease(ptr %[[A]])
 // ARC-NEXT: tail call void @llvm.objc.autoreleasePoolPop(ptr %[[A]])
 // ARC-NEXT: %[[V1:.*]] = tail call ptr @llvm.objc.autoreleasePoolPush()
@@ -88,7 +88,7 @@ unwindBlock:
 // ARC-NEXT: tail call void @llvm.objc.arc.annotation.bottomup.bbend(ptr %[[B]], ptr %[[C]])
 // ARC-NEXT: invoke void @objc_autoreleasePoolPop(ptr %[[A]])
 
-// NOUPGRADE: define void @testRuntimeCalls(ptr %[[A:.*]], ptr %[[B:.*]], ptr %[[C:.*]], ptr %[[D:.*]], ptr %[[E:.*]]) personality
+// NOUPGRADE: define void @testRuntimeCalls(ptr %[[A:.*]], ptr %[[B:.*]], ptr %[[C:.*]], ptr %[[D:.*]], ptr %[[E:.*]]) #0 personality
 // NOUPGRADE: %[[V0:.*]] = tail call ptr @objc_autorelease(ptr %[[A]])
 // NOUPGRADE-NEXT: tail call void @objc_autoreleasePoolPop(ptr %[[A]])
 // NOUPGRADE-NEXT: %[[V1:.*]] = tail call ptr @objc_autoreleasePoolPush()
diff --git a/llvm/test/LTO/AArch64/link-branch-target-enforcement.ll b/llvm/test/LTO/AArch64/link-branch-target-enforcement.ll
index ccf8cf67ede6d..74d9c86881d52 100644
--- a/llvm/test/LTO/AArch64/link-branch-target-enforcement.ll
+++ b/llvm/test/LTO/AArch64/link-branch-target-enforcement.ll
@@ -32,6 +32,7 @@ entry:
 ; CHECK-DUMP: <main>:
 ; CHECK-DUMP:      bl      0x8 <main+0x8>
 ; CHECK-DUMP: <foo>:
+; CHECK-DUMP:     paciasp
 
 ; `main` doesn't support BTI while `foo` does, so in the binary
 ; we should see only PAC which is supported by both.
diff --git a/llvm/test/LTO/AArch64/link-sign-return-address.ll b/llvm/test/LTO/AArch64/link-sign-return-address.ll
new file mode 100644
index 0000000000000..b2e70b2b04e08
--- /dev/null
+++ b/llvm/test/LTO/AArch64/link-sign-return-address.ll
@@ -0,0 +1,43 @@
+; Testcase to check that module with different branch-target-enforcement can
+; be mixed.
+;
+; RUN: llvm-as %s -o %t1.bc
+; RUN: llvm-as %p/Inputs/foo.ll -o %t2.bc
+; RUN: llvm-lto -exported-symbol main \
+; RUN:          -exported-symbol foo \
+; RUN:          -filetype=obj \
+; RUN:           %t2.bc %t1.bc \
+; RUN:           -o %t1.exe 2>&1
+; RUN: llvm-objdump -d %t1.exe | FileCheck --check-prefix=CHECK-DUMP %s
+; RUN: llvm-readelf -n %t1.exe | FileCheck --allow-empty --check-prefix=CHECK-PROP %s
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
+declare i32 @foo();
+
+define i32 @main() {
+entry:
+  %add = call i32 @foo()
+  ret i32 %add
+}
+
+!llvm.module.flags = !{!0, !1, !2, !3 }
+!0 = !{i32 8, !"branch-target-enforcement", i32 0}
+!1 = !{i32 8, !"sign-return-address", i32 0}
+!2 = !{i32 8, !"sign-return-address-all", i32 0}
+!3 = !{i32 8, !"sign-return-address-with-bkey", i32 0}
+
+; CHECK-DUMP: <foo>:
+; CHECK-DUMP:     paciasp
+; CHECK-DUMP:     mov     w0, #0x2a
+; CHECK-DUMP:     autiasp
+; CHECK-DUMP:     ret
+; CHECK-DUMP: <main>:
+; CHECK-DUMP-NOT:  paciasp
+; CHECK-DUMP:      str     x30,
+; CHECK-DUMP:      bl      0x14 <main+0x4>
+
+; `main` doesn't support PAC sign-return-address while `foo` does, so in the binary
+; we should not see anything.
+; CHECK-PROP-NOT:   Proper ties: aarch64 feature: PAC
\ No newline at end of file
diff --git a/llvm/test/Linker/link-arm-and-thumb.ll b/llvm/test/Linker/link-arm-and-thumb.ll
index a90f2128e4430..37bd8c37f8b5e 100644
--- a/llvm/test/Linker/link-arm-and-thumb.ll
+++ b/llvm/test/Linker/link-arm-and-thumb.ll
@@ -13,11 +13,12 @@ entry:
   ret i32 %add
 }
 
-; CHECK: define i32 @main() {
+; CHECK: define i32 @main() [[MAIN_ATTRS:#[0-9]+]]
 ; CHECK: define i32 @foo(i32 %a, i32 %b) [[ARM_ATTRS:#[0-9]+]]
 ; CHECK: define i32 @bar(i32 %a, i32 %b) [[THUMB_ATTRS:#[0-9]+]]
 
-; CHECK: attributes [[ARM_ATTRS]] = { "target-features"="-thumb-mode" }
-; CHECK: attributes [[THUMB_ATTRS]] = { "target-features"="+thumb-mode" }
+; CHECK: attributes [[MAIN_ATTRS]] = { {{.*}} }
+; CHECK: attributes [[ARM_ATTRS]] = { {{.*}} "target-features"="-thumb-mode" }
+; CHECK: attributes [[THUMB_ATTRS]] = { {{.*}} "target-features"="+thumb-mode" }
 
 ; STDERR-NOT: warning: Linking two modules of different target triples:
diff --git a/llvm/test/Transforms/Inline/inline-sign-return-address.ll b/llvm/test/Transforms/Inline/inline-sign-return-address.ll
new file mode 100644
index 0000000000000..d83460e1e7684
--- /dev/null
+++ b/llvm/test/Transforms/Inline/inline-sign-return-address.ll
@@ -0,0 +1,126 @@
+; Check the inliner doesn't inline a function with different sign return address schemes.
+; RUN: opt < %s -passes=inline -S | FileCheck %s
+
+declare void @init(ptr)
+
+define internal i32 @foo_all() #0 {
+  ret i32 43
+}
+
+define internal i32 @foo_nonleaf() #1 {
+  ret i32 44
+}
+
+define internal i32 @foo_none() #2 {
+  ret i32 42
+}
+
+define internal i32 @foo_lr() #3 {
+  ret i32 45
+}
+
+define internal i32 @foo_bkey() #4 {
+  ret i32 46
+}
+
+define dso_local i32 @bar_all() #0 {
+; CHECK-LABEL: bar_all
+; CHECK-NOT:   call i32 @foo_all()
+; CHECK:       call i32 @foo_nonleaf()
+; CHECK:       call i32 @foo_none()
+; CHECK:       call i32 @foo_lr()
+; CHECK:       call i32 @foo_bkey()
+  %1 = call i32 @foo_all()
+  %2 = call i32 @foo_nonleaf()
+  %3 = call i32 @foo_none()
+  %4 = call i32 @foo_lr()
+  %5 = call i32 @foo_bkey()
+  %6 = add nsw i32 %1, %2
+  %7 = add nsw i32 %6, %3
+  %8 = add nsw i32 %7, %4
+  %9 = add nsw i32 %8, %5
+  ret i32 %9
+}
+
+define dso_local i32 @bar_nonleaf() #1 {
+; CHECK-LABEL: bar_nonleaf
+; CHECK:       call i32 @foo_all()
+; CHECK-NOT:   call i32 @foo_nonleaf()
+; CHECK:       call i32 @foo_none()
+; CHECK:       call i32 @foo_lr()
+; CHECK:       call i32 @foo_bkey()
+  %1 = call i32 @foo_all()
+  %2 = call i32 @foo_nonleaf()
+  %3 = call i32 @foo_none()
+  %4 = call i32 @foo_lr()
+  %5 = call i32 @foo_bkey()
+  %6 = add nsw i32 %1, %2
+  %7 = add nsw i32 %6, %3
+  %8 = add nsw i32 %7, %4
+  %9 = add nsw i32 %8, %5
+  ret i32 %9
+}
+
+define dso_local i32 @bar_none() #2 {
+; CHECK-LABEL: bar_none
+; CHECK:       call i32 @foo_all()
+; CHECK:       call i32 @foo_nonleaf()
+; CHECK-NOT:   call i32 @foo_none()
+; CHECK:       call i32 @foo_lr()
+; CHECK:       call i32 @foo_bkey()
+  %1 = call i32 @foo_all()
+  %2 = call i32 @foo_nonleaf()
+  %3 = call i32 @foo_none()
+  %4 = call i32 @foo_lr()
+  %5 = call i32 @foo_bkey()
+  %6 = add nsw i32 %1, %2
+  %7 = add nsw i32 %6, %3
+  %8 = add nsw i32 %7, %4
+  %9 = add nsw i32 %8, %5
+  ret i32 %9
+}
+
+define dso_local i32 @bar_lr() #3 {
+; CHECK-LABEL: bar_lr
+; CHECK:       call i32 @foo_all()
+; CHECK:       call i32 @foo_nonleaf()
+; CHECK:       call i32 @foo_none()
+; CHECK-NOT:   call i32 @foo_lr()
+; CHECK:       call i32 @foo_bkey()
+  %1 = call i32 @foo_all()
+  %2 = call i32 @foo_nonleaf()
+  %3 = call i32 @foo_none()
+  %4 = call i32 @foo_lr()
+  %5 = call i32 @foo_bkey()
+  %6 = add nsw i32 %1, %2
+  %7 = add nsw i32 %6, %3
+  %8 = add nsw i32 %7, %4
+  %9 = add nsw i32 %8, %5
+  ret i32 %9
+}
+
+define dso_local i32 @bar_bkey() #4 {
+; CHECK-LABEL: bar_bkey
+; CHECK:       call i32 @foo_all()
+; CHECK:       call i32 @foo_nonleaf()
+; CHECK:       call i32 @foo_none()
+; CHECK:       call i32 @foo_lr()
+; CHECK-NOT:   call i32 @foo_bkey()
+  %1 = call i32 @foo_all()
+  %2 = call i32 @foo_nonleaf()
+  %3 = call i32 @foo_none()
+  %4 = call i32 @foo_lr()
+  %5 = call i32 @foo_bkey()
+  %6 = add nsw i32 %1, %2
+  %7 = add nsw i32 %6, %3
+  %8 = add nsw i32 %7, %4
+  %9 = add nsw i32 %8, %5
+  ret i32 %9
+}
+
+
+attributes #0 = { "branch-protection-pauth-lr"="false" "sign-return-address"="all" }
+attributes #1 = { "branch-protection-pauth-lr"="false" "sign-return-address"="non-leaf" }
+attributes #2 = { "branch-protection-pauth-lr"="false" "sign-return-address"="none" }
+attributes #3 = { "branch-protection-pauth-lr"="true" "sign-return-address"="non-leaf" }
+attributes #4 = { "branch-protection-pauth-lr"="true" "sign-return-address"="non-leaf" "sign-return-address-key"="b_key" }
\ No newline at end of file

``````````

</details>


https://github.com/llvm/llvm-project/pull/80642


More information about the llvm-commits mailing list