[libcxx] Disable c++experimental by default in DLL builds

[mstorsjo]

Summary

c++experimental is always created as a static library, but if the
libcxx headers indicate DLL linkage, linking against c++experimental
fails (with unresolved symbols).

Set LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY to OFF by default in such
configurations. If enabled anyway, mark the tests under std/experimental
as unsupported.

Remove the explicit LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF from the
examples for how to build the library.

[Mordante]

I like the general improvement. It would be nice if we can better warn the user and guard against the invalid combination of options.

libcxx/docs/BuildingLibcxx.rst

93

I assume things go wrong if the user uses -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=YES. Can you document this isn't supported? I think it would be nice if CMake doesn't allow this invalid combination.

[mstorsjo]

libcxx/docs/BuildingLibcxx.rst

93

I kind of wanted to leave it possible to do that still, while you'd essentially be on your own in that case. But I guess I could at least add a cmake warning for that combination - or a hard error if others also feel that way.

[Mordante]

libcxx/docs/BuildingLibcxx.rst

93

I'm mainly concerned with the "removing" of documentation. Before the documentation mentioned you shouldn't do -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=YES. After the patch is says nothing and using this will cause errors. So I think it's a good thing CMake does the right thing by default, but I dislike the removal of clarity.

I think we should add some documentation here the combination isn't supported.

In my personal opinion I think we should add a hard error in CMake, something like:

if (WIN32 AND LIBCXX_ENABLE_SHARED AND LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY)
    message(fatal_error "The libc++experimental isn't available for Windows shared libraries")
endif()

But if you rather show a warning I won't object.

[mstorsjo]

libcxx/docs/BuildingLibcxx.rst

93

I'd rather keep such details out of this part of the docs - there's still some amount of ifs and buts around this configuration, so it's better if we can reduce some amount of the noise in it at least, But I guess I can make it an error on the cmake level - that would at least make the situation clearer.

[mstorsjo]

Explicitly error out on the cmake level if trying to enable the unsupported combination.

[Mordante]

LGTM modulo a small suggestion. If you don't want to address that suggestion, please rebase to trigger a CI run. I'd like to see it builds without issues before giving my approval.

libcxx/CMakeLists.txt

413

Nice. One suggestion "Windows, as the" -> "Windows. The".

libcxx/docs/BuildingLibcxx.rst

93

Oke fair enough.

[mstorsjo]

libcxx/CMakeLists.txt

413

Good point, will fix.

[mstorsjo]

Updated an error message as suggested.

[mstorsjo]

Rebased past 9abff04e506f.

[mstorsjo]

In D99178#2668045, @Mordante wrote:

LGTM modulo a small suggestion. If you don't want to address that suggestion, please rebase to trigger a CI run. I'd like to see it builds without issues before giving my approval.

