Reland "[pgo] Avoid introducing relocations by using private alias"

ilovepi · ilovepi · commit 3cfaea2d4999 · 2022-12-08T21:03:37.000Z
In many cases, we can use an alias to avoid a symbolic relocations, instead of using the public, interposable symbol. When the instrumented function is in a COMDAT, we can use a hidden alias, and still avoid references to discarded sections. This disables the failing runtime test on Windows, since the compiler options (-fPIC) are unsupported on that platform. Reviewed By: phosek Differential Revision: https://reviews.llvm.org/D137982
diff --git a/compiler-rt/test/profile/instrprof-discarded-comdat.cpp 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: 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 -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 <typename T> struct C {
+  void f(T x);
+  int g(T x) {
+    f(x);
+    return v;
+  }
+  int v;
+};
+
+template <typename T>
+#ifdef OBJECT_1
+__attribute__((weak))
+#else
+__attribute__((noinline))
+#endif
+void C<T>::f(T x) {
+  v += x;
+}
+
+#ifdef OBJECT_1
+int foo() {
+  C<int> c;
+  c.f(1);
+  return c.g(2);
+}
+#else
+int bar() {
+  C<int> 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
@@ -823,6 +823,36 @@ static inline bool shouldRecordFunctionAddr(Function *F) {
   return F->hasAddressTaken() || F->hasLinkOnceLinkage();
 }
 
+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. When the function has local linkage, we
+  // can use the symbol directly without introducing relocations.
+  if (Fn->isDeclarationForLinker() || Fn->hasLocalLinkage())
+    return ConstantExpr::getBitCast(Fn, Int8PtrTy);
+
+  // When possible use a private alias to avoid symbolic relocations.
+  auto *GA = GlobalAlias::create(GlobalValue::LinkageTypes::PrivateLinkage,
+                                 Fn->getName(), 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.
+  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())
@@ -1014,9 +1044,7 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfInstBase *Inc) {
   };
   auto *DataTy = StructType::get(Ctx, makeArrayRef(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
@@ -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 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.1 = linkonce_odr hidden alias void (), ptr @linkonceodr
+; ELF:   @weakodr.2 = weak_odr hidden alias void (), ptr @weakodr
+; ELF:   @weak.3 = weak hidden alias void (), ptr @weak
+; ELF:   @linkonce.4 = linkonce hidden alias void (), ptr @linkonce
diff --git a/llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll b/llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll
@@ -0,0 +1,86 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals
+; 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.1,
+; CHECK-NOT: @__profd_foo = private global {{.*}}, ptr @foo,
+; CHECK: @[[__PROFC_WEAK:[a-zA-Z0-9_$"\\.-]+]] = weak hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_WEAK:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -5028622335731970946, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_weak to i64), i64 ptrtoint (ptr @__profd_weak to i64)), ptr @weak.2, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_weak), align 8
+; CHECK: @[[__PROFC_LINKONCE:[a-zA-Z0-9_$"\\.-]+]] = linkonce hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_LINKONCE:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -121947654961992603, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_linkonce to i64), i64 ptrtoint (ptr @__profd_linkonce to i64)), ptr @linkonce.3, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_linkonce), align 8
+; CHECK: @[[__PROFC_WEAKODR:[a-zA-Z0-9_$"\\.-]+]] = weak_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_WEAKODR:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -4807837289933096997, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_weakodr to i64), i64 ptrtoint (ptr @__profd_weakodr to i64)), ptr @weakodr.4, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_weakodr), align 8
+; CHECK: @[[__PROFC_LINKONCEODR:[a-zA-Z0-9_$"\\.-]+]] = linkonce_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_LINKONCEODR:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 4214081367395809689, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_linkonceodr to i64), i64 ptrtoint (ptr @__profd_linkonceodr to i64)), ptr @linkonceodr.5, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_linkonceodr), align 8
+; CHECK: @[[__PROFC_AVAILABLE_EXTERNALLY_742261418966908927:[a-zA-Z0-9_$"\\.-]+]] = linkonce_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_AVAILABLE_EXTERNALLY_742261418966908927:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -8510055422695886042, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_available_externally.742261418966908927 to i64), i64 ptrtoint (ptr @__profd_available_externally.742261418966908927 to i64)), ptr null, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_available_externally.742261418966908927), align 8
+
+;; 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.1 = private alias i32 (i32), ptr @foo
+; CHECK: @[[WEAK_2:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @weak
+; CHECK: @[[LINKONCE_3:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @linkonce
+; CHECK: @[[WEAKODR_4:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @weakodr
+; CHECK: @[[LINKONCEODR_5:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @linkonceodr
+
+;; We should never generate an alias for available_externally functions
+; CHECK-NOT: @[[AVAILABLE_EXTERNALLY_6:[a-zA-Z0-9_$"\\.-]+]] = 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.742261418966908927, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_available_externally.742261418966908927, align 8
+; CHECK-NEXT:    ret void
+  ret void
+}
