Page MenuHomePhabricator

[libcxx] [libcxxabi] Set flags for visibility when statically linking libcxxabi into libcxx for windows
ClosedPublic

Authored by mstorsjo on Oct 23 2020, 3:27 AM.

Details

Summary

Visibility attributes on windows (dllexport, dllimport) don't only have an effect when compiling the definitions of symbols, but also affect how undefined references to symbols behave (expecting them to live in the same DLL or not).

If we are building libcxxabi statically, with the intent to bundle it into the built libcxx (which is the only form that really works on windows, due to their interdependencies), we should have it flag its symbols as if it was being built in the same way as libcxx.

I.e., if libcxx is being built shared and libcxxabi is built static (to be linked into libcxx), libcxxabi should include dllexport on its symbols and don't import symbols from libcxx with dllimport as they are going to reside in the same DLL. And if building a static libcxx and going to bundle libcxxabi into it, it needs to access cxxabi symbols without dllimport attributes. (libcxxabi doesn't add #define _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS to any header like libcxx does, if configured for a static-only build.)

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 23 2020, 3:27 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 23 2020, 3:27 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
mstorsjo requested review of this revision.Oct 23 2020, 3:27 AM
mstorsjo added subscribers: thieta, mati865.
mati865 accepted this revision.Oct 25 2020, 3:55 AM

Sorry for the delay.
LGTM from MinGW point of view.

ldionne requested changes to this revision.Oct 26 2020, 9:51 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/CMakeLists.txt
879

Why does _LIBCXXABI_BUILDING_LIBRARY have an impact when building libc++?

libcxxabi/CMakeLists.txt
297

IMO, the right thing to do here is to get rid of the places where we #define _LIBCPP_BUILDING_LIBRARY in libcxxabi, and define _LIBCPP_BUILDING_LIBRARY unconditionally when building libc++abi. Then, as a follow-up change, we should clarify the status of these source files that are shared/duplicated between libc++ and libc++abi (that's stdlib_new_delete.cpp and stdlib_exception.cpp AFAICT).

This revision now requires changes to proceed.Oct 26 2020, 9:51 AM
rnk added a comment.Oct 26 2020, 10:25 AM

I read the commit message, and what you said makes sense to me. As I understand it, there are two supported configs on Windows:

  • Shared: libc++ and libc++abi as one DLL.
  • Static: both libc++ and libc++abi are static libraries.

Is it possible to simplify the cmake conditionals if we add some checks for that?

In D90021#2354009, @rnk wrote:

I read the commit message, and what you said makes sense to me. As I understand it, there are two supported configs on Windows:

  • Shared: libc++ and libc++abi as one DLL.
  • Static: both libc++ and libc++abi are static libraries.

... with an option in libcxx to merge the static libc++abi into the static libc++

But yeah, if using both libraries together, this pretty much are the only working combinations. Additionally, there's also the option to use libc++ standalone on top of MSVC's ABI runtime, both as static and dynamic library.

Is it possible to simplify the cmake conditionals if we add some checks for that?

I think that might be possible yeah, but not sure how big variance there is in how this all is hooked up, as most existing setups probably have done more or less of adhoc hackery to get it to work. (My builds so far amounted to building them each standalone, by passing these flags manually with CMAKE_CXX_FLAGS.)

libcxx/CMakeLists.txt
879

In practice, _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS works essentially the same. If there's inline definitions in headers that would need to be exported, I think _LIBCXXABI_BUILDING_LIBRARY would be a bit more consistent (as the statically linked libcxxabi would essentially be a part of the final libc++ DLL), but I'm not sure it matters.

libcxxabi/CMakeLists.txt
297

fallback_malloc.cpp also has an instance of it.

mstorsjo added inline comments.Oct 26 2020, 1:46 PM
libcxx/CMakeLists.txt
879

Phrasing it differently: If nothing is defined, any reference to symbols provided by libcxxabi, in code generated within libcxx, is generated as if it is going to be imported from a different DLL. If _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS is defined, the references are as if the code is going to be within the same DLL (which is true).

If _LIBCXXABI_BUILDING_LIBRARY is defined, any reference to the symbols are as if they are in the same DLL (same as with _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS), but any time such a symbol is defined (which only would happen for inline functions in headers), it also generates an annotation in the object file, saying that this particular symbol should be exported out from the DLL.

ldionne added inline comments.Oct 28 2020, 12:39 PM
libcxx/CMakeLists.txt
879

My point was that _LIBCXXABI_BUILDING_LIBRARY is not used anywhere in libc++, except when checking whether to enable availability attributes on Apple (which is unrelated to what you're trying to achieve). So adding this definition is effectively a no-op. Am I mistaken?

libcxxabi/CMakeLists.txt
297

Right. I would still prefer that we go down the route I laid out above, since it simplifies things in addition to making them work for your use case.

mstorsjo added inline comments.Oct 28 2020, 2:02 PM
libcxx/CMakeLists.txt
879

The libc++ code itself doesn't check _LIBCXXABI_BUILDING_LIBRARY, but it does affect what _LIBCXXABI_FUNC_VIS expands to, in the libcxxabi headers that are included by the libc++ sources.

If nothing is defined, _LIBCXXABI_FUNC_VIS expands to __declspec(dllimport), which makes the generated libc++ code reference e.g. __imp___cxa_uncaught_exceptions instead of __cxa_uncaught_exceptions directly. With _LIBCXXABI_BUILDING_LIBRARY defined, it expands to __declspec(dllexport) instead, and the generated code references __cxa_uncaught_exceptions, likewise with _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS.

libcxxabi/CMakeLists.txt
297

Ok, I'll give that a shot.

mstorsjo updated this revision to Diff 302521.Nov 3 2020, 2:22 AM

Updated after landing D90476, removed nuances that don't have any practical effect (distinguishing between defining _LIBCXXABI_BUILDING_LIBRARY and _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS within libcxx), ending up with a fairly simple form.

ldionne accepted this revision.Nov 3 2020, 5:01 AM
This revision is now accepted and ready to land.Nov 3 2020, 5:01 AM