[profile] Move __llvm_profile_raw_version into a separate file

[myhsu]

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

[vsk]

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() {}?

[vsk]

Thanks, lgtm.

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

[myhsu]

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.

[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

[vitalybuka]

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?

[myhsu]

In D83967#2157394, @vitalybuka wrote:

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

[MaskRay]

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]

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]

In D83967#2160599, @MaskRay wrote:

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]

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]

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]

In D83967#2160747, @myhsu wrote:

In D83967#2160599, @MaskRay wrote:

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]

In D83967#2160760, @MaskRay wrote:

In D83967#2160747, @myhsu wrote:

In D83967#2160599, @MaskRay wrote:

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.

[MaskRay]

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]

In D83967#2161209, @MaskRay wrote:

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]

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

compiler-rt/lib/profile/CMakeLists.txt

set(PROFILE_SOURCES
   InstrProfilingInternal.c
   InstrProfilingValue.c
   InstrProfilingBiasVar.c
   InstrProfilingBuffer.c
   InstrProfilingFile.c
   InstrProfilingMerge.c
   InstrProfilingMergeFile.c
   InstrProfilingNameVar.c
+  InstrProfilingVersionVar.c
   InstrProfilingWriter.c
   InstrProfilingPlatformDarwin.c
   InstrProfilingPlatformFuchsia.c
   InstrProfilingPlatformLinux.c
   InstrProfilingPlatformOther.c
   InstrProfilingPlatformWindows.c
   InstrProfilingRuntime.cpp
   InstrProfilingUtil.c

compiler-rt/lib/profile/InstrProfiling.c

 
 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 
 #define INSTR_PROF_VALUE_PROF_DATA
 #include "profile/InstrProfData.inc"
 
 
-COMPILER_RT_WEAK uint64_t INSTR_PROF_RAW_VERSION_VAR = INSTR_PROF_RAW_VERSION;
-
 COMPILER_RT_VISIBILITY uint64_t __llvm_profile_get_magic(void) {
   return sizeof(void *) == sizeof(uint64_t) ? (INSTR_PROF_RAW_MAGIC_64)
                                             : (INSTR_PROF_RAW_MAGIC_32);
 }
 
 COMPILER_RT_VISIBILITY void __llvm_profile_set_dumped() {
   lprofSetProfileDumped(1);
 }

compiler-rt/lib/profile/InstrProfilingVersionVar.c

+/*===- InstrProfilingVersionVar.c - profile version variable setup  -------===*\
+|*
+|* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+|* See https://llvm.org/LICENSE.txt for license information.
+|* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+|*
+\*===----------------------------------------------------------------------===*/
+
+#include "InstrProfiling.h"
+
+/* uint64 __llvm_profile_raw_version
+ *
+ * The runtime should only provide its own definition of this symbol when the
+ * user has not specified one. Set this up by moving the runtime's copy of this
+ * symbol to an object file within the archive.
+ */
+COMPILER_RT_WEAK uint64_t INSTR_PROF_RAW_VERSION_VAR = INSTR_PROF_RAW_VERSION;

compiler-rt/test/profile/instrprof-lto-pgogen.c

+// REQUIRES: lto
+
+// RUN: %clang_pgogen=%t.profraw -flto %s -o %t
+// RUN: %run %t
+// RUN: llvm-profdata merge %t.profraw -o %t.profdata
+// RUN: llvm-profdata show %t.profdata | FileCheck %s
+
+// Testing a bug that happens when trying to generate IR
+// profile with BFD linker + LTO plugin
+
+// CHECK: Instrumentation level: IR
+int main() { return 0; }