This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by w2yehia on Sep 19 2022, 7:55 PM.

Details

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

Diff Detail

Event Timeline

w2yehia created this revision.Sep 19 2022, 7:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 7:55 PM
Herald added subscribers: Enna1, wenlei. · View Herald Transcript
w2yehia requested review of this revision.Sep 19 2022, 7:55 PM
w2yehia updated this revision to Diff 461460.Sep 19 2022, 8:39 PM

add testcase

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 added inline comments.Sep 20 2022, 6:17 AM
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 updated this revision to Diff 461569.Sep 20 2022, 8:00 AM

Address code review comments.

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 added inline comments.Sep 20 2022, 11:44 AM
compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
266

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

w2yehia updated this revision to Diff 464314.Sep 30 2022, 10:19 AM
w2yehia edited the summary of this revision. (Show Details)

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

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

@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).

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.

This revision is now accepted and ready to land.Sep 30 2022, 9:05 PM
w2yehia added inline comments.Oct 17 2022, 12:06 PM
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 updated this revision to Diff 468288.Oct 17 2022, 12:29 PM

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

@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

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.

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

w2yehia abandoned this revision.Oct 27 2022, 9:10 AM

Fixed by D136192