Page MenuHomePhabricator

[libcxx] Base MSVC autolinking on _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS
ClosedPublic

Authored by mstorsjo on Apr 15 2021, 3:00 AM.

Details

Summary

Previously the decision of which library to try to autolink was
based on _DLL, however the _DLL define (which is set by the compiler)
is tied to whether using a dynamically linked CRT or not, and the choice
of dynamic or static CRT is entirely orthogonal to whether libc++ is
linked dynamically or statically.

If _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS isn't defined, then all
declarations are decorated with dllimport, and there's no doubt that
the DLL version of the library is what must be linked.

_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is defined if building with
LIBCXX_ENABLE_SHARED disabled, and thus the static library is what
should be linked.

If defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS manually but wanting
to link against the DLL version of the library, that's not a canonical
configuration, and then it's probably reasonable to manually define
_LIBCPP_NO_AUTO_LINK too, and manually link against the desired
library.

This fixes, among other issues, running tests for the library if
built with LIBCXX_ENABLE_STATIC disabled.

This issue was brought up when autolinking was introduced in
https://reviews.llvm.org/D40660, but was shrugged off back then,
but it is causing real issues, and instead of avoiding the issue
by disabling the autolinking feature, we should fix it to actually
match how the library was built.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Apr 15 2021, 3:00 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 3:00 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rnk accepted this revision.Apr 15 2021, 11:44 AM

lgtm

I agree this makes sense, it's reasonable to statically link libc++ and dynamically link the CRT.

I think this is Windows-specific, so my approval is enough, but I could be wrong.


In Chrome, we define _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS in all libc++ static build configs:
https://source.chromium.org/chromium/chromium/src/+/master:build/config/c++/BUILD.gn;l=57?q=_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS&ss=chromium
I see that libc++ appears to support building both static and dynamic libraries in the same build:
https://github.com/llvm/llvm-project/blob/280678122d3175943e41310d56a17924ea38ffc1/libcxx/CMakeLists.txt#L89-L90
That makes it impossible to bake this visibility setting into __config_site. Otherwise, I would suggest that we do that, and make the Windows behavior the cross-platform behavior. Then we would rename this macro to something like _LIBCPP_STATIC_LIBRARY or something like that.

I see that libc++ appears to support building both static and dynamic libraries in the same build:
https://github.com/llvm/llvm-project/blob/280678122d3175943e41310d56a17924ea38ffc1/libcxx/CMakeLists.txt#L89-L90
That makes it impossible to bake this visibility setting into __config_site. Otherwise, I would suggest that we do that, and make the Windows behavior the cross-platform behavior. Then we would rename this macro to something like _LIBCPP_STATIC_LIBRARY or something like that.

libcxx actually already does bake _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS into __config_site if LIBCXX_ENABLE_SHARED isn't set.

And yes, while one _can_ build both static and shared at the same time, the static library can't really be used practically in such configs. But that's an orthogonal issue (that I'm not planning on doing anything about really, as there's no really neat solutions that I see).

mstorsjo updated this revision to Diff 337869.Apr 15 2021, 12:27 PM

Rebase to rerun CI

mstorsjo updated this revision to Diff 337876.Apr 15 2021, 12:46 PM

Rerunning CI yet again after another fix on main for clang-format

smeenai accepted this revision.Apr 15 2021, 12:48 PM

LGTM as well.

curdeius accepted this revision as: curdeius.Apr 15 2021, 1:30 PM
curdeius added a subscriber: curdeius.

LGTM.

Ping for second approval

Mordante accepted this revision.Apr 19 2021, 9:07 AM
Mordante added a subscriber: Mordante.

LGTM.

This revision is now accepted and ready to land.Apr 19 2021, 9:07 AM