[libcxx-commits] [PATCH] D99178: [libcxx] Disable c++experimental by default in DLL builds

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

In D99178#2674713 <https://reviews.llvm.org/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.



================
Comment at: libcxx/test/libcxx/experimental/lit.local.cfg:6-7
+# linking against the static-only libc++experimental.
+if config.enable_shared and 'windows' in config.available_features:
+  config.unsupported = True
----------------
ldionne wrote:
> 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.
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99178/new/

https://reviews.llvm.org/D99178