Page MenuHomePhabricator

[profile] Move __llvm_profile_raw_version into a separate file
ClosedPublic

Authored by myhsu on Jul 16 2020, 10:26 AM.

Details

Summary

Similar to the reason behind moving __llvm_profile_filename into a separate file[1]. When users use Full LTO with BFD linker to generate IR level PGO profile, the __llvm_profile_raw_version variable, which is used for marking instrumentation level and generated by frontend, would somehow conflict with the weak symbol provided by the profiling runtime. To go a little more details, in many cases, BFD linkers will pick profiling runtime's weak symbol as the real definition of __llvm_profile_raw_version rather than the one generated by frontend and thus generate the incorrect instrumentation level metadata in the final executables.

Moving __llvm_profile_raw_version into a separate file would make linkers not seeing the weak symbol in the archive unless the frontend doesn't generate one.

[1] https://reviews.llvm.org/D34797

Diff Detail

Event Timeline

myhsu created this revision.Jul 16 2020, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2020, 10:26 AM
Herald added subscribers: Restricted Project, dexonsmith, steven_wu and 2 others. · View Herald Transcript
dmajor added a subscriber: dmajor.Jul 16 2020, 11:08 AM
vsk added inline comments.Jul 16 2020, 12:07 PM
compiler-rt/test/profile/instrprof-linux-lto-pgogen.c
1 ↗(On Diff #278534)

Is there a need to limit this to Linux? I notice that it's not also limited to when the bfd linker is in use.

13 ↗(On Diff #278534)

Can the test be reduced to just int main() {}?

myhsu updated this revision to Diff 278596.Jul 16 2020, 1:46 PM
myhsu marked 2 inline comments as done.
vsk accepted this revision.Jul 16 2020, 1:48 PM

Thanks, lgtm.

Separate from this patch: perhaps it's worth renaming compiler-rt/test/profile/Linux to compiler-rt/test/profile/LinuxWithGoldLinker (or similar).

This revision is now accepted and ready to land.Jul 16 2020, 1:48 PM
myhsu added a comment.Jul 16 2020, 2:00 PM
In D83967#2156952, @vsk wrote:

Separate from this patch: perhaps it's worth renaming compiler-rt/test/profile/Linux to compiler-rt/test/profile/LinuxWithGoldLinker (or similar).

I don't have any particularly insight on it but I guess that makes sense since the config implicitly uses gold linker for some lit substitutions like %clang_profgen.

That reminds me to change the test file in this patch from instrprof-linux-lto-pgogen.c to just instrprof-lto-pgogen.c. I will do that when I committed it without another diff update here since it's trivial.

This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.Jul 16 2020, 7:38 PM
vitalybuka added a subscriber: vitalybuka.

The test does not work on windows
http://lab.llvm.org:8011/builders/sanitizer-windows/builds/66529/steps/stage%201%20check/logs/stdio

FAIL: Profile-x86_64 :: instrprof-lto-pgogen.c (58 of 91)
******************** TEST 'Profile-x86_64 :: instrprof-lto-pgogen.c' FAILED ********************
Script:
--
: 'RUN: at line 3';      C:/b/slave/sanitizer-windows/build/stage1/./bin/clang.exe     -Wl,-incremental:no  -fprofile-generate=C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\profile\Profile-x86_64\Output\instrprof-lto-pgogen.c.tmp.profraw -flto C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\profile\instrprof-lto-pgogen.c -o C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\profile\Profile-x86_64\Output\instrprof-lto-pgogen.c.tmp
: 'RUN: at line 4';    C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\profile\Profile-x86_64\Output\instrprof-lto-pgogen.c.tmp
: 'RUN: at line 5';   llvm-profdata merge C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\profile\Profile-x86_64\Output\instrprof-lto-pgogen.c.tmp.profraw -o C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\profile\Profile-x86_64\Output\instrprof-lto-pgogen.c.tmp.profdata
: 'RUN: at line 6';   llvm-profdata show C:\b\slave\sanitizer-windows\build\stage1\projects\compiler-rt\test\profile\Profile-x86_64\Output\instrprof-lto-pgogen.c.tmp.profdata | FileCheck C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\profile\instrprof-lto-pgogen.c
--
Exit Code: 1107
This revision is now accepted and ready to land.Jul 16 2020, 7:38 PM

I've disabled the test https://reviews.llvm.org/rGb128f719a4c826e8f723eaa9b42b607c81f563a5
Please could you take a look and fix it or add comment explaining why it can't be done?

I've disabled the test https://reviews.llvm.org/rGb128f719a4c826e8f723eaa9b42b607c81f563a5
Please could you take a look and fix it or add comment explaining why it can't be done?

Sure. Though I don't have access to a Windows machine now but I'll try my best

Can you elaborate a bit how the previous behavior was broken? The test %clang_pgogen=%t.profraw -flto %s -o %t works with me with the code change reverted. By passing -Wl,-plugin-opt=save-temps, I get:

/usr/bin/ld.bfd: /tmp/instrprof-lto-pgogen-bb10dc.o (symbol from plugin): definition of __llvm_profile_raw_version
/usr/bin/ld.bfd: /tmp/Debug/lib/clang/12.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
% cat a.out.resolution.txt 
instrprof-lto-pgogen.o
-r=instrprof-lto-pgogen.o,main,plx
-r=instrprof-lto-pgogen.o,__llvm_profile_raw_version,l

The resolution to __llvm_profile_raw_version should be plx. This is a GNU ld bug (https://sourceware.org/bugzilla/show_bug.cgi?id=26262 ). However, it does not affect correctness if your clang and libclang_rt.profile- are of the same version.

MaskRay added inline comments.Jul 18 2020, 5:08 PM
compiler-rt/test/profile/instrprof-lto-pgogen.c
3

The test does not work if

  • ld in PATH is lld of an older version with a different summary version (ld: error: Invalid summary version 9. Version should be in the range [1-8].)
  • ld is GNU ld or gold but LLVMgold.so is not built
myhsu added a comment.Jul 18 2020, 8:44 PM

Can you elaborate a bit how the previous behavior was broken? The test %clang_pgogen=%t.profraw -flto %s -o %t works with me with the code change reverted. By passing -Wl,-plugin-opt=save-temps, I get:

This patch is a workaround for exactly the same binutils bug you mentioned below. I'm happy to see it got fixed in binutils, but I think this patch is still necessary given the fact that many users are still using unfixed version of binutils.

I'm not sure why you could not reproduce the bug with this changes reverted, because the compile command is nearly identical to what you attached in the binutils bug report:

./bin/clang   -m64  -ldl  -fprofile-generate=build/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-lto-pgogen.c.tmp.profraw -flto llvm-project/compiler-rt/test/profile/instrprof-lto-pgogen.c -o llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-lto-pgogen.c.tmp
/usr/bin/ld.bfd: /tmp/instrprof-lto-pgogen-bb10dc.o (symbol from plugin): definition of __llvm_profile_raw_version
/usr/bin/ld.bfd: /tmp/Debug/lib/clang/12.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
% cat a.out.resolution.txt 
instrprof-lto-pgogen.o
-r=instrprof-lto-pgogen.o,main,plx
-r=instrprof-lto-pgogen.o,__llvm_profile_raw_version,l

The resolution to __llvm_profile_raw_version should be plx. This is a GNU ld bug (https://sourceware.org/bugzilla/show_bug.cgi?id=26262 ). However, it does not affect correctness if your clang and libclang_rt.profile- are of the same version.

myhsu marked 2 inline comments as done.Jul 18 2020, 8:54 PM
myhsu added inline comments.
compiler-rt/test/profile/instrprof-lto-pgogen.c
3

The test does not work if
ld in PATH is lld of an older version with a different summary version (ld: error: Invalid summary version 9. Version should be in the range [1-8].)

I encountered this bug before. But as you said, this is due to inconsistent version of lld, which is not regression tests' responsibility. If you have a clean build and run the regression tests, the issue will not happen.

ld is GNU ld or gold but LLVMgold.so is not built

Correct, but if you don't build LLVMgold.so, all of the tests related to LTO will fail, not just this one

myhsu marked an inline comment as done.Jul 18 2020, 8:56 PM
myhsu marked an inline comment as done.
myhsu added inline comments.
compiler-rt/test/profile/instrprof-lto-pgogen.c
3

Correct, but if you don't build LLVMgold.so, all of the tests related to LTO will fail, not just this one

Correction: "...if you're using ld or gold and didn't build LLVMgold.so, all of the tests..."

MaskRay added a comment.EditedJul 18 2020, 9:11 PM

Can you elaborate a bit how the previous behavior was broken? The test %clang_pgogen=%t.profraw -flto %s -o %t works with me with the code change reverted. By passing -Wl,-plugin-opt=save-temps, I get:

This patch is a workaround for exactly the same binutils bug you mentioned below. I'm happy to see it got fixed in binutils, but I think this patch is still necessary given the fact that many users are still using unfixed version of binutils.

I'm not sure why you could not reproduce the bug with this changes reverted, because the compile command is nearly identical to what you attached in the binutils bug report:

./bin/clang   -m64  -ldl  -fprofile-generate=build/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-lto-pgogen.c.tmp.profraw -flto llvm-project/compiler-rt/test/profile/instrprof-lto-pgogen.c -o llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-lto-pgogen.c.tmp

My ld in PATH is a slightly older one.

% /usr/local/bin/ld --version 
LLD 11.0.0 (https://github.com/llvm/llvm-project/ 4e813bbdf335626a741398314709a535ef52fe8e) (compatible with GNU linkers)

%clang_pgogen=%t.profraw -flto %s -o %t fails because there is a mismatch of LTO summary versions.

This patch is a workaround for exactly the same binutils bug you mentioned below. I'm happy to see it got fixed in binutils, but I think this patch is still necessary given the fact that many users are still using unfixed version of binutils.

I still fail to understand your rationale. This is a GNU ld bug and it affects the prevailing definition it picks (application object file or libclang_rt.profile). However, that does not matter in practice. So what did you try to fix in this patch? Are you using clang with a different version of LLVMgold.so? How? I am not sure we want to support clang with an LLVMgold.so of a mismatched version.

myhsu added a comment.EditedJul 18 2020, 9:21 PM

Can you elaborate a bit how the previous behavior was broken? The test %clang_pgogen=%t.profraw -flto %s -o %t works with me with the code change reverted. By passing -Wl,-plugin-opt=save-temps, I get:

This patch is a workaround for exactly the same binutils bug you mentioned below. I'm happy to see it got fixed in binutils, but I think this patch is still necessary given the fact that many users are still using unfixed version of binutils.

I'm not sure why you could not reproduce the bug with this changes reverted, because the compile command is nearly identical to what you attached in the binutils bug report:

./bin/clang   -m64  -ldl  -fprofile-generate=build/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-lto-pgogen.c.tmp.profraw -flto llvm-project/compiler-rt/test/profile/instrprof-lto-pgogen.c -o llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-lto-pgogen.c.tmp

My ld in PATH is a slightly older one.

% /usr/local/bin/ld --version 
LLD 11.0.0 (https://github.com/llvm/llvm-project/ 4e813bbdf335626a741398314709a535ef52fe8e) (compatible with GNU linkers)

As I mentioned in the description and commit message, this bug only happened with BFD linker. "LLD 11.0.0...." showing that you're using LLD.

%clang_pgogen=%t.profraw -flto %s -o %t fails because there is a mismatch of LTO summary versions.

This patch is a workaround for exactly the same binutils bug you mentioned below. I'm happy to see it got fixed in binutils, but I think this patch is still necessary given the fact that many users are still using unfixed version of binutils.

I still fail to understand your rationale. This is a GNU ld bug and it affects the prevailing definition it picks (application object file or libclang_rt.profile). However, that does not matter in practice.

Ahh I finally understand your confusions: So __llvm_profile_raw_version is much more important than just version number, it is also used to marked the instrumentation level of PGO profile (i.e. Frontend, IR, or Context-sensitive(CS) IR). Certain optimizations, especially those included in LTO pipeline, _only_ accept IR or even CS IR level profile. If the prevailing symbol in compiler-rt override the one provided in users' code, the PGO profile generated by executables will _always_ be Frontend level. Many optimizations in LTO pipelines will thus happily reject those PGO profile files and lost many optimization opportunities.

So what did you try to fix in this patch? Are you using clang with a different version of LLVMgold.so? How? I am not sure we want to support clang with an LLVMgold.so of a mismatched version.

Thanks. Sent D84133

I know little about PGO and its runtime. Do you know what will happen if Instrumentation level: Front-end is used? (I added a comment to https://sourceware.org/bugzilla/show_bug.cgi?id=26262#c2 )

myhsu added a comment.Jul 20 2020, 9:13 AM

Thanks. Sent D84133

I know little about PGO and its runtime. Do you know what will happen if Instrumentation level: Front-end is used? (I added a comment to https://sourceware.org/bugzilla/show_bug.cgi?id=26262#c2 )

As I mentioned in the previous comment, if the instrumentation level is Frontend, some optimization Passes will not consume the PGO profile files and finish the optimization process silently. In other words, there won't be any compilation error or warning, but the input code won't be optimized as well.

MaskRay closed this revision.EditedJul 22 2020, 10:54 AM

The soon-be-released binutils 2.35 will be good without the workaround. https://sourceware.org/bugzilla/show_bug.cgi?id=26262

clang -fuse-ld=bfd --ld-path=~/Dev/binutils-gdb/Debug/ld/ld-new -fprofile-generate=out -flto a.c -o a
./a
llvm-profdata merge out -o a.profdata
llvm-profdata show a.profdata => IR