Now it has a green CI checkmark. As the Windows CI isn't merged yet, this patch didn't run a windows configuration though (but I ran D99093 through CI just now as well, and that one also is green) - and with this patch applied, one can just remove one case of -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO from the CI config, which gets set implicitly instead. (I've run a combination of these two through CI elsewhere too.)

[ldionne]

I'd like to suggest that we instead add a libcxx/cmake/caches/Windows.cmake file that contains those defaults. This would move us towards removing platform-specific knowledge from the CMake instead of towards it. WDYT?

[mstorsjo]

In D99178#2669529, @ldionne wrote:

I'd like to suggest that we instead add a libcxx/cmake/caches/Windows.cmake file that contains those defaults. This would move us towards removing platform-specific knowledge from the CMake instead of towards it. WDYT?

Can you elaborate on which defaults you'd like to have there? While having example setups as cmake caches can be useful, use of the cmake caches is very much optional, so logic for disabling it by default needs to be in the main CMakeLists.txt instead of in an optional cmake cache to me.

[mstorsjo]

Updated after landing the CI configuration.

[Mordante]

Approved to make clear all my comments have been addressed.

[ldionne]

In D99178#2669567, @mstorsjo wrote:

Can you elaborate on which defaults you'd like to have there? While having example setups as cmake caches can be useful, use of the cmake caches is very much optional, so logic for disabling it by default needs to be in the main CMakeLists.txt instead of in an optional cmake cache to me.

What we're trying to achieve with the libc++/libc++abi CMakes is to slowly divorce them from platform-specific logic. Instead, we use CMake caches to configure the library properly for a given use/platform. For example, I'd rather have a Windows.cmake cache that sets the following:

set(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY OFF CACHE BOOL "") # optionally explain why

That way, you don't need any of the CMake or Lit logic added by this patch, you only need to use the right cache file when building.

Essentially, I'd like the libc++ CMake to be simple and contain generic platform agnostic knobs, which can then be turned on/off as needed on different platforms and for different use cases. WDYT?

[mstorsjo]

In D99178#2674471, @ldionne wrote:

In D99178#2669567, @mstorsjo wrote:

Can you elaborate on which defaults you'd like to have there? While having example setups as cmake caches can be useful, use of the cmake caches is very much optional, so logic for disabling it by default needs to be in the main CMakeLists.txt instead of in an optional cmake cache to me.

What we're trying to achieve with the libc++/libc++abi CMakes is to slowly divorce them from platform-specific logic. Instead, we use CMake caches to configure the library properly for a given use/platform. For example, I'd rather have a Windows.cmake cache that sets the following:

set(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY OFF CACHE BOOL "") # optionally explain why

That way, you don't need any of the CMake or Lit logic added by this patch, you only need to use the right cache file when building.

Essentially, I'd like the libc++ CMake to be simple and contain generic platform agnostic knobs, which can then be turned on/off as needed on different platforms and for different use cases. WDYT?

I agree that avoiding platform specific logic in the main cmakefiles would be nice, but I disagree that cmake cache files are the solution here.

Cmake caches capture individual possible ways that you _can_ configure and build the library, but they aren't the only allowed way to do it, as there's far more possible configurations than is sensible to enumerate.

For this particular case, procedural logic in cmake captures it much better.

If building for windows as a DLL, the experimental library should be implicitly disabled if the user expressed no opinion on the matter. If the user requested the library to be enabled, we should tell him why that can't be done. If building for windows but as a static library only, the experimental library works fine and can be enabled by default.

I'm quite against trying to shoehorn all allowed build configurations into a fixed list of configurations. Sure, we can't guarantee that any random configuration not covered by CI will work though, but building without a presupplied cmake cache should definitely be ok and work.

[ldionne]

In D99178#2674584, @mstorsjo wrote:

I agree that avoiding platform specific logic in the main cmakefiles would be nice, but I disagree that cmake cache files are the solution here.

Cmake caches capture individual possible ways that you _can_ configure and build the library, but they aren't the only allowed way to do it, as there's far more possible configurations than is sensible to enumerate.

For this particular case, procedural logic in cmake captures it much better.

If building for windows as a DLL, the experimental library should be implicitly disabled if the user expressed no opinion on the matter. If the user requested the library to be enabled, we should tell him why that can't be done. If building for windows but as a static library only, the experimental library works fine and can be enabled by default.

For the record, the logic added in this patch technically doesn't work, because LIBCXX_ENABLE_SHARED and LIBCXX_ENABLE_STATIC are not mutually exclusive. So, for example, if you set LIBCXX_ENABLE_SHARED=ON but also LIBCXX_ENABLE_STATIC=ON, you might want to have the experimental library available for use with the static version of libc++ but not the shared version. I don't see a way to make that possible without adding an option like LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY_FOR_STATIC_LIBRARY and LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY_FOR_SHARED_LIBRARY, and having only the shared one be defaulted to OFF on Windows.

That sort of complexity is exactly the sort of thing that we must not add.

I'm quite against trying to shoehorn all allowed build configurations into a fixed list of configurations. Sure, we can't guarantee that any random configuration not covered by CI will work though, but building without a presupplied cmake cache should definitely be ok and work.

I understand the appeal to have things work out of the box. I think it can be achieved. What if we loaded a CMake cache by default based on the platform we're building for first thing in the CMake? For example:

#===============================================================================
# Setup Project
#===============================================================================
cmake_minimum_required(VERSION 3.13.4)

set(CMAKE_BUILD_WITH_INSTALL_NAME_DIR ON)
...

if (WIN32)
  include(cmake/caches/Windows.cmake)
elseif(APPLE)
  include(cmake/caches/Apple.cmake)
else()
  ...
endif()

We'd also need a way to completely skip those if CMake is called with -C <path-to-some-cache-file>. If we had something along those lines, we could hide platform specific knowledge into those files and avoid adding complexity to our CMake.

libcxx/test/libcxx/experimental/lit.local.cfg

6–7 ↗

(On Diff #335322)

We already have a way to exclude the experimental tests from the test suite, and it's not acceptable to add another one based on platform specific knowledge. Lit can be passed --param enable_experimental=False for that purpose. Whatever we end up doing, we must find a way to pass --param enable_experimental=False to Lit when the configuration you're dealing with is used, without having to add that knowledge inside the test suite. Note that this is what LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF does.

[mstorsjo]

In D99178#2674713, @ldionne wrote:

For the record, the logic added in this patch technically doesn't work, because LIBCXX_ENABLE_SHARED and LIBCXX_ENABLE_STATIC are not mutually exclusive. So, for example, if you set LIBCXX_ENABLE_SHARED=ON but also LIBCXX_ENABLE_STATIC=ON, you might want to have the experimental library available for use with the static version of libc++ but not the shared version. I don't see a way to make that possible without adding an option like LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY_FOR_STATIC_LIBRARY and LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY_FOR_SHARED_LIBRARY, and having only the shared one be defaulted to OFF on Windows.

That sort of complexity is exactly the sort of thing that we must not add.

I agree that we really shouldn't go down the path of adding something like LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY_FOR_STATIC_LIBRARY.

However the observation that the patch doesn't work for the case with LIBCXX_ENABLE_SHARED enabled together with LIBCXX_ENABLE_STATIC (which is the default configuration) isn't correct.

The problem that a static c++experimental doesn't work when libc++ itself is built shared, is that the headers signal every declaration with __declspec(dllimport). When building static-only, __config_site defines _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS which inhibits the __declspec(dllimport). Now if building with both LIBCXX_ENABLE_SHARED and LIBCXX_ENABLE_STATIC, the headers always produce declarations with __declspec(dllimport), which makes the static version of libc++ itself also fairly non-working. (But for both most regular users, and the testsuite, they'd end up linking the dynamic version of the library anyway if both are available.)

So if both static and shared are enabled, the issue with not being able to use the static-only c++experimental remains, and this patch works as intended.

I'm quite against trying to shoehorn all allowed build configurations into a fixed list of configurations. Sure, we can't guarantee that any random configuration not covered by CI will work though, but building without a presupplied cmake cache should definitely be ok and work.

I understand the appeal to have things work out of the box. I think it can be achieved. What if we loaded a CMake cache by default based on the platform we're building for first thing in the CMake? For example:

#===============================================================================
# Setup Project
#===============================================================================
cmake_minimum_required(VERSION 3.13.4)

set(CMAKE_BUILD_WITH_INSTALL_NAME_DIR ON)
...

if (WIN32)
  include(cmake/caches/Windows.cmake)
elseif(APPLE)
  include(cmake/caches/Apple.cmake)
else()
  ...
endif()

We'd also need a way to completely skip those if CMake is called with -C <path-to-some-cache-file>. If we had something along those lines, we could hide platform specific knowledge into those files and avoid adding complexity to our CMake.

So, if the end goal is just to move code away from the main cmake file, then sure, this works. At least if such a cache file allows imperative logic as in a regular cmake file and not just plain declarations of what the settings are. It works for setting certain default configurations, but e.g. for platform specific logic later in the cmake file, it doesn't help much - then you'd need to include e.g. an Windows-Check-Effective-Config.cmake and Windows-Set-Defines.cmake.

Consider e.g. these bits that we have at the very bottom of the file today:

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

if (WIN32 AND LIBCXX_ENABLE_STATIC_ABI_LIBRARY)
  # If linking libcxxabi statically into libcxx, skip the dllimport attributes
  # on symbols we refer to from libcxxabi.
  add_definitions(-D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS)
endif()

So this works fine with written logic with if statements like this, but if we'd be splitting it out into individual cmake cache files for different potential configurations, like Windows-<abi_library>-<link_abi_library_statically?>.cmake etc, we'd have combinatorial explosion.

libcxx/test/libcxx/experimental/lit.local.cfg

6–7 ↗

(On Diff #335322)

Sure. This was a remnant from an earlier approach of this patch (where I just set the default to OFF here, but allowed the user to override it and enable it anyway) - I wanted to have the tests be skipped if the user willingly enabled the broken configuration. But it can indeed be dropped, as just disabling the whole experimental library is enough.

[mstorsjo]

Removed the lit.local.cfg changes that aren't needed, as we shouldn't end up with the experimental library enabled at all in those configurations now.

[mstorsjo]

Rebased on top of other Windows CI updates.

[mstorsjo]

Rerun CI

[philnik]

Is there still interest in pursuing this? If not, could you abandon the patch?

[mstorsjo]

The handling of the libc++experimental library was changed (in a different direction) last year, so this patch is indeed stale at the moment. (The root issue, that the static-only libc++experimental doesn't work when the main libc++ headers flag symbols as dllimport, remains though.)

libcxx/CMakeLists.txt

 #===============================================================================
 include(CMakeDependentOption)
 include(HandleCompilerRT)
 
 # Basic options ---------------------------------------------------------------
 option(LIBCXX_ENABLE_ASSERTIONS "Enable assertions independent of build mode." OFF)
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
-option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" ON)
+set(ENABLE_EXPERIMENTAL_DEFAULT ON)
+if (WIN32 AND LIBCXX_ENABLE_SHARED)
+  # When libc++ is built as a DLL, all headers indicate DLL linkage, while
+  # libc++experimental always is linked statically.
+  set(ENABLE_EXPERIMENTAL_DEFAULT OFF)
+endif()
+option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" ${ENABLE_EXPERIMENTAL_DEFAULT})
 set(ENABLE_FILESYSTEM_DEFAULT ON)
 if (WIN32 AND NOT MINGW)
   # Filesystem is buildable for windows, but it requires __int128 helper
   # functions, that currently are provided by libgcc or compiler_rt builtins.
   # These are available in MinGW environments, but not currently in MSVC
   # environments.
   set(ENABLE_FILESYSTEM_DEFAULT OFF)
 endif()
if (LIBCXX_ABI_FORCE_ITANIUM AND LIBCXX_ABI_FORCE_MICROSOFT)
   message(FATAL_ERROR "Only one of LIBCXX_ABI_FORCE_ITANIUM and LIBCXX_ABI_FORCE_MICROSOFT can be specified.")
 endif ()
 
 #===============================================================================
 # Configure System
 #===============================================================================
 
 if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
   set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++)
   set(LIBCXX_HEADER_DIR ${LLVM_BINARY_DIR})
   set(LIBCXX_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++)
   if(LIBCXX_LIBDIR_SUBDIR)
     string(APPEND LIBCXX_LIBRARY_DIR /${LIBCXX_LIBDIR_SUBDIR})
     string(APPEND LIBCXX_INSTALL_LIBRARY_DIR /${LIBCXX_LIBDIR_SUBDIR})
   endif()
 elseif(LLVM_LIBRARY_OUTPUT_INTDIR)
   set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})

