Page MenuHomePhabricator

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

Authored by nikic on Aug 15 2021, 2:50 PM.

Details

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.

Diff Detail

Event Timeline

nikic created this revision.Aug 15 2021, 2:50 PM
nikic requested review of this revision.Aug 15 2021, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2021, 2:50 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

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.

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?

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.

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.

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.

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!

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 accepted this revision.Aug 16 2021, 4:21 AM

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).

This revision is now accepted and ready to land.Aug 16 2021, 4:21 AM
This revision was automatically updated to reflect the committed changes.