From 7a9286dffe5695be2d2341109b0cf07475ae74dd Mon Sep 17 00:00:00 2001 From: Daniel Kiss Date: Mon, 29 Jan 2024 16:27:50 +0100 Subject: [PATCH 1/2] [llvm][AArch64] Autoupgrade function attributes from Module attributes. sign-return-address and similar module attributes should be propagated to the function level before got merged because module flags may contradict and this information is not recoverable. Generated code will match with the normal linking flow. --- llvm/include/llvm/IR/AutoUpgrade.h | 3 +- llvm/lib/Bitcode/Reader/BitcodeReader.cpp | 2 +- llvm/lib/IR/AutoUpgrade.cpp | 77 ++++++++++++++++++- llvm/lib/Linker/IRMover.cpp | 4 + .../test/Bitcode/upgrade-arc-runtime-calls.ll | 4 +- .../AArch64/link-branch-target-enforcement.ll | 1 + .../LTO/AArch64/link-sign-return-address.ll | 43 +++++++++++ llvm/test/Linker/link-arm-and-thumb.ll | 7 +- 8 files changed, 133 insertions(+), 8 deletions(-) create mode 100644 llvm/test/LTO/AArch64/link-sign-return-address.ll 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 515a1d0caa041..7954537882e15 100644 --- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp @@ -6705,7 +6705,7 @@ Error BitcodeReader::materialize(GlobalValue *GV) { } // Look for functions that rely on old function attribute behavior. - UpgradeFunctionAttributes(*F); + UpgradeFunctionAttributes(*F, true); // Bring in any functions that this function forward-referenced via // blockaddresses. diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp index b90bbe71ac189..86f7bc1ed62f4 100644 --- a/llvm/lib/IR/AutoUpgrade.cpp +++ b/llvm/lib/IR/AutoUpgrade.cpp @@ -5155,7 +5155,51 @@ struct StrictFPUpgradeVisitor : public InstVisitor { }; } // namespace -void llvm::UpgradeFunctionAttributes(Function &F) { +// Check if the module attribute is present and not zero. +static bool isModuleAttributeSet(const Module *M, const StringRef &ModAttr) { + if (const auto *Attr = + mdconst::extract_or_null(M->getModuleFlag(ModAttr))) + if (Attr->getZExtValue()) + return true; + return false; +} + +// Copy an attribute from module to the function if exists. +// First value of the pair is used when the module attribute is not zero +// the second otherwise. +static void +CopyModuleAttributeToFunction(Function &F, StringRef FnAttrName, + StringRef ModAttrName, + std::pair Values) { + Module *M = F.getParent(); + assert(M && "Missing module"); + if (F.hasFnAttribute(FnAttrName)) + return; + if (isModuleAttributeSet(M, ModAttrName)) + F.addFnAttr(FnAttrName, Values.first); + else + F.addFnAttr(FnAttrName, Values.second); +} + +// Copy a boolean attribute from module to the function if exists. +// Module attribute treated false if zero otherwise true. +static void CopyModuleAttributeToFunction(Function &F, StringRef AttrName) { + CopyModuleAttributeToFunction( + F, AttrName, AttrName, + std::make_pair("true", "false")); +} + +// Copy an attribute from module to the function if exists. +// First value of the pair is used when the module attribute is not zero +// the second otherwise. +static void +CopyModuleAttributeToFunction(Function &F, StringRef AttrName, + std::pair 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 +5211,37 @@ 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 (isModuleAttributeSet(M, "sign-return-address")) + SignType = "non-leaf"; + + if (isModuleAttributeSet(M, "sign-return-address-all")) + 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("b_key", "a_key")); + } } static bool isOldLoopArgument(Metadata *MD) { diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp index 37d21119447b9..9f45ebc6eda01 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 copying 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/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:
: ; CHECK-DUMP: bl 0x8 ; CHECK-DUMP: : +; 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..c25857ceed7b4 --- /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: : +; CHECK-DUMP: paciasp +; CHECK-DUMP: mov w0, #0x2a +; CHECK-DUMP: autiasp +; CHECK-DUMP: ret +; CHECK-DUMP:
: +; CHECK-DUMP-NOT: paciasp +; CHECK-DUMP: str x30, +; CHECK-DUMP: bl 0x14 + +; `main` doesn't support PAC sign-return-address while `foo` does, so in the binary +; we should not see anything. +; CHECK-PROP-NOT: Properties: 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: From a0396b297dae3b4e3dcc511b0ab56ec5924f44da Mon Sep 17 00:00:00 2001 From: Daniel Kiss Date: Wed, 21 Feb 2024 20:38:43 +0100 Subject: [PATCH 2/2] Address review comments. --- llvm/lib/IR/AutoUpgrade.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp index 86f7bc1ed62f4..edff13c796b31 100644 --- a/llvm/lib/IR/AutoUpgrade.cpp +++ b/llvm/lib/IR/AutoUpgrade.cpp @@ -5157,11 +5157,9 @@ struct StrictFPUpgradeVisitor : public InstVisitor { // Check if the module attribute is present and not zero. static bool isModuleAttributeSet(const Module *M, const StringRef &ModAttr) { - if (const auto *Attr = - mdconst::extract_or_null(M->getModuleFlag(ModAttr))) - if (Attr->getZExtValue()) - return true; - return false; + const auto *Attr = + mdconst::extract_or_null(M->getModuleFlag(ModAttr)); + return Attr && Attr->getZExtValue(); } // Copy an attribute from module to the function if exists. @@ -5171,14 +5169,11 @@ static void CopyModuleAttributeToFunction(Function &F, StringRef FnAttrName, StringRef ModAttrName, std::pair Values) { - Module *M = F.getParent(); - assert(M && "Missing module"); if (F.hasFnAttribute(FnAttrName)) return; - if (isModuleAttributeSet(M, ModAttrName)) - F.addFnAttr(FnAttrName, Values.first); - else - F.addFnAttr(FnAttrName, Values.second); + F.addFnAttr(FnAttrName, isModuleAttributeSet(F.getParent(), ModAttrName) + ? Values.first + : Values.second); } // Copy a boolean attribute from module to the function if exists.