From 1f3f3c0ea724335e7eb518a8fe30990c9245875b Mon Sep 17 00:00:00 2001 From: Arthur Eubanks Date: Thu, 19 Jan 2023 10:19:27 -0800 Subject: [PATCH] Revert "Reland [pgo] Avoid introducing relocations by using private alias" This reverts commit da5a8d14b8cc6cea16ee0929413c0672b47c93d9. Causes more duplicate symbol errors, see https://bugs.chromium.org/p/chromium/issues/detail?id=1408161. --- .../test/profile/instrprof-discarded-comdat.cpp | 51 ------------- .../Transforms/Instrumentation/InstrProfiling.cpp | 60 +--------------- llvm/test/Transforms/PGOProfile/comdat.ll | 31 -------- .../Transforms/PGOProfile/profdata_priv_alias.ll | 84 ---------------------- 4 files changed, 3 insertions(+), 223 deletions(-) delete mode 100644 compiler-rt/test/profile/instrprof-discarded-comdat.cpp delete mode 100644 llvm/test/Transforms/PGOProfile/profdata_priv_alias.ll diff --git a/compiler-rt/test/profile/instrprof-discarded-comdat.cpp b/compiler-rt/test/profile/instrprof-discarded-comdat.cpp deleted file mode 100644 index feaa7c1..0000000 --- a/compiler-rt/test/profile/instrprof-discarded-comdat.cpp +++ /dev/null @@ -1,51 +0,0 @@ -// 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 index e4a7eda..c040920 100644 --- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp +++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp @@ -823,62 +823,6 @@ static inline bool shouldRecordFunctionAddr(Function *F) { 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; - - // For comdat functions, an alias would need the same linkage as the original - // function and hidden visibility, and there is not point in adding an alias - // with identical linkage an visibility to avoid introducing relocations. - // This caused duplicate symbols to be introduced under the combination of - // PGO + ThinLTO + CFI, due to some unfavorable interaction between the new - // alias, and alias related transforms in GlobalOpt and LowerTypeTests. - 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); - } - 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()) @@ -1070,7 +1014,9 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfInstBase *Inc) { }; auto *DataTy = StructType::get(Ctx, ArrayRef(DataTypes)); - Constant *FunctionAddr = getFuncAddrForProfData(Fn); + Constant *FunctionAddr = shouldRecordFunctionAddr(Fn) + ? ConstantExpr::getBitCast(Fn, Int8PtrTy) + : ConstantPointerNull::get(Int8PtrTy); 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 index 765a775..9f5c0ee 100644 --- a/llvm/test/Transforms/PGOProfile/comdat.ll +++ b/llvm/test/Transforms/PGOProfile/comdat.ll @@ -4,8 +4,6 @@ $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 @@ -29,32 +27,3 @@ define linkonce_odr void @linkonceodr() comdat { 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 deleted file mode 100644 index 806a2ab..0000000 --- a/llvm/test/Transforms/PGOProfile/profdata_priv_alias.ll +++ /dev/null @@ -1,84 +0,0 @@ -; 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 -} -- 2.7.4