[PGO][AIX] Improve dummy var retention and allow -bcdtors:csect linking.

[w2yehia]

Summary

This patch subsumes https://reviews.llvm.org/D134253.
It addresses a couple of problems:

1. The problem addressed by D134253; namely, incorrect behavior when linking a shared library that has PGO into an executable with PGO (see LIT test below). The symbol keep was seen as duplicate, and one copy was thrown away; we want both to stay as keep was creating a reference to other internal symbols that we want the linker to keep.
2. The keep symbol was visible to the user and would conflict with a user symbol with the same name.
3. When compiling and linking with -ffunction-sections -Wl,-bcdtors:mbr, the PGO runtime was not initialized.
4. When compiling and linking with -ffunction-sections -Wl,-bcdtors:csect, the PGO runtime was not initialized. The constructor function that does the runtime registration must be associated to a live variable, otherwise under the semantics of -Wl,-bcdtors:csect it would get GC'ed by the linker.

The following changes are made:

1. Convert the library-internal function keep into an array (__llvm_profile_keep) that references the dummy variables that we want to retain.
2. associate the liveness of __llvm_profile_keep with the runtime registration variable since we want that array to be live when the profile write out code is.
3. Preserve the runtime hook variable (__llvm_profile_runtime) explicitly on AIX, same as on Linux.
4. On AIX only, perform the runtime registration from a constructor of a variable that is guaranteed to be live; so we chose the runtime hook as that variable. This is needed to allow linking PGO rt with -Wl,-bcdtors:csect.
5. Refactor the runtime registration logic to accomodate for AIX.

[hubert.reinterpretcast]

compiler-rt/lib/profile/InstrProfilingRuntime.cpp

37

Should follow the example of __llvm_profile_runtime, etc. in terms of using __llvm_profile_ as the prefix.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/AIX.cpp

356

359

Don't need to use MakeArgString to persist a string literal.

compiler-rt/lib/profile/InstrProfilingRuntime.cpp

17–18

The class still needs to be in an unnamed namespace for the constructor to not clash with a user-defined class with the same name.

[w2yehia]

compiler-rt/lib/profile/InstrProfilingRuntime.cpp

17–18

yes, i had a follow up patch that I was testing on both linux and AIX that gets rid of the class all together:

