Differential D99178
[libcxx] Disable c++experimental by default in DLL builds Authored by mstorsjo on Mar 23 2021, 5:06 AM.
Details c++experimental is always created as a static library, but if the Set LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY to OFF by default in such Remove the explicit LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF from the
Diff Detail
Event TimelineComment Actions I like the general improvement. It would be nice if we can better warn the user and guard against the invalid combination of options.
Comment Actions Explicitly error out on the cmake level if trying to enable the unsupported combination. Comment Actions 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.
Comment Actions 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.) Comment Actions 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? Comment Actions 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. Comment Actions 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? Comment Actions 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. Comment Actions 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 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.
Comment Actions 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.
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 Actions 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. Comment Actions 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.) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Nice. One suggestion "Windows, as the" -> "Windows. The".