[AIX][PGO] Add missing visibility attribute on an internal function.

[w2yehia]

Summary

Convert the library-internal function keep into a local array that references the dummy variables that we want to retain.
This achieves two things:

1. The function keep is no longer an external, which was causing problems when a shared library, that has PGO, is linked into an executable with PGO. The symbol keep is seen as duplicate, and one copy is thrown away; we want both to stay as this function is creating a reference to other internal symbols that we want the linker to keep.
2. Associate the liveness of the dummy variables with that of the __start_/__stop_ symbols of the 4 named sections used in PGO.

Since the fake volatile reference causes an unnecessary load inside function __llvm_profile_end_vnodes, I picked that function because it gets called the least (twice) number of times, during a train run, compared to the other 3 functions (which get called 3 times) based on running SPEC 2017 benchmarks.

Reproduced on AIX:

$ cat build.sh 
#!/bin/bash

cat <<EOF > foo.c
void foo() {}
EOF
cat << EOF > user.c
void foo();
int main() { foo(); }
EOF
touch empty.exp

FLAGS=-fprofile-generate
clang foo.c $FLAGS -c
clang -shared -bE:empty.exp foo.o $FLAGS -o libfoo.so -bexpall
clang user.c libfoo.so $FLAGS -o user1 
LIBPATH=$PWD ./user1

$ ./build.sh 
ld: 0711-224 WARNING: Duplicate symbol: keep
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
./build.sh: line 16: 20382302 Illegal instruction     (core dumped) LIBPATH=$PWD ./user1

[w2yehia]

add testcase

[hubert.reinterpretcast]

compiler-rt/test/profile/AIX/sharedlib.c

1 ↗

(On Diff #461460)

Are we sure all build environments for compiler-rt necessarily has split-file available?

5 ↗

(On Diff #461460)

-bexpall means that -bE:empty.emp is not needed.

6 ↗

(On Diff #461460)

The $FLAGS should not be present here (no similar usage in nearby tests).

6 ↗

(On Diff #461460)

If -L%t is added, the LIBPATH on the next line won't be needed.

[w2yehia]

compiler-rt/test/profile/AIX/sharedlib.c

1 ↗

(On Diff #461460)

this testcase will only run on AIX (see above file). Other platforms (Windows, Darwin, Linux) do the same for PGO runtime tests.
llvm/test/Linker/aix-duplicate-globals.ll is an existing test that uses split-file and runs on AIX fine.

5 ↗

(On Diff #461460)

fixed. Thanks

6 ↗

(On Diff #461460)

I forgot to replace FLAGS with -fprofile-generate; fixed.
And added -L%t and removed LIBPATH.

Thanks alot!!

[w2yehia]

Address code review comments.

[hubert.reinterpretcast]

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

266

Seems if there is a user function called keep, then the user function gets called by the __attribute__((destructor)) semantics instead.

Program:

int printf(const char *, ...);
void keep(void) { printf("Hello, world!\n"); }
int main(void) { keep(); }

Output:

$ ./a.out
Hello, world!
Hello, world!
Return:  0x00:0   Tue Sep 20 13:33:25 2022 EDT

Maybe we rename this and also have the Clang driver modified to add -u __llvm_profile_aix_gc_retain to the linker invocation when linking in libclang_rt.profile? We can probably change this from a function into an array of int * as well.

[w2yehia]

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

266

collisions with user symbols is really bad... Oops. Definitely will rename it.

[w2yehia]

Redo the variable retainment mechanism based on offline discussion with @hubert.reinterpretcast that followed his code review comments.

[w2yehia]

@hubert.reinterpretcast do you think we shud add the "keep()" testcase as a LIT?

[hubert.reinterpretcast]

In D134253#3827696, @w2yehia wrote:

@hubert.reinterpretcast do you think we shud add the "keep()" testcase as a LIT?

I don't think we should. It's specific to the previous implementation (not a generic test that guards against a common failure mode).

[hubert.reinterpretcast]

LGTM (with note); thanks!

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

119–120

Just noting that I am not familiar with whether this function is always referenced whenever any one of the __start_* or __stop_* symbols would be.

[w2yehia]

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

119–120

Currently, the lprofGetLoadModuleSignature() function references all eight __llvm_profile_[begin|end]_[counters|data|names|vnodes] functions. And lprofGetLoadModuleSignature can be traced up in the call graph all the way to __llvm_profile_initialize(). So, if one of the start/stop symbols is alive, all are.

Ofcourse, this liveness relationship is not by design I think. However, if this relationship is ever broken then simple -fprofile-generate link steps, such as compiler-rt/test/profile/instrprof-basic.c, will fail (in the AIX buildbot).

[w2yehia]

replace %clang -fprofile-generate with %clang_pgogen in the testcase.

[w2yehia]

@MaskRay please let me know if you have any comments. This affect AIX only, but might reduce readability with the introduction of the #if's

[hubert.reinterpretcast]

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

121

In https://reviews.llvm.org/D136192, we are going to keep a symbol (and, in turn, keep __llvm_profile_initialize) anyway.
Maybe we can merge the two patches and keep one symbol (the array) and have the array also store the address of the registration object.

[hubert.reinterpretcast]

@w2yehia, this can be closed now (the other patch that also covers what this one is solving has landed)?

[w2yehia]

Fixed by D136192

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

 extern ValueProfNode PROF_VNODES_START COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
 extern ValueProfNode PROF_VNODES_STOP COMPILER_RT_VISIBILITY COMPILER_RT_WEAK;
 
+#ifdef _AIX
+// Empty stubs to allow linking object files using the registration-based scheme
+COMPILER_RT_VISIBILITY
+void __llvm_profile_register_function(void *Data_) {}
+
+COMPILER_RT_VISIBILITY
+void __llvm_profile_register_names_function(void *NamesStart,
+                                            uint64_t NamesSize) {}
+
+// The __start_SECNAME and __stop_SECNAME symbols (for SECNAME \in
+// {"__llvm_prf_cnts", "__llvm_prf_data", "__llvm_prf_name", "__llvm_prf_vnds"})
+// are always live when linking on AIX, regardless if the .o's being linked
+// 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.
+static int dummy_cnts[0] COMPILER_RT_SECTION(
+    COMPILER_RT_SEG INSTR_PROF_CNTS_SECT_NAME);
+static int dummy_data[0] COMPILER_RT_SECTION(
+    COMPILER_RT_SEG INSTR_PROF_DATA_SECT_NAME);
+static const int dummy_name[0] COMPILER_RT_SECTION(
+    COMPILER_RT_SEG INSTR_PROF_NAME_SECT_NAME);
+static int dummy_vnds[0] COMPILER_RT_SECTION(
+    COMPILER_RT_SEG INSTR_PROF_VNODES_SECT_NAME);
+static void *volatile keep[] = {(void *)&dummy_cnts, (void *)&dummy_data,
+                                (void *)&dummy_name, (void *)&dummy_vnds};
+#endif
+
 COMPILER_RT_VISIBILITY const __llvm_profile_data *
 __llvm_profile_begin_data(void) {
   return &PROF_DATA_START;

   return &PROF_VNODES_START;
 }
 COMPILER_RT_VISIBILITY ValueProfNode *__llvm_profile_end_vnodes(void) {
+#ifdef _AIX
+  // Avoid GC'ing of the dummy variables by the linker. Also, this binds the
+  // liveness of the dummy vars with that of the __start_* and __stop_* symbols.
+  (void)keep[0];
+#endif
   return &PROF_VNODES_STOP;
 }
 COMPILER_RT_VISIBILITY ValueProfNode *CurrentVNode = &PROF_VNODES_START;

 }
 #endif
 
-#if defined(_AIX)
-// Empty stubs to allow linking object files using the registration-based scheme
-COMPILER_RT_VISIBILITY
-void __llvm_profile_register_function(void *Data_) {}
-
-COMPILER_RT_VISIBILITY
-void __llvm_profile_register_names_function(void *NamesStart,
-                                            uint64_t NamesSize) {}
-
-// The __start_SECNAME and __stop_SECNAME symbols (for SECNAME \in
-// {"__llvm_prf_cnts", "__llvm_prf_data", "__llvm_prf_name", "__llvm_prf_vnds"})
-// are always live when linking on AIX, regardless if the .o's being linked
-// 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(
-    COMPILER_RT_SEG INSTR_PROF_CNTS_SECT_NAME);
-COMPILER_RT_VISIBILITY 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(
-    COMPILER_RT_SEG INSTR_PROF_NAME_SECT_NAME);
-COMPILER_RT_VISIBILITY 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;
-}
-#endif
-
 #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(); }