diff --git a/compiler-rt/test/profile/instrprof-discarded-comdat.cpp b/compiler-rt/test/profile/instrprof-discarded-comdat.cpp new file mode 100644 --- /dev/null +++ b/compiler-rt/test/profile/instrprof-discarded-comdat.cpp @@ -0,0 +1,51 @@ +// Check that instrprof does not introduce references to discarded sections when +// using comdats. +// +// Occasionally, it is possible that the same function can be compiled in +// different TUs with slightly different linkages, e.g., due to different +// compiler options. However, if these are comdat functions, a single +// implementation will be chosen at link time. we want to ensure that the +// profiling data does not contain a reference to the discarded section. + +// UNSUPPORTED: target={{.*windows.*}} + +// RUN: mkdir -p %t.d +// RUN: %clangxx_pgogen -O2 -fPIC -ffunction-sections -fdata-sections -c %s -o %t.d/a1.o -DOBJECT_1 -mllvm -disable-preinline +// RUN: %clangxx_pgogen -O2 -fPIC -ffunction-sections -fdata-sections -c %s -o %t.d/a2.o +// RUN: %clangxx_pgogen -fPIC -shared -o %t.d/liba.so %t.d/a1.o %t.d/a2.o 2>&1 | FileCheck %s --allow-empty + +// Ensure that we don't get an error when linking +// CHECK-NOT: relocation refers to a discarded section: .text._ZN1CIiE1fEi + +template struct C { + void f(T x); + int g(T x) { + f(x); + return v; + } + int v; +}; + +template +#ifdef OBJECT_1 +__attribute__((weak)) +#else +__attribute__((noinline)) +#endif +void C::f(T x) { + v += x; +} + +#ifdef OBJECT_1 +int foo() { + C c; + c.f(1); + return c.g(2); +} +#else +int bar() { + C c; + c.f(3); + return c.g(4); +} +#endif diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp --- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp +++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp @@ -823,6 +823,72 @@ return F->hasAddressTaken() || F->hasLinkOnceLinkage(); } +static inline bool shouldUsePublicSymbol(Function *Fn) { + // It isn't legal to make an alias of this function at all + if (Fn->isDeclarationForLinker()) + return true; + + // Symbols with local linkage can just use the symbol directly without + // introducing relocations + if (Fn->hasLocalLinkage()) + return true; + + // PGO + ThinLTO + CFI cause duplicate symbols to be introduced due to some + // unfavorable interaction between the new alias and the alias renaming done + // in LowerTypeTests under ThinLTO. For comdat functions that would normally + // be deduplicated, but the renaming scheme ends up preventing renaming, since + // it creates unique names for each alias, resulting in duplicated symbols. In + // the future, we should update the CFI related passes to migrate these + // aliases to the same module as the jump-table they refer to will be defined. + if (Fn->hasMetadata(LLVMContext::MD_type)) + return true; + + // For comdat functions, an alias would need the same linkage as the original + // function and hidden visibility. There is no point in adding an alias with + // identical linkage an visibility to avoid introducing symbolic relocations. + if (Fn->hasComdat() && + (Fn->getVisibility() == GlobalValue::VisibilityTypes::HiddenVisibility)) + return true; + + // its OK to use an alias + return false; +} + +static inline Constant *getFuncAddrForProfData(Function *Fn) { + auto *Int8PtrTy = Type::getInt8PtrTy(Fn->getContext()); + // Store a nullptr in __llvm_profd, if we shouldn't use a real address + if (!shouldRecordFunctionAddr(Fn)) + return ConstantPointerNull::get(Int8PtrTy); + + // If we can't use an alias, we must use the public symbol, even though this + // may require a symbolic relocation. + if (shouldUsePublicSymbol(Fn)) + return ConstantExpr::getBitCast(Fn, Int8PtrTy); + + // When possible use a private alias to avoid symbolic relocations. + auto *GA = GlobalAlias::create(GlobalValue::LinkageTypes::PrivateLinkage, + Fn->getName() + ".local", Fn); + + // When the instrumented function is a COMDAT function, we cannot use a + // private alias. If we did, we would create reference to a local label in + // this function's section. If this version of the function isn't selected by + // the linker, then the metadata would introduce a reference to a discarded + // section. So, for COMDAT functions, we need to adjust the linkage of the + // alias. Using hidden visibility avoids a dynamic relocation and an entry in + // the dynamic symbol table. + // + // Note that this handles COMDAT functions with visibility other than Hidden, + // since that case is covered in shouldUsePublicSymbol() + if (Fn->hasComdat()) { + GA->setLinkage(Fn->getLinkage()); + GA->setVisibility(GlobalValue::VisibilityTypes::HiddenVisibility); + } + + // appendToCompilerUsed(*Fn->getParent(), {GA}); + + return ConstantExpr::getBitCast(GA, Int8PtrTy); +} + static bool needsRuntimeRegistrationOfSectionRange(const Triple &TT) { // Don't do this for Darwin. compiler-rt uses linker magic. if (TT.isOSDarwin()) @@ -1014,9 +1080,7 @@ }; auto *DataTy = StructType::get(Ctx, ArrayRef(DataTypes)); - Constant *FunctionAddr = shouldRecordFunctionAddr(Fn) - ? ConstantExpr::getBitCast(Fn, Int8PtrTy) - : ConstantPointerNull::get(Int8PtrTy); + Constant *FunctionAddr = getFuncAddrForProfData(Fn); Constant *Int16ArrayVals[IPVK_Last + 1]; for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind) diff --git a/llvm/test/Transforms/PGOProfile/comdat.ll b/llvm/test/Transforms/PGOProfile/comdat.ll --- a/llvm/test/Transforms/PGOProfile/comdat.ll +++ b/llvm/test/Transforms/PGOProfile/comdat.ll @@ -4,6 +4,8 @@ $linkonceodr = comdat any $weakodr = comdat any +$weak = comdat any +$linkonce = comdat any ;; profc/profd have hash suffixes. This definition doesn't have value profiling, ;; so definitions with the same name in other modules must have the same CFG and @@ -27,3 +29,32 @@ define weak_odr void @weakodr() comdat { ret void } + +;; weak in a comdat is not renamed. There is no guarantee that definitions in +;; other modules don't have value profiling. profd should be conservatively +;; non-private to prevent a caller from referencing a non-prevailing profd, +;; causing a linker error. +; ELF: @__profc_weak = weak hidden global {{.*}} comdat, align 8 +; ELF: @__profd_weak = weak hidden global {{.*}} comdat($__profc_weak), align 8 +; COFF: @__profc_weak = weak hidden global {{.*}} comdat, align 8 +; COFF: @__profd_weak = weak hidden global {{.*}} comdat, align 8 +define weak void @weak() comdat { + ret void +} + +;; profc/profd have hash suffixes. This definition doesn't have value profiling, +;; so definitions with the same name in other modules must have the same CFG and +;; cannot have value profiling, either. profd can be made private for ELF. +; ELF: @__profc_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8 +; ELF: @__profd_linkonce.[[#]] = private global {{.*}} comdat($__profc_linkonce.[[#]]), align 8 +; COFF: @__profc_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8 +; COFF: @__profd_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8 +define linkonce void @linkonce() comdat { + ret void +} + +; Check that comdat aliases are hidden for all linkage types +; ELF: @linkonceodr.local = linkonce_odr hidden alias void (), ptr @linkonceodr +; ELF: @weakodr.local = weak_odr hidden alias void (), ptr @weakodr +; ELF: @weak.local = weak hidden alias void (), ptr @weak +; ELF: @linkonce.local = linkonce hidden alias void (), ptr @linkonce diff --git a/llvm/test/Transforms/PGOProfile/profdata_priv_alias.ll b/llvm/test/Transforms/PGOProfile/profdata_priv_alias.ll new file mode 100644 --- /dev/null +++ b/llvm/test/Transforms/PGOProfile/profdata_priv_alias.ll @@ -0,0 +1,84 @@ +; RUN: opt -S -passes=pgo-instr-gen,instrprof < %s | 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" + +;; Test that we use private aliases to reference function addresses inside profile data +; CHECK: @__profd_foo = private global {{.*}} ptr @foo.local +; CHECK-NOT: @__profd_foo = private global {{.*}} ptr @foo + +; CHECK: @__profd_weak = private global {{.*}} ptr @weak.local +; CHECK: @__profd_linkonce = private global {{.*}} ptr @linkonce.local +; CHECK: @__profd_weakodr = private global {{.*}} ptr @weakodr.local +; CHECK: @__profd_linkonceodr = private global {{.*}} ptr @linkonceodr.local + +; available_externally shouldn't have an alias, so make sure it doesn't appear here +; CHECK: @__profc_available_externally.[[HASH:[#0-9]+]] +; CHECK-NOT: @__profd_available_externally.[[HASH]] = {{.*}}ptr @available_externally.[[HASH]].local + +;; Ensure when not instrumenting a non-comdat function, then if we generate an +;; alias, then it is private. We check comdat versions in comdat.ll +; CHECK: @foo.local = private alias i32 (i32), ptr @foo +; CHECK: @weak.local = private alias void (), ptr @weak +; CHECK: @linkonce.local = private alias void (), ptr @linkonce +; CHECK: @weakodr.local = private alias void (), ptr @weakodr +; CHECK: @linkonceodr.local = private alias void (), ptr @linkonceodr + +;; We should never generate an alias for available_externally functions +; CHECK-NOT: @available_externally{{.*}} = private alias void (), ptr @available_externally + +define i32 @foo(i32 %0) { +; CHECK-LABEL: @foo( +; CHECK-NEXT: entry: +; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_foo, align 8 +; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1 +; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_foo, align 8 +; CHECK-NEXT: ret i32 0 +entry: + ret i32 0 +} + +define weak void @weak() { +; CHECK-LABEL: @weak( +; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_weak, align 8 +; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1 +; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_weak, align 8 +; CHECK-NEXT: ret void + ret void +} + +define linkonce void @linkonce() { +; CHECK-LABEL: @linkonce( +; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_linkonce, align 8 +; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1 +; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_linkonce, align 8 +; CHECK-NEXT: ret void + ret void +} + +define weak_odr void @weakodr() { +; CHECK-LABEL: @weakodr( +; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_weakodr, align 8 +; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1 +; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_weakodr, align 8 +; CHECK-NEXT: ret void + ret void +} + +define linkonce_odr void @linkonceodr() { +; CHECK-LABEL: @linkonceodr( +; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_linkonceodr, align 8 +; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1 +; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_linkonceodr, align 8 +; CHECK-NEXT: ret void + ret void +} + +define available_externally void @available_externally(){ +; CHECK-LABEL: @available_externally( +; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_available_externally.[[HASH]], align 8 +; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1 +; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_available_externally.[[HASH]], align 8 +; CHECK-NEXT: ret void + ret void +}