-namespace {
-
-class RegisterRuntime {
-public:
-  RegisterRuntime() {
-    __llvm_profile_initialize();
-  }
-};
-
-RegisterRuntime Registration;
-
+static int RegisterRuntime() {
+  __llvm_profile_initialize();
+  return 0;
 }
+
+#ifdef _AIX
+COMPILER_RT_VISIBILITY
+#else
+static
+#endif
+int __llvm_register_runtime = RegisterRuntime();

[w2yehia]

Improve the current patch and integrate D134253 into it.

[w2yehia]

Don't move around code in InstrProfilingPlatformLinux.c

[w2yehia]

@phosek @MaskRay If you can kindly review this patch as it affects non-AIX platforms with the changes in compiler-rt/lib/profile/InstrProfilingRuntime.cpp. I'm hoping the change are like for like but I cannot be sure for all platforms.

[hubert.reinterpretcast]

This LGTM. Size of Registration changes from 1 byte to (likely) 4 bytes on non-AIX platforms and now incurs an extra memory write, but that should be insignificant.

[phosek]

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

265–267

Does AIX linker support the retain attribute? That would be a cleaner solution.

compiler-rt/lib/profile/InstrProfilingRuntime.cpp

13–18

Is it possible to reuse the runtime hook variable on other platforms as well? I'd really like to avoid introducing another variable.

[phosek]

I wasn't included as a reviewer on D124857 and I missed that change so couldn't comment there, but I'm not a fan of including the AIX support in InstrProfilingPlatformLinux.c. AIX is neither Linux nor ELF-based and big chunks of that file are now #ifdefed out making it harder to comprehend which part is used where. I'd prefer introducing InstrProfilingPlatformAIX.c, moving the AIX-specific logic there, and then figuring out how to possibly share common parts between InstrProfilingPlatformLinux.c and InstrProfilingPlatformAIX.c, for example by moving them to an .inc file.

[w2yehia]

I wasn't included as a reviewer on D124857 and I missed that change so couldn't comment there, but I'm not a fan of including the AIX support in InstrProfilingPlatformLinux.c. AIX is neither Linux nor ELF-based and big chunks of that file are now #ifdefed out making it harder to comprehend which part is used where. I'd prefer introducing InstrProfilingPlatformAIX.c, moving the AIX-specific logic there, and then figuring out how to possibly share common parts between InstrProfilingPlatformLinux.c and InstrProfilingPlatformAIX.c, for example by moving them to an .inc file.

Splitting was considered but because AIX reuses the entirety of that file (except two #include's) and adds some AIX-only stuff it seemed better not to duplicate the common code. Doing a .inc file was not considered. The .inc file will have to contain the entire InstrProfilingPlatformLinux.c content.
Let me know if you want me to post a patch for the .inc. Thanks

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

265–267

thanks for pointing out. Unfortunately the retain attribute is not supported by the AIX linker. It could be implemented on the compiler side, but as of now it's not supported by the build compiler on AIX.

compiler-rt/lib/profile/InstrProfilingRuntime.cpp

13–18

I also wanted to do that but didn't in fear of breaking others. I can do the change and see what breaks?

clang/lib/Driver/ToolChains/AIX.h

public:
 
   void AddClangCXXStdlibIncludeArgs(
       const llvm::opt::ArgList &DriverArgs,
       llvm::opt::ArgStringList &CC1Args) const override;
 
   void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) const override;
 
+  void addProfileRTLibs(const llvm::opt::ArgList &Args,
+                        llvm::opt::ArgStringList &CmdArgs) const override;
+
   CXXStdlibType GetDefaultCXXStdlibType() const override;
 
   RuntimeLibType GetDefaultRuntimeLibType() const override;
 
   // Set default DWARF version to 3 for now as latest AIX OS supports version 3.
   unsigned GetDefaultDwarfVersion() const override { return 3; }
 
   llvm::DebuggerKind getDefaultDebuggerTuning() const override {

clang/lib/Driver/ToolChains/AIX.cpp

 //===--- AIX.cpp - AIX ToolChain Implementations ----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AIX.h"
 #include "Arch/PPC.h"
 #include "CommonArgs.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/SanitizerArgs.h"
 #include "llvm/Option/ArgList.h"
+#include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/Path.h"
 
 using AIX = clang::driver::toolchains::AIX;
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang::driver::toolchains;
 
 using namespace llvm::opt;
if (Args.hasArg(options::OPT_fexperimental_library))
       CmdArgs.push_back("-lc++experimental");
     CmdArgs.push_back("-lc++abi");
     return;
   }
 
   llvm_unreachable("Unexpected C++ library type; only libc++ is supported.");
 }
 
+void AIX::addProfileRTLibs(const llvm::opt::ArgList &Args,
+                           llvm::opt::ArgStringList &CmdArgs) const {
+  // Add linker option -u__llvm_profile_runtime to cause runtime
+  // initialization to occur.
+  if (needsProfileRT(Args))
+    CmdArgs.push_back(Args.MakeArgString(
+        Twine("-u", llvm::getInstrProfRuntimeHookVarName())));
+  ToolChain::addProfileRTLibs(Args, CmdArgs);
+}
+
 ToolChain::CXXStdlibType AIX::GetDefaultCXXStdlibType() const {
   return ToolChain::CST_Libcxx;
 }
 
 ToolChain::RuntimeLibType AIX::GetDefaultRuntimeLibType() const {
   return ToolChain::RLT_CompilerRT;
 }
 
 auto AIX::buildAssembler() const -> Tool * { return new aix::Assembler(*this); }
 
 auto AIX::buildLinker() const -> Tool * { return new aix::Linker(*this); }

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

 // reference symbols from the profile library (for example when no files were
 // compiled with -fprofile-generate). That's because these symbols are kept
 // alive through references in constructor functions that are always live in the
 // default linking model on AIX (-bcdtors:all). The __start_SECNAME and
 // __stop_SECNAME symbols are only resolved by the linker when the SECNAME
 // section exists. So for the scenario where the user objects have no such
 // section (i.e. when they are compiled with -fno-profile-generate), we always
 // define these zero length variables in each of the above 4 sections.
