Reland [pgo] Avoid introducing relocations by using private alias

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.

Previous versions of this patch allowed the compiler to name the
generated alias, but that would only be valid when the functions were
local. Since the alias may be used across TUs we use a more
deterministic naming convention, and add a ".local" suffix to the alias
name just as we do for relative vtables aliases.

https://reviews.llvm.org/rG20894a478da224bdd69c91a22a5175b28bc08ed9
removed an incorrect assertion on Mach-O which caused assertion failures in LLD.

We addressed the link errors under ThinLTO + PGO + CFI by being more
selective about which comdat functions can be given aliases.
Specifically, we now do not emit an alias in the case of a comdat
function with hidden visibility, since the alias would have the same
linkage and visibility, giving no benefit over using the symbol
directly. This also prevents LowerTypeTest from incorrectly updating the
dangling alias after GlobalOpt replaces uses, and introducing a
duplicate symbol.

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
new file mode 100644 (file)
index 0000000..feaa7c1
--- /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 <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
index c040920..e4a7eda 100644 (file)
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -823,6 +823,62 @@ 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())
@@ -1014,9 +1070,7 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfInstBase *Inc) {
   };
   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
index 9f5c0ee..765a775 100644 (file)
--- 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.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 (file)
index 0000000..806a2ab
--- /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
+}