[profile] Don't use pragma comment linker on mingw

[nikic]

Summary

At least when compiling with gcc, this is not supported and will result in errors when linking against the profiler runtime. Only use the pragma comment linker based code with MSVC, but not with a mingw toolchain. This also undoes D107620, which shouldn't be relevant anymore.

[mstorsjo]

FWIW, when using lld as linker in a mingw setting, it does support those embedded directives, but ld.bfd doesn't indeed. We do use those extensions in compiler-rt's sanitizers though, and have been doing that for a couple years. I guess those are considered unsupported when building with GCC.

In this case, we'd lose the weak linked symbol altogether in mingw configurations. I'm not familiar with the profiler library so I don't know exactly how big a feature this loses though... Alternatively this could maybe use a regular __attribute__((weak)) symbol in mingw configurations, which both GCC and Clang, ld.bfd and lld might support.

[mstorsjo]

In D108095#2945817, @mstorsjo wrote:

FWIW, when using lld as linker in a mingw setting, it does support those embedded directives, but ld.bfd doesn't indeed. We do use those extensions in compiler-rt's sanitizers though, and have been doing that for a couple years. I guess those are considered unsupported when building with GCC.

In this case, we'd lose the weak linked symbol altogether in mingw configurations. I'm not familiar with the profiler library so I don't know exactly how big a feature this loses though... Alternatively this could maybe use a regular __attribute__((weak)) symbol in mingw configurations, which both GCC and Clang, ld.bfd and lld might support.

Oh, now I see there actually is such an #else codepath here. I guess that'd work yes. I don't have any continuous testing setup that uses the profiling library though so I won't know immediately if it broke or not.

Btw, to understand your setup - you're building compiler-rt/profile as part of building clang, via e.g. LLVM_ENABLE_PROJECTS (so it ends up being built with the existing host compiler, GCC) but then compile code using the clang and using the newly built compiler-rt/profile library with the newly built clang?

[mstorsjo]

In D108095#2945822, @mstorsjo wrote:

In D108095#2945817, @mstorsjo wrote:

FWIW, when using lld as linker in a mingw setting, it does support those embedded directives, but ld.bfd doesn't indeed. We do use those extensions in compiler-rt's sanitizers though, and have been doing that for a couple years. I guess those are considered unsupported when building with GCC.

In this case, we'd lose the weak linked symbol altogether in mingw configurations. I'm not familiar with the profiler library so I don't know exactly how big a feature this loses though... Alternatively this could maybe use a regular __attribute__((weak)) symbol in mingw configurations, which both GCC and Clang, ld.bfd and lld might support.

Oh, now I see there actually is such an #else codepath here. I guess that'd work yes. I don't have any continuous testing setup that uses the profiling library though so I won't know immediately if it broke or not.

Btw, to understand your setup - you're building compiler-rt/profile as part of building clang, via e.g. LLVM_ENABLE_PROJECTS (so it ends up being built with the existing host compiler, GCC) but then compile code using the clang and using the newly built compiler-rt/profile library with the newly built clang?

Sorry, I’m a bit slow today: I presume the situation is that this setup used to work fine, when doing profiled builds with clang, when linking with ld.bfd - but now it no longer does. But sanitizers have always required that they (the compiler-rt code) are built with clang (e.g. with LLVM_ENABLE_RUNTIMES, or entirely manually/separately) and linked with lld.

[nikic]

In D108095#2945822, @mstorsjo wrote:

Btw, to understand your setup - you're building compiler-rt/profile as part of building clang, via e.g. LLVM_ENABLE_PROJECTS (so it ends up being built with the existing host compiler, GCC) but then compile code using the clang and using the newly built compiler-rt/profile library with the newly built clang?

We're compiling the profile runtime for use with rustc. The runtime gets compiled using the host compiler and then linked with programs built by rustc with the newly built LLVM. Clang is never built.

In D108095#2945825, @mstorsjo wrote:

Sorry, I’m a bit slow today: I presume the situation is that this setup used to work fine, when doing profiled builds with clang, when linking with ld.bfd - but now it no longer does. But sanitizers have always required that they (the compiler-rt code) are built with clang (e.g. with LLVM_ENABLE_RUNTIMES, or entirely manually/separately) and linked with lld.

Is that a mingw-specific requirement? I don't think sanitizers for other platforms need to be be built with clang or linked with lld. We don't support sanitizers on mingw right now, so we may not have run into that issue.

[mstorsjo]

In D108095#2946115, @nikic wrote:

In D108095#2945822, @mstorsjo wrote:

Btw, to understand your setup - you're building compiler-rt/profile as part of building clang, via e.g. LLVM_ENABLE_PROJECTS (so it ends up being built with the existing host compiler, GCC) but then compile code using the clang and using the newly built compiler-rt/profile library with the newly built clang?

We're compiling the profile runtime for use with rustc. The runtime gets compiled using the host compiler and then linked with programs built by rustc with the newly built LLVM. Clang is never built.

Ah, right, that's even clearer. Thanks!

In D108095#2945825, @mstorsjo wrote:

Sorry, I’m a bit slow today: I presume the situation is that this setup used to work fine, when doing profiled builds with clang, when linking with ld.bfd - but now it no longer does. But sanitizers have always required that they (the compiler-rt code) are built with clang (e.g. with LLVM_ENABLE_RUNTIMES, or entirely manually/separately) and linked with lld.

Is that a mingw-specific requirement? I don't think sanitizers for other platforms need to be be built with clang or linked with lld. We don't support sanitizers on mingw right now, so we may not have run into that issue.

Yes, that's a mingw specific requirement. The sanitizers on windows are fairly new and only working in that minimal configuration (built with clang, linked with lld - or in full MSVC/clang-cl environments, where they can be linked with MS link.exe too).

[mstorsjo]

LGTM, thanks for the fix. (I finished testing both variants of this for myself now.)

Also I see this needs to be backported to the 13.x release branch (together with the previous fix which is reverted).

compiler-rt/lib/profile/CMakeLists.txt

set(EXTRA_FLAGS
      ${EXTRA_FLAGS}
      -DCOMPILER_RT_HAS_UNAME=1)
 endif()
 
 # We don't use the C++ Standard Library here, so avoid including it by mistake.
 append_list_if(COMPILER_RT_HAS_NOSTDINCXX_FLAG -nostdinc++ EXTRA_FLAGS)
 # XRay uses C++ standard library headers.
 string(REGEX REPLACE "-stdlib=[a-zA-Z+]*" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})
-# The Windows specific code uses a #pragma comment(linker, ...) which requires
-# -fms-extensions on MinGW targets.
-append_list_if(MINGW -fms-extensions EXTRA_FLAGS)
 
 # This appears to be a C-only warning banning the use of locals in aggregate
 # initializers. All other compilers accept this, though.
 # nonstandard extension used : 'identifier' : cannot be initialized using address of automatic variable
 append_list_if(COMPILER_RT_HAS_WD4221_FLAG /wd4221 EXTRA_FLAGS)
 
 if(APPLE)
   add_compiler_rt_runtime(clang_rt.profile

compiler-rt/lib/profile/InstrProfilingFile.c

 #elif defined(__ELF__) || defined(_WIN32)
 
 #define INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR                            \
   INSTR_PROF_CONCAT(INSTR_PROF_PROFILE_COUNTER_BIAS_VAR, _default)
 intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR = 0;
 
 /* This variable is a weak external reference which could be used to detect
  * whether or not the compiler defined this symbol. */
-#if defined(_WIN32)
+#if defined(_MSC_VER)
 COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
 #if defined(_M_IX86) || defined(__i386__)
 #define WIN_SYM_PREFIX "_"
 #else
 #define WIN_SYM_PREFIX
 #endif
 #pragma comment(                                                               \
     linker, "/alternatename:" WIN_SYM_PREFIX INSTR_PROF_QUOTE(                 \