-COMPILER_RT_VISIBILITY int dummy_cnts[0] COMPILER_RT_SECTION(
+static int dummy_cnts[0] COMPILER_RT_SECTION(
     COMPILER_RT_SEG INSTR_PROF_CNTS_SECT_NAME);
-COMPILER_RT_VISIBILITY int dummy_data[0] COMPILER_RT_SECTION(
+static int dummy_data[0] COMPILER_RT_SECTION(
     COMPILER_RT_SEG INSTR_PROF_DATA_SECT_NAME);
-COMPILER_RT_VISIBILITY const int dummy_name[0] COMPILER_RT_SECTION(
+static const int dummy_name[0] COMPILER_RT_SECTION(
     COMPILER_RT_SEG INSTR_PROF_NAME_SECT_NAME);
-COMPILER_RT_VISIBILITY int dummy_vnds[0] COMPILER_RT_SECTION(
+static int dummy_vnds[0] COMPILER_RT_SECTION(
     COMPILER_RT_SEG INSTR_PROF_VNODES_SECT_NAME);
 
-// Create a fake reference to avoid GC'ing of the dummy variables by the linker.
-// Ideally, we create a ".ref" of each variable inside the function
-// __llvm_profile_begin_counters(), but there's no source level construct
-// that allows us to generate that.
-__attribute__((destructor)) void keep() {
-  int volatile use = &dummy_cnts < &dummy_data && &dummy_name < &dummy_vnds;
-  (void)use;
-}
+// To avoid GC'ing of the dummy variables by the linker, reference them in an
+// array and reference the array in the runtime registration code
+// (InstrProfilingRuntime.cpp)
+COMPILER_RT_VISIBILITY
+void *__llvm_profile_keep[] = {(void *)&dummy_cnts, (void *)&dummy_data,
+                               (void *)&dummy_name, (void *)&dummy_vnds};
 #endif
 
 #endif

compiler-rt/lib/profile/InstrProfilingRuntime.cpp

 //===- InstrProfilingRuntime.cpp - PGO runtime initialization -------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 extern "C" {
 
 #include "InstrProfiling.h"
 
-/* int __llvm_profile_runtime  */
-COMPILER_RT_VISIBILITY int INSTR_PROF_PROFILE_RUNTIME_VAR;
-}
-
-namespace {
-
-class RegisterRuntime {
-public:
-  RegisterRuntime() {
-    __llvm_profile_initialize();
-  }
-};
+static int RegisterRuntime() {
+  return 0;
 
-RegisterRuntime Registration;
+#ifndef _AIX
+/* int __llvm_profile_runtime  */
+COMPILER_RT_VISIBILITY int INSTR_PROF_PROFILE_RUNTIME_VAR;
 
+static int Registration = RegisterRuntime();
+#else
+extern COMPILER_RT_VISIBILITY void *__llvm_profile_keep[];
+/* On AIX, when linking with -bcdtors:csect, the variable whose constructor does
+ * the registration needs to be explicitly kept, hence we reuse the runtime hook
+ * variable to do the registration since it'll be kept via the -u linker flag.
+ * Create a volatile reference to __llvm_profile_keep to keep the array alive.*/
+COMPILER_RT_VISIBILITY int INSTR_PROF_PROFILE_RUNTIME_VAR =
+    ((void)*(void *volatile *)__llvm_profile_keep, RegisterRuntime());
+#endif
 }

compiler-rt/test/profile/AIX/lit.local.cfg.py

+def getRoot(config):
+  if not config.parent:
+    return config
+  return getRoot(config.parent)
+
+root = getRoot(config)
+
+if root.host_os not in ['AIX']:
+  config.unsupported = True

compiler-rt/test/profile/AIX/shared-bexpall-pgo.c

+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_pgogen foo.c -c -o foo.o
+// RUN: %clang_pgogen -shared foo.o -o libfoo.so -bexpall
+// RUN: %clang_pgogen -L%t user.c libfoo.so -o user1
+// RUN: ./user1
+
+//--- foo.c
+void foo() {}
+
+//--- user.c
+void foo();
+int main() { foo(); }

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

void InstrProfiling::emitRegistration() {
   }
 
   IRB.CreateRetVoid();
 }
 
 bool InstrProfiling::emitRuntimeHook() {
   // We expect the linker to be invoked with -u<hook_var> flag for Linux
   // in which case there is no need to emit the external variable.
-  if (TT.isOSLinux())
+  if (TT.isOSLinux() || TT.isOSAIX())
     return false;
 
   // If the module's provided its own runtime, we don't need to do anything.
   if (M->getGlobalVariable(getInstrProfRuntimeHookVarName()))
     return false;
 
   // Declare an external variable that will pull in the runtime initialization.
   auto *Int32Ty = Type::getInt32Ty(M->getContext());

llvm/test/Instrumentation/InstrProfiling/no-counters.ll

 ;; Emit the runtime hook even if there are no counter increments.
 
 ; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -passes=instrprof -S | FileCheck %s -check-prefixes=ALL,DARWIN
 ; RUN: opt < %s -mtriple=x86_64-linux-unknown -passes=instrprof -S | FileCheck %s -check-prefixes=ALL,LINUX
-; RUN: opt < %s -mtriple=powerpc64-ibm-aix-xcoff -passes=instrprof -S | FileCheck %s -check-prefixes=ALL,DARWIN
+; RUN: opt < %s -mtriple=powerpc64-ibm-aix-xcoff -passes=instrprof -S | FileCheck %s -check-prefixes=ALL,LINUX
 ; ALL-NOT: @__profc
 ; ALL-NOT: @__profd
 ; DARWIN: @__llvm_profile_runtime
 ; LINUX-NOT: @__llvm_profile_runtime
 
 define void @foo() {
   ret void
 }

llvm/test/Instrumentation/InstrProfiling/profiling.ll

 ;; Test runtime symbols and various linkages.
 
 ; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -passes=instrprof -S | FileCheck %s --check-prefixes=MACHO
 ; RUN: opt < %s -mtriple=x86_64 -passes=instrprof -S | FileCheck %s --check-prefix=ELF_GENERIC
 ; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefixes=ELF,ELFRT
 ; RUN: opt < %s -mtriple=x86_64-unknown-fuchsia -passes=instrprof -S | FileCheck %s --check-prefixes=ELF,ELFRT
 ; RUN: opt < %s -mtriple=x86_64-scei-ps4 -passes=instrprof -S | FileCheck %s --check-prefixes=ELF,PS
 ; RUN: opt < %s -mtriple=x86_64-sie-ps5 -passes=instrprof -S | FileCheck %s --check-prefixes=ELF,PS
 ; RUN: opt < %s  -mtriple=x86_64-pc-win32-coff -passes=instrprof -S | FileCheck %s --check-prefixes=COFF
 ; RUN: opt < %s -mtriple=powerpc64-ibm-aix-xcoff -passes=instrprof -S | FileCheck %s --check-prefixes=XCOFF
 ; RUN: opt < %s -mtriple=x86_64-pc-freebsd13 -passes=instrprof -S | FileCheck %s --check-prefixes=ELF
 
 ; MACHO: @__llvm_profile_runtime = external hidden global i32
 ; ELF_GENERIC: @__llvm_profile_runtime = external hidden global i32
 ; ELF-NOT: @__llvm_profile_runtime = external global i32
-; XCOFF: @__llvm_profile_runtime = external hidden global i32
+; XCOFF-NOT: @__llvm_profile_runtime = external hidden global i32
 ; COFF: @__llvm_profile_runtime = external hidden global i32
 
 ; ELF: $__profc_foo = comdat nodeduplicate
 ; ELF: $__profc_foo_weak = comdat nodeduplicate
 ; ELF: $"__profc_linkage.ll:foo_internal" = comdat nodeduplicate
 ; ELF: $__profc_foo_inline = comdat nodeduplicate
 ; ELF: $__profc_foo_extern = comdat any
 

 }
 
 declare void @llvm.instrprof.increment(ptr, i64, i32, i32)
 
 ; ELF:         @llvm.compiler.used = appending global {{.*}} [{{.*}}ptr @__profd_foo, ptr @__profd_foo_weak, ptr @"__profd_linkage.ll:foo_internal", ptr @__profd_foo_inline, ptr @__profd_foo_extern{{.*}}]
 ; ELF_GENERIC: @llvm.compiler.used = appending global [6 x ptr] [ptr @__llvm_profile_runtime, ptr @__profd_foo, ptr @__profd_foo_weak, ptr @"__profd_linkage.ll:foo_internal", ptr @__profd_foo_inline, ptr @__profd_foo_extern]
 ; MACHO:       @llvm.compiler.used = appending global [6 x ptr] [ptr @__llvm_profile_runtime_user, ptr @__profd_foo, {{.*}}
 ; COFF:        @llvm.compiler.used = appending global [6 x ptr] [ptr @__llvm_profile_runtime_user, ptr @__profd_foo, ptr @__profd_foo_weak, ptr @"__profd_linkage.ll:foo_internal", ptr @__profd_foo_inline, ptr @__profd_foo_extern]
-; XCOFF:       @llvm.used = appending global [7 x ptr] [ptr @__llvm_profile_runtime_user, ptr @__profd_foo, ptr @__profd_foo_weak, ptr @"__profd_linkage.ll:foo_internal", ptr @__profd_foo_inline, ptr @__profd_foo_extern, ptr @__llvm_prf_nm]
+; XCOFF:       @llvm.used = appending global [6 x ptr] [ptr @__profd_foo, ptr @__profd_foo_weak, ptr @"__profd_linkage.ll:foo_internal", ptr @__profd_foo_inline, ptr @__profd_foo_extern, ptr @__llvm_prf_nm]
 
 ; MACHO: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
 ; MACHO:   %[[REG:.*]] = load i32, ptr @__llvm_profile_runtime
 ; MACHO:   ret i32 %[[REG]]
 ; MACHO: }
 ; COFF: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} comdat {
 ; ELFRT-NOT: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
 ; ELFRT-NOT:   %[[REG:.*]] = load i32, ptr @__llvm_profile_runtime
 ; PS: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
 ; PS:   %[[REG:.*]] = load i32, ptr @__llvm_profile_runtime
-; XCOFF: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
-; XCOFF:   %[[REG:.*]] = load i32, ptr @__llvm_profile_runtime
-; XCOFF:   ret i32 %[[REG]]
-; XCOFF: }
+; XCOFF-NOT: define .* __llvm_profile_runtime_user
 
 ; ELF_GENERIC:      define internal void @__llvm_profile_register_functions() unnamed_addr {
 ; ELF_GENERIC-NEXT:   call void @__llvm_profile_register_function(ptr @__llvm_profile_runtime)
 ; ELF_GENERIC-NEXT:   call void @__llvm_profile_register_function(ptr @__profd_foo)
 ; ELF_GENERIC-NEXT:   call void @__llvm_profile_register_function(ptr @__profd_foo_weak)
 ; ELF_GENERIC:        call void @__llvm_profile_register_names_function(ptr @__llvm_prf_nm
 ; ELF_GENERIC-NEXT:   ret void
 ; ELF_GENERIC-NEXT: }
 
 ; XCOFF-NOT:  internal void @__llvm_profile_register_functions()