Page MenuHomePhabricator

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

Authored by mstorsjo on Mar 23 2021, 5:06 AM.

Details

Reviewers
ldionne
Mordante
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 23 2021, 5:06 AM
mstorsjo requested review of this revision.Mar 23 2021, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 5:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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 added inline comments.Mar 31 2021, 12:28 PM
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 added inline comments.Apr 1 2021, 9:13 AM
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 added inline comments.Apr 1 2021, 2:13 PM
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 updated this revision to Diff 334827.Apr 1 2021, 2:20 PM

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

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 added inline comments.Apr 4 2021, 9:56 AM
libcxx/CMakeLists.txt
413

Good point, will fix.

mstorsjo updated this revision to Diff 335155.Apr 4 2021, 10:01 AM

Updated an error message as suggested.

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 requested changes to this revision.Apr 5 2021, 1:51 PM
ldionne added a subscriber: 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?

This revision now requires changes to proceed.Apr 5 2021, 1:51 PM

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 this revision to Diff 335322.Apr 5 2021, 2:11 PM

Updated after landing the CI configuration.

Mordante accepted this revision as: Mordante.Apr 7 2021, 10:13 AM

Approved to make clear all my comments have been addressed.

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?

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.

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.

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 updated this revision to Diff 335907.Apr 7 2021, 12:37 PM

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.