From 451799bb8261bde52bbfef226d019caf1d82aa42 Mon Sep 17 00:00:00 2001 From: Tim Neumann Date: Tue, 21 Feb 2023 18:35:30 +0100 Subject: [PATCH] [IRMover] Remove UB implying parameter attributes when necessary When importing functions from some module X into some module Y, they may reference other functions already present in Y. The signature (especially attributes) of those other functions may have diverged between X and Y (e.g. due to the Dead Argument Elimination optimization). If necessary, modify the attributes to avoid UB. See the added test and implementation comments for more details. This was exposed by https://reviews.llvm.org/D133036 before it was reverted. Fixes https://github.com/llvm/llvm-project/issues/58976. Reviewed By: tejohnson Differential Revision: https://reviews.llvm.org/D139209 --- llvm/include/llvm/IR/Attributes.h | 7 +++ llvm/lib/Linker/IRMover.cpp | 54 +++++++++++++++++++--- .../Inputs/attr_fixup_dae_noundef.ll | 17 +++++++ .../FunctionImport/attr_fixup_dae_noundef.ll | 34 ++++++++++++++ 4 files changed, 105 insertions(+), 7 deletions(-) create mode 100644 llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll create mode 100644 llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h index c4e12a6..fa179c9 100644 --- a/llvm/include/llvm/IR/Attributes.h +++ b/llvm/include/llvm/IR/Attributes.h @@ -360,6 +360,13 @@ public: /// Return true if the attribute exists in this set. bool hasAttribute(StringRef Kind) const; + /// Return true if the attribute exists in this set. + bool hasAttribute(Attribute A) const { + if (A.isStringAttribute()) + return hasAttribute(A.getKindAsString()); + return hasAttribute(A.getKindAsEnum()); + } + /// Return the attribute object. Attribute getAttribute(Attribute::AttrKind Kind) const; diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp index 721acb1..fc22630 100644 --- a/llvm/lib/Linker/IRMover.cpp +++ b/llvm/lib/Linker/IRMover.cpp @@ -529,7 +529,7 @@ class IRLinker { void linkNamedMDNodes(); /// Update attributes while linking. - void updateAttributes(GlobalValue &GV); + void updateAttributes(GlobalValue &DstGV, GlobalValue &SrcGV); public: IRLinker(Module &DstM, MDMapT &SharedMDs, @@ -641,7 +641,7 @@ Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) { if (ForIndirectSymbol || shouldLink(New, *SGV)) setError(linkGlobalValueBody(*New, *SGV)); - updateAttributes(*New); + updateAttributes(*New, *SGV); return New; } @@ -1533,7 +1533,10 @@ static std::string adjustInlineAsm(const std::string &InlineAsm, return InlineAsm; } -void IRLinker::updateAttributes(GlobalValue &GV) { +void IRLinker::updateAttributes(GlobalValue &DstGV, GlobalValue &SrcGV) { + auto *DstF = dyn_cast(&DstGV); + auto *SrcF = dyn_cast(&SrcGV); + /// Remove nocallback attribute while linking, because nocallback attribute /// indicates that the function is only allowed to jump back into caller's /// module only by a return or an exception. When modules are linked, this @@ -1548,16 +1551,53 @@ void IRLinker::updateAttributes(GlobalValue &GV) { /// participate in the LTO link, and thus ends up in the merged module /// containing its caller and callee, removing the attribute doesn't hurt as /// it has no effect on definitions in the same module. - if (auto *F = dyn_cast(&GV)) { - if (!F->isIntrinsic()) - F->removeFnAttr(llvm::Attribute::NoCallback); + if (DstF) { + if (!DstF->isIntrinsic()) + DstF->removeFnAttr(llvm::Attribute::NoCallback); // Remove nocallback attribute when it is on a call-site. - for (BasicBlock &BB : *F) + for (BasicBlock &BB : *DstF) for (Instruction &I : BB) if (CallBase *CI = dyn_cast(&I)) CI->removeFnAttr(Attribute::NoCallback); } + + // Fix compatibility of diverged function signatures. + // + // If `F` was a definition in its source module but is (already) a + // declaration in the destination module, then the signatures of the two + // functions may have diverged (even if they were originally idential) due to + // optimizations performed on the source module. + // + // See the test in `FunctionImport/attr_fixup_dae_noundef.ll` for an example + // based on how Dead Argument Elimination can remove `noundef` from function + // arguments. + // + // The code below handles _known_ incompatibilities. Others may exist. + if (DstF && SrcF && DstF->isDeclaration() && !SrcF->isDeclaration()) { + assert(DstF->arg_size() == SrcF->arg_size() && + "Dst and Src should have the same signature."); + + // Remove UB implying attributes present on `Dst`'s arguments but not + // `Src`'s. + AttributeMask UBImplyingAttributes = + AttributeFuncs::getUBImplyingAttributes(); + for (size_t ArgI = 0; ArgI < DstF->arg_size(); ArgI++) { + AttributeSet DstAttrs = DstF->getAttributes().getParamAttrs(ArgI); + AttributeSet SrcAttrs = SrcF->getAttributes().getParamAttrs(ArgI); + AttributeMask ToRemove; + + for (auto &Attr : DstAttrs) { + if (SrcAttrs.hasAttribute(Attr)) + continue; + + if (UBImplyingAttributes.contains(Attr)) + ToRemove.addAttribute(Attr); + } + + DstF->removeParamAttrs(ArgI, ToRemove); + } + } } Error IRLinker::run() { diff --git a/llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll b/llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll new file mode 100644 index 0000000..c8e3121 --- /dev/null +++ b/llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll @@ -0,0 +1,17 @@ +; This file contains the post-"deadargelim" IR. + +target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-unknown-linux-gnu" + +define void @outer(i32 noundef %arg) { + ; The parameter was originally `noundef %arg`, changed to `poison` by "deadargelim". + call void @inner(i32 poison) + ret void +} + +; %arg was originally `noundef`, removed by "deadargelim". +define void @inner(i32 %arg) #0 { + ret void +} + +attributes #0 = { noinline } diff --git a/llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll b/llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll new file mode 100644 index 0000000..fa43f98 --- /dev/null +++ b/llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll @@ -0,0 +1,34 @@ +; Test to ensure that if a definition is imported, already-present declarations +; are updated as necessary: Definitions from the same module may be optimized +; together. Thus care must be taken when importing only a subset of the +; definitions from a module (because other referenced definitions from that +; module may have been changed by the optimizer and may no longer match +; declarations already present in the module being imported into). + +; Generate bitcode and index, and run the function import. +; `Inputs/attr_fixup_dae_noundef.ll` contains the post-"Dead Argument Elimination" IR, which +; removed the `noundef` from `@inner`. +; RUN: opt -module-summary %p/Inputs/attr_fixup_dae_noundef.ll -o %t.inputs.attr_fixup_dae_noundef.bc +; RUN: opt -module-summary %s -o %t.main.bc +; RUN: llvm-lto -thinlto -o %t.summary %t.main.bc %t.inputs.attr_fixup_dae_noundef.bc +; RUN: opt -passes=function-import -summary-file %t.summary.thinlto.bc %t.main.bc -S 2>&1 \ +; RUN: | FileCheck %s + +target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-unknown-linux-gnu" + +define void @main() { + call void @outer(i32 noundef 1) + call void @inner(i32 noundef 1) + ret void +} + +; Because `@inner` is `noinline`, it should not get imported. However, the +; `noundef` should be removed. +; CHECK: declare void @inner(i32) +declare void @inner(i32 noundef) + +; `@outer` should get imported. +; CHECK: define available_externally void @outer(i32 noundef %arg) +; CHECK-NEXT: call void @inner(i32 poison) +declare void @outer(i32 noundef) -- 2.7.4