[libcxx] Don't treat Windows specially with visibility annotations

[phosek]

Summary

There's no reason to treat Windows specially, users of libc++ can define
_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS just like they would on other
platforms.

See http://lists.llvm.org/pipermail/libcxx-dev/2020-October/000978.html
for more details.

[phosek]

According to git blame this change was authored by @smeenai in rG546442160881b, @smeenai do you know what was the motivation for having a special handling on Windows?

[smeenai]

I think the reason for this restriction is that on Windows, __declspec(dllimport) changes the compiler's code generation to assume that the symbol will be imported (i.e. if the compiler sees a call to a function X which is marked __declspec(dllimport), it'll generate an indirect call to __imp_X instead). If you're linking against libc++ statically, the imported version of the symbol will of course not exist, so you want to disable the __declspec(dllimport) annotations by default if you're building just the static library for Windows.

(IIRC, Microsoft's linker actually supports this case; it'll generate a dummy __imp_X for you pointing to the local X symbol, but it'll also give you a warning about doing so. I think LLD COFF supports that as well. It's just less clean to do it that way.)

Since this is Windows-specific behavior (or technically dllimport-specific behavior), perhaps it makes more sense to do some Windows-specific visibility macro adjustments than overloading the cross-platform _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS for this purpose?

[ldionne]

I'm happy with removing this special case, especially since the linker handles it. I believe it makes more sense for a vendor shipping the static archive to disable these annotations explicitly instead.

[mstorsjo]

I'm a "vendor" building libc++ for windows, both statically and dynamically, and without this, I'll need to manually inject a #define _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS into the installed headers somehow.

At least in my experience, code generation with headers lacking dllimport attributes, but linking against an import library for a DLL, works much better than building user code with dllimport but linking against a static library. (Yes, the linker can sometimes generate the __imp_ stubs for dllimport linking against oneself, but iirc it doesn't work flawlessly for things like libc++.)

Also cc @thieta

[mstorsjo]

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

[mstorsjo]

So to be very clear, I'd prefer this not to be committed, as it will break my build setup, and the way for me to work around it isn't very pretty.

A different potential way forward would be to default to not using visibility annotations on windows, making them opt-in instead of opt-out. Because at least in my experience in mingw configurations, mismatches between actual libc++ vs option set in the user's code works much better if the visibility annotations are disabled. IIRC @thieta just ran into the same issue a few weeks ago as well and concluded the same.

[ldionne]

In D88843#2312799, @mstorsjo wrote:

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

Let's add a proper LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS option and config_define_if(LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) like we do for other __config_site macros. Then, you can configure your build with -DLIBCXX_DISABLE_VISIBILITY_ANNOTATIONS and remove the platform specific logic from libc++'s CMake.

WDYT?

[mstorsjo]

In D88843#2312846, @ldionne wrote:

In D88843#2312799, @mstorsjo wrote:

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

Let's add a proper LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS option and config_define_if(LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) like we do for other __config_site macros. Then, you can configure your build with -DLIBCXX_DISABLE_VISIBILITY_ANNOTATIONS and remove the platform specific logic from libc++'s CMake.

WDYT?

That sounds good to me and would be easy to configure going forward.

Not sure how well it works in practice with @phosek 's original usecase from http://lists.llvm.org/pipermail/libcxx-dev/2020-October/000978.html, wanting to use one single __config_site for libc++ for multiple targets at once though.

[phosek]

In D88843#2312850, @mstorsjo wrote:

In D88843#2312846, @ldionne wrote:

In D88843#2312799, @mstorsjo wrote:

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

Let's add a proper LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS option and config_define_if(LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) like we do for other __config_site macros. Then, you can configure your build with -DLIBCXX_DISABLE_VISIBILITY_ANNOTATIONS and remove the platform specific logic from libc++'s CMake.

WDYT?

That sounds good to me and would be easy to configure going forward.

Not sure how well it works in practice with @phosek 's original usecase from http://lists.llvm.org/pipermail/libcxx-dev/2020-October/000978.html, wanting to use one single __config_site for libc++ for multiple targets at once though.

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

[ldionne]

In D88843#2312944, @phosek wrote:

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

[phosek]

In D88843#2312961, @ldionne wrote:

In D88843#2312944, @phosek wrote:

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

__config_site is the only file that differs across targets, but we always concat it with __config so I assumed we're going to have a per-target copy of __config, but we could make this even granular by keeping them separate and using #include <__config_site> guarded by #if __has_include(<__config_site>) ... #endif inside __config. Would that be preferable?

[thieta]

A compile time option sounds good, per target configuration sounds even better!

[ldionne]

In D88843#2313287, @phosek wrote:

In D88843#2312961, @ldionne wrote:

In D88843#2312944, @phosek wrote:

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

__config_site is the only file that differs across targets, but we always concat it with __config so I assumed we're going to have a per-target copy of __config, but we could make this even granular by keeping them separate and using #include <__config_site> guarded by #if __has_include(<__config_site>) ... #endif inside __config. Would that be preferable?

Actually, I have a patch that changes libc++ to use #include <__config_site>. I've been wanting to do that for a while. Let me put it up for review.

[phosek]

In D88843#2315203, @ldionne wrote:

In D88843#2313287, @phosek wrote:

In D88843#2312961, @ldionne wrote:

In D88843#2312944, @phosek wrote:

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

__config_site is the only file that differs across targets, but we always concat it with __config so I assumed we're going to have a per-target copy of __config, but we could make this even granular by keeping them separate and using #include <__config_site> guarded by #if __has_include(<__config_site>) ... #endif inside __config. Would that be preferable?

Actually, I have a patch that changes libc++ to use #include <__config_site>. I've been wanting to do that for a while. Let me put it up for review.

I have uploaded my change as D89013, which in turn depends on D83911 and D88452. Once your change is up for review, I'll rebase my change on top of it.

[ldionne]

Ping, what are we doing with this?

[phosek]

I don't think we need this anymore.

libcxx/CMakeLists.txt

if (NOT abi_define MATCHES "^_LIBCPP_ABI_")
       message(SEND_ERROR "Invalid ABI macro ${abi_define} in LIBCXX_ABI_DEFINES")
     endif()
     list(APPEND abi_defines "#define ${abi_define}")
   endforeach()
   string(REPLACE ";" "\n" abi_defines "${abi_defines}")
   config_define(${abi_defines} _LIBCPP_ABI_DEFINES)
 endif()
 
-# By default libc++ on Windows expects to use a shared library, which requires
-# the headers to use DLL import/export semantics. However when building a
-# static library only we modify the headers to disable DLL import/export.
-if (DEFINED WIN32 AND LIBCXX_ENABLE_STATIC AND NOT LIBCXX_ENABLE_SHARED)
-  message(STATUS "Generating custom __config for non-DLL Windows build")
-  config_define(ON _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-endif()
-
 # Setup all common build flags =================================================
 function(cxx_add_common_build_flags target)
   cxx_add_basic_build_flags(${target})
   cxx_add_warning_flags(${target})
   cxx_add_windows_flags(${target})
   cxx_add_exception_flags(${target})
   cxx_add_rtti_flags(${target})
   cxx_add_module_flags(${target})