[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

87–88

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

87–88

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

 }
 
 COMPILER_RT_VISIBILITY ValueProfNode *
 __llvm_profile_begin_vnodes(void) {
   return &PROF_VNODES_START;
 }
 COMPILER_RT_VISIBILITY ValueProfNode *__llvm_profile_end_vnodes(void) {
   return &PROF_VNODES_STOP;
 }
 COMPILER_RT_VISIBILITY ValueProfNode *CurrentVNode = &PROF_VNODES_START;
 COMPILER_RT_VISIBILITY ValueProfNode *EndVNode = &PROF_VNODES_STOP;
 
 #ifdef NT_GNU_BUILD_ID
 static size_t RoundUp(size_t size, size_t align) {
   return (size + align - 1) & ~(align - 1);
 }
 
 /*
static int WriteOneBinaryId(ProfDataWriter *Writer, uint64_t BinaryIdLen,
   return 0;
 }
 
 /*
  * Look for the note that has the name "GNU\0" and type NT_GNU_BUILD_ID
  * that contains build id. If build id exists, write binary id.
  *
  * Each note in notes section starts with a struct which includes
  * n_namesz, n_descsz, and n_type members. It is followed by the name
  * (whose length is defined in n_namesz) and then by the descriptor
  * (whose length is defined in n_descsz).
  *
  * Note sections like .note.ABI-tag and .note.gnu.build-id are aligned
  * to 4 bytes, so round n_namesz and n_descsz to the nearest 4 bytes.
  */
 static int WriteBinaryIdForNote(ProfDataWriter *Writer,
                                 const ElfW(Nhdr) * Note) {
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() {
+COMPILER_RT_VISIBILITY __attribute__((destructor)) void keep() {
   int volatile use = &dummy_cnts < &dummy_data && &dummy_name < &dummy_vnds;
   (void)use;
 }
 #endif
 
 #endif