[compiler-rt/profile] Reland mark __llvm_profile_raw_version as hidden

[pirama]

Summary

Since libclang_rt.profile is added later in the command line, a
definition of __llvm_profile_raw_version is not included if it is
provided from an earlier object, e.g. from a shared dependency.

This causes an extra dependence edge where if libA.so depends on libB.so
and both are coverage-instrumented, libA.so uses libB.so's definition of
__llvm_profile_raw_version. This leads to a runtime link failure if the
libB.so available at runtime does not provide this symbol (but provides
the other dependent symbols). Such a scenario can occur in Android's
mainline modules.
E.g.:

ld -o libB.so libclang_rt.profile-x86_64.a
ld -o libA.so -l B libclang_rt.profile-x86_64.a

libB.so has a global definition of llvm_profile_raw_version. libA.so
uses libB.so's definition of llvm_profile_raw_version. At runtime,
libB.so may not be coverage-instrumented (i.e. not export
__llvm_profile_raw_version) so runtime linking of libA.so will fail.

Marking this symbol as hidden forces each binary to use the definition
of __llvm_profile_raw_version from libclang_rt.profile. The visiblity
is unchanged for Apple platforms where its presence is checked by the
TAPI tool.

[phosek]

LGTM

[MaskRay]

This leads to a runtime link failure if the libB.so available at runtime does not provide this symbol (but provides the other dependent symbols). Such a scenario can occur in Android's mainline modules.

The symbol is defined at link time but not available at run time? That's a pilot error.

compiler-rt/lib/profile/InstrProfiling.h

312

References don't need a visibility attribute.

[pirama]

Fix typos

[davidxl]

In the summary, do you mean 'not excluded' instead of 'not included' ?

[pirama]

Remove attribute from reference.

[pirama]

In D111759#3062703, @davidxl wrote:

In the summary, do you mean 'not excluded' instead of 'not included' ?

I did mean "not included".

`ld -o libA.so -lB ...`

If libB.so has an externally visible __llvm_profile_raw_version, then a definition of that symbol is not included in libA.so.

[pirama]

Apply clang-format linter suggestion.

[pirama]

In D111759#3062678, @MaskRay wrote:

This leads to a runtime link failure if the libB.so available at runtime does not provide this symbol (but provides the other dependent symbols). Such a scenario can occur in Android's mainline modules.

The symbol is defined at link time but not available at run time? That's a pilot error.

Normally I'd agree. However in this instance, this symbol is added by the coverage instrumentation. I think we should keep instrumentation as low-touch and avoid breaking workflows that otherwise work without coverage instrumentation.

[davidxl]

lgtm

[MaskRay]

This leads to a runtime link failure if the libB.so available at runtime does not provide this symbol (but provides the other dependent symbols). Such a scenario can occur in Android's mainline modules.

I guess my confusion is from the first paragraph:

Since it is marked as weak, a definition of __llvm_profile_raw_version is not included if it is available at runtime from another object, e.g. from a shared dependency.

I fail to connect the cause and the effect. How is weak related in this situation?

The Android use case needs a clear description. If someone needs to untangle the history, this can make contributors confused.

[pirama]

This is not related to weak attribute to __llvm_profile_raw_version

[pirama]

Describe Android use case.

[MaskRay]

Thanks for the description update. It is clear now :)

Maybe something like Hide __llvm_profile_raw_version so as not to resolve reference from a dependent shared object can clarify the intention.

[MaskRay]

Looks great!

[zequanwu]

This causes instrprof-darwin-exports.c test failure on MacOS:
ld: warning: cannot export hidden symbol ___llvm_profile_raw_version from /Users/zequanwu/llvm-project/build/release/lib/clang/14.0.0/lib/darwin/libclang_rt.profile_osx.a(InstrProfilingVersionVar.c.o)

[pirama]

In D111759#3076819, @zequanwu wrote:

This causes instrprof-darwin-exports.c test failure on MacOS:
ld: warning: cannot export hidden symbol ___llvm_profile_raw_version from /Users/zequanwu/llvm-project/build/release/lib/clang/14.0.0/lib/darwin/libclang_rt.profile_osx.a(InstrProfilingVersionVar.c.o)

I think we should stop exporting ___llvm_profile_raw_version from Darwin::addProfileRTLibs() in clang/lib/Driver/ToolChains/Darwin.cpp. @vsk Can you confirm?

[zequanwu]

In D111759#3076827, @pirama wrote:

In D111759#3076819, @zequanwu wrote:

This causes instrprof-darwin-exports.c test failure on MacOS:
ld: warning: cannot export hidden symbol ___llvm_profile_raw_version from /Users/zequanwu/llvm-project/build/release/lib/clang/14.0.0/lib/darwin/libclang_rt.profile_osx.a(InstrProfilingVersionVar.c.o)

I think we should stop exporting ___llvm_profile_raw_version from Darwin::addProfileRTLibs() in clang/lib/Driver/ToolChains/Darwin.cpp. @vsk Can you confirm?

Can you revert it for now to unblock the test failure on MacOS bots?

[pirama]

In D111759#3078579, @zequanwu wrote:

In D111759#3076827, @pirama wrote:

In D111759#3076819, @zequanwu wrote:

This causes instrprof-darwin-exports.c test failure on MacOS:
ld: warning: cannot export hidden symbol ___llvm_profile_raw_version from /Users/zequanwu/llvm-project/build/release/lib/clang/14.0.0/lib/darwin/libclang_rt.profile_osx.a(InstrProfilingVersionVar.c.o)

I think we should stop exporting ___llvm_profile_raw_version from Darwin::addProfileRTLibs() in clang/lib/Driver/ToolChains/Darwin.cpp. @vsk Can you confirm?

Can you revert it for now to unblock the test failure on MacOS bots?

Done. Zequan, can you help test if removing https://github.com/llvm/llvm-project/blob/ab3d5d0533678ee93c93bd62db79c091741096f0/clang/lib/Driver/ToolChains/Darwin.cpp#L1292 fixes all failures? I'll submit this again once @vsk confirms if that is acceptable.

[zequanwu]

In D111759#3078677, @pirama wrote:

In D111759#3078579, @zequanwu wrote:

In D111759#3076827, @pirama wrote:

In D111759#3076819, @zequanwu wrote:

This causes instrprof-darwin-exports.c test failure on MacOS:
ld: warning: cannot export hidden symbol ___llvm_profile_raw_version from /Users/zequanwu/llvm-project/build/release/lib/clang/14.0.0/lib/darwin/libclang_rt.profile_osx.a(InstrProfilingVersionVar.c.o)

I think we should stop exporting ___llvm_profile_raw_version from Darwin::addProfileRTLibs() in clang/lib/Driver/ToolChains/Darwin.cpp. @vsk Can you confirm?

Can you revert it for now to unblock the test failure on MacOS bots?

Done. Zequan, can you help test if removing https://github.com/llvm/llvm-project/blob/ab3d5d0533678ee93c93bd62db79c091741096f0/clang/lib/Driver/ToolChains/Darwin.cpp#L1292 fixes all failures? I'll submit this again once @vsk confirms if that is acceptable.

That doesn't help. I got different error:

Command Output (stdout):
--
2d1
< ___llvm_profile_raw_version

--

[pirama]

In D111759#3078775, @zequanwu wrote:

In D111759#3078677, @pirama wrote:

In D111759#3078579, @zequanwu wrote:

In D111759#3076827, @pirama wrote:

In D111759#3076819, @zequanwu wrote:

This causes instrprof-darwin-exports.c test failure on MacOS:
ld: warning: cannot export hidden symbol ___llvm_profile_raw_version from /Users/zequanwu/llvm-project/build/release/lib/clang/14.0.0/lib/darwin/libclang_rt.profile_osx.a(InstrProfilingVersionVar.c.o)

I think we should stop exporting ___llvm_profile_raw_version from Darwin::addProfileRTLibs() in clang/lib/Driver/ToolChains/Darwin.cpp. @vsk Can you confirm?

Can you revert it for now to unblock the test failure on MacOS bots?

Done. Zequan, can you help test if removing https://github.com/llvm/llvm-project/blob/ab3d5d0533678ee93c93bd62db79c091741096f0/clang/lib/Driver/ToolChains/Darwin.cpp#L1292 fixes all failures? I'll submit this again once @vsk confirms if that is acceptable.

That doesn't help. I got different error:

Command Output (stdout):
--
2d1
< ___llvm_profile_raw_version

--

Is this the same test? What is the 'RUN: ' line that's failing?

[zequanwu]

In D111759#3078904, @pirama wrote:

In D111759#3078775, @zequanwu wrote:

In D111759#3078677, @pirama wrote:

In D111759#3078579, @zequanwu wrote:

In D111759#3076827, @pirama wrote:

In D111759#3076819, @zequanwu wrote:

This causes instrprof-darwin-exports.c test failure on MacOS:
ld: warning: cannot export hidden symbol ___llvm_profile_raw_version from /Users/zequanwu/llvm-project/build/release/lib/clang/14.0.0/lib/darwin/libclang_rt.profile_osx.a(InstrProfilingVersionVar.c.o)

I think we should stop exporting ___llvm_profile_raw_version from Darwin::addProfileRTLibs() in clang/lib/Driver/ToolChains/Darwin.cpp. @vsk Can you confirm?

Can you revert it for now to unblock the test failure on MacOS bots?

Done. Zequan, can you help test if removing https://github.com/llvm/llvm-project/blob/ab3d5d0533678ee93c93bd62db79c091741096f0/clang/lib/Driver/ToolChains/Darwin.cpp#L1292 fixes all failures? I'll submit this again once @vsk confirms if that is acceptable.

That doesn't help. I got different error:

Command Output (stdout):
--
2d1
< ___llvm_profile_raw_version

--

Is this the same test? What is the 'RUN: ' line that's failing?

It's the same test failing at line 26 after removing.

[MaskRay]

According to https://reviews.llvm.org/rGf5380185623be243ba0f1b18d4bd594ac5cc7163 Apple's API verification tool (tapi) checks the exported __llvm_profile_raw_version.
However, ld64 cannot export a private-extern (hidden) symbol.

I wonder whether Apple's tool should lift the restriction. If not, we may need to special case __APPLE__.

[pirama]

In D111759#3079067, @MaskRay wrote:

According to https://reviews.llvm.org/rGf5380185623be243ba0f1b18d4bd594ac5cc7163 Apple's API verification tool (tapi) checks the exported __llvm_profile_raw_version.
However, ld64 cannot export a private-extern (hidden) symbol.

I wonder whether Apple's tool should lift the restriction. If not, we may need to special case __APPLE__.

Special-casing __APPLE__ SGTM but I'll wait a few days for @vsk's response. Though disabling the export and fixing the test seems the right thing to do. Removing the addExportedSymbol(CmdArgs, "___llvm_profile_raw_version"); ought to have fixed it but there seems to be some nuance to this test that I'm missing.

[pirama]

Do not hide __llvm_profile_raw_version on Apple platforms.

[pirama]

arc diff --verbatim to update phabricator message

compiler-rt/lib/profile/InstrProfiling.h

  * define this variable, then InstProfileRuntime.o won't be linked in.
  * Note that this variable's visibility needs to be hidden so that the
  * definition of this variable in an instrumented shared library won't
  * affect runtime initialization decision of the main program.
  *  __llvm_profile_profile_runtime. */
 COMPILER_RT_VISIBILITY extern int INSTR_PROF_PROFILE_RUNTIME_VAR;
 
 /*!
- * This variable is defined in InstrProfiling.c. Its main purpose is to
- * encode the raw profile version value and other format related information
- * such as whether the profile is from IR based instrumentation. The variable
- * is defined as weak so that compiler can emit an overriding definition
- * depending on user option.  Since we don't support mixing FE and IR based
- * data in the same raw profile data file (in other words, shared libs and
- * main program are expected to be instrumented in the same way), there is
- * no need for this variable to be hidden.
+ * This variable is defined in InstrProfilingVersionVar.c as a hidden symbol
+ * (except on Apple platforms where this symbol is checked by TAPI).  Its main
+ * purpose is to encode the raw profile version value and other format related
+ * information such as whether the profile is from IR based instrumentation. The
+ * variable is defined as weak so that compiler can emit an overriding
+ * definition depending on user option.
  */
 extern uint64_t INSTR_PROF_RAW_VERSION_VAR; /* __llvm_profile_raw_version */
 
 /*!
  * This variable is a weak symbol defined in InstrProfiling.c. It allows
  * compiler instrumentation to provide overriding definition with value
  * from compiler command line. This variable has default visibility.
  */
 extern char INSTR_PROF_PROFILE_NAME_VAR[1]; /* __llvm_profile_filename. */
 
 #endif /* PROFILE_INSTRPROFILING_H_ */

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.
+ *
+ * Hide this symbol everywhere except Apple platforms, where its presence is
+ * checked by the TAPI tool.
  */
-COMPILER_RT_WEAK uint64_t INSTR_PROF_RAW_VERSION_VAR = INSTR_PROF_RAW_VERSION;
+#if !defined(__APPLE__)
+#define VERSION_VAR_VISIBILITY COMPILER_RT_VISIBILITY
+#else
+#define VERSION_VAR_VISIBILITY
+#endif
+VERSION_VAR_VISIBILITY COMPILER_RT_WEAK uint64_t INSTR_PROF_RAW_VERSION_VAR =
+    INSTR_PROF_RAW_VERSION;