This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Make emitted symbols hidden
ClosedPublic

Authored by abrachet on Oct 5 2022, 7:07 PM.

Details

Summary

This was reverted because it was breaking when targeting Darwin which
tried to export these symbols which are now hidden. It should be safe
to just stop attempting to export these symbols in the clang driver,
though Apple folks will need to change their TAPI allow list described
in the commit where these symbols were originally exported
https://github.com/llvm/llvm-project/commit/f5380185623be243ba0f1b18d4bd594ac5cc7163

Bug: https://github.com/llvm/llvm-project/issues/58265

Diff Detail

Event Timeline

abrachet created this revision.Oct 5 2022, 7:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 7:07 PM
abrachet requested review of this revision.Oct 5 2022, 7:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 7:07 PM

I think __llvm_profile_raw_version can be made hidden but can you state the motivation?

Is it due to the mixing instrumented shared libraries built with different compiler and profile versions?

It's just not necessary for these symbols to have default visibility. and its preferable that the compiler expose as few implementation details as possible. We track the abi's of our shared objects closely with llvm-ifs and were surprised to find these symbols publicly exposed

mcgrathr accepted this revision.Oct 6 2022, 11:14 AM
mcgrathr added a subscriber: mcgrathr.

lgtm

This revision is now accepted and ready to land.Oct 6 2022, 11:14 AM
MaskRay accepted this revision.Oct 6 2022, 11:24 AM

The subject should change the emitted symbol to __llvm_profile_raw_version.

The subject should change the emitted symbol to __llvm_profile_raw_version.

This change affects __llvm_profile_raw_version and __llvm_profile_filename.
The invariant that should be maintained is that *all* symbols this instrumentation injects should have hidden visibility (if they're not local).

This revision was automatically updated to reflect the committed changes.
abrachet reopened this revision.Oct 10 2022, 9:48 AM
abrachet updated this revision to Diff 466535.
abrachet edited the summary of this revision. (Show Details)
abrachet added a reviewer: vsk.
This revision is now accepted and ready to land.Oct 10 2022, 9:48 AM
gulfem added a subscriber: gulfem.Oct 10 2022, 4:54 PM

This definitely needs some more eyes from Apple engineers. Unfortunately @vsk isn't too active on this part of the compiler anymore. I don't work on PGO myself or TAPI so I can't approve this change. I've added @arphaman and @cishida who might have opinions here.

cishida accepted this revision.Oct 13 2022, 12:44 PM

This change LGTM. My understanding of the history here, is that these symbols were exported expecting to be eventually referenced when profiling options are used but weren't always consistently exported. This kind of inconsistency of symbol exports breaks TAPI verification. In the recent past we've had a similar issue where the symbols got optimized out based on the optimization level. If __llvm_profile_raw_version and __llvm_profile_filename always have hidden visibility this should be fine for TAPI.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 12:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This causing instrprof-darwin-dead-strip.c failed on mac.

FAIL: Profile-x86_64 :: instrprof-darwin-dead-strip.c (1 of 238)
******************** TEST 'Profile-x86_64 :: instrprof-darwin-dead-strip.c' FAILED ********************
Script:
--
: 'RUN: at line 4';      /Users/zequanwu/llvm-project/build/cmake/./bin/clang   -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.10 -isysroot /Users/zequanwu/llvm-project/build/cmake/../../sysroot/MacOSX.sdk   -fprofile-instr-generate=/Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.profraw -fcoverage-mapping -mllvm -enable-name-compression=false -DCODE=1 -Wl,-dead_strip -o /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c
: 'RUN: at line 5';    /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp
: 'RUN: at line 6';   llvm-profdata merge -o /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.profdata /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.profraw
: 'RUN: at line 7';   llvm-profdata show --all-functions /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.profdata | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=PROF
: 'RUN: at line 8';   llvm-cov show /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp -instr-profile /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.profdata | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=COV
: 'RUN: at line 9';   nm /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=NM
: 'RUN: at line 10';   otool -V -s __DATA __llvm_prf_names /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=PRF_NAMES
: 'RUN: at line 11';   otool -V -s __DATA __llvm_prf_cnts /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=PRF_CNTS
: 'RUN: at line 13';      /Users/zequanwu/llvm-project/build/cmake/./bin/clang  -Wl,-lto_library,/Users/zequanwu/llvm-project/build/cmake/./lib/libLTO.dylib -flto  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.10 -isysroot /Users/zequanwu/llvm-project/build/cmake/../../sysroot/MacOSX.sdk   -fprofile-instr-generate=/Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto.profraw -fcoverage-mapping -mllvm -enable-name-compression=false -DCODE=1 -Wl,-dead_strip -flto -o /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c
: 'RUN: at line 14';    /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto
: 'RUN: at line 15';   llvm-profdata merge -o /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto.profdata /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto.profraw
: 'RUN: at line 16';   llvm-profdata show --all-functions /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto.profdata | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=PROF
: 'RUN: at line 17';   llvm-cov show /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto -instr-profile /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto.profdata | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=COV
: 'RUN: at line 18';   nm /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=NM
: 'RUN: at line 19';   otool -V -s __DATA __llvm_prf_names /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=PRF_NAMES
: 'RUN: at line 20';   otool -V -s __DATA __llvm_prf_cnts /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.lto | FileCheck /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c -check-prefix=PRF_CNTS
: 'RUN: at line 28';      /Users/zequanwu/llvm-project/build/cmake/./bin/clang   -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.10 -isysroot /Users/zequanwu/llvm-project/build/cmake/../../sysroot/MacOSX.sdk   -fprofile-instr-generate -fcoverage-mapping -Wl,-dead_strip -dynamiclib -o /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.nocode.dylib /Users/zequanwu/llvm-project/compiler-rt/test/profile/instrprof-darwin-dead-strip.c
: 'RUN: at line 29';   nm -jgU /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.nocode.dylib > /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.nocode.syms
: 'RUN: at line 30';   nm -jgU /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp | grep -vE "main|foo|mh_execute_header" > /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.code.syms
: 'RUN: at line 31';   diff /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.nocode.syms /Users/zequanwu/llvm-project/build/cmake/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-darwin-dead-strip.c.tmp.code.syms
--
Exit Code: 1

Command Output (stdout):
--
1d0
< ___llvm_profile_filename

--
abrachet updated this revision to Diff 468425.Oct 17 2022, 11:07 PM

The error @zequanwu ran into happens only on MacOS because it this Darwin-only test happens to be the only one that tests instrprof with no code. __llvm_profile_filename doesn't get emitted in this case so it ends up pulling in the one in libprofile.a which is not hidden. I've changed this to make __llvm_profile_filename hidden, as well as __llvm_profile_raw_version even though that symbol is always emitted.

@cishida, to verify, the comment in InstrProfilingVersionVar.c said that __llvm_profile_raw_version was not hidden on Apple platforms because of TAPI, it should be ok to mark this as hidden in libprofile.a, right? I'm guessing this should be no different than when the symbol is emitted by llvm.

This revision was landed with ongoing or failed builds.Oct 24 2022, 12:05 PM
thakis added a subscriber: thakis.Oct 25 2022, 5:54 AM

This is still breaking Profile-x86_64 :: instrprof-darwin-dead-strip.c on mac. I'm reverting this again for now.

abrachet updated this revision to Diff 470597.Oct 25 2022, 12:41 PM

Use sed to filter nm output instead of grep. grep exits with exit code 1 because there are no matching lines. This is expected because there are no more external symbols other than _main and __mh_execute_header, note _foo has been dead stripped.

This is still breaking Profile-x86_64 :: instrprof-darwin-dead-strip.c on mac. I'm reverting this again for now.

Sorry about that, I've tested it locally and it should be good to go now. But I'll give some time before resubmitting if you'd like to look it over.

This revision was landed with ongoing or failed builds.Oct 26 2022, 10:13 AM