libcxx/docs/BuildingLibcxx.rst

 it is the simplest way to build.
 
 .. code-block:: batch
 
   > cmake -G "Visual Studio 16 2019"              ^
           -T "ClangCL"                            ^
           -DLIBCXX_ENABLE_SHARED=YES              ^
           -DLIBCXX_ENABLE_STATIC=NO               ^
-          -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO ^
           \path\to\libcxx
   > cmake --build .
 
 CMake + ninja (MSVC)
 ~~~~~~~~~~~~~~~~~~~~
 
 Building with ninja is required for development to enable tests.
 Running the tests also requires a Bash shell and Python to be available.

 In either case, then run:
 
 .. code-block:: batch
 
   > cmake -G Ninja                                                                    ^
           -DCMAKE_BUILD_TYPE=Release                                                  ^
           -DCMAKE_C_COMPILER=clang-cl                                                 ^
           -DCMAKE_CXX_COMPILER=clang-cl                                               ^
-          -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO                                     ^
           path/to/libcxx
   > ninja cxx
   > ninja check-cxx
 
 If you are running in an MSYS2 shell and you have installed the
 MSYS2-provided clang package (which defaults to a non-MSVC target), you
 should add e.g. ``-DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc`` (replacing
 ``x86_64`` with the architecture you're targeting) to the ``cmake`` command

libcxx/test/std/experimental/lit.local.cfg

 # Disable all of the experimental tests if the correct feature is not available.
 if 'c++experimental' not in config.available_features:
   config.unsupported = True
+# If built as a DLL on Windows, headers indicate DLL linkage, which breaks
+# linking against the static-only libc++experimental.
+if config.enable_shared and 'windows' in config.available_features:
+  config.unsupported = True