mstorsjo added inline comments.
================
Comment at: libcxx/CMakeLists.txt:250
+
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt
libstdc++ libsupc++ vcruntime)
+set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++
ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")
----------------
ldionne wrote:
> phosek wrote:
> > All of these values refer to system ABI libraries except for libcxxabi
> > where `libcxxabi is the in-tree one and `system-libcxxabi` is the system
> > one. From a consistency perspective, I think it'd be better for `libcxxabi`
> > to also refer to a system ABI library, and then use a different name for
> > the in-tree one, perhaps `default`?
> I agree with the consistency argument. However, I'd like to avoid `default`
> since it doesn't convey any information, and it also used to be one of the
> valid values but it represented "whatever automatically selected ABI we
> pick", which might not be libc++abi. I'd suggest `libcxxabi` and
> `intree-libcxxabi`. It's not extremely pretty, but it conveys exactly what it
> is.
>
> Edit: I actually went ahead and made this change, and I realized that it
> might be breaking for a bunch of people. Indeed, I think that most people who
> specify `libcxxabi` are expecting to build against the in-tree one, which is
> what happens today as long as they have `libcxxabi` in their
> `LLVM_ENABLE_RUNTIMES`. With this renaming, they would implicitly start
> building against the system libc++abi, and that might happen silently. I'm
> not sure I like this. So, here's the diff to implement this change, however
> I'd rather not apply it unless you think consistency is more important that
> this concern:
>
> ```
> diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
> index a7f1684235e6..7cbf8957ac90 100644
> --- a/libcxx/CMakeLists.txt
> +++ b/libcxx/CMakeLists.txt
> @@ -244,10 +244,10 @@ if (LIBCXX_TARGETING_MSVC)
> elseif (${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD")
> set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxrt")
> else()
> - set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxabi")
> + set(LIBCXX_DEFAULT_ABI_LIBRARY "intree-libcxxabi")
> endif()
>
> -set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt
> libstdc++ libsupc++ vcruntime)
> +set(LIBCXX_SUPPORTED_ABI_LIBRARIES none intree-libcxxabi libcxxabi libcxxrt
> libstdc++ libsupc++ vcruntime)
> set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++
> ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")
>
> # Temporary to still accept existing CMake caches that contain "default" as
> the value
> diff --git a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> index 6e7a53258c0a..f22cfd674623 100644
> --- a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> +++ b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> @@ -97,7 +97,7 @@ add_library(libcxx-abi-headers INTERFACE)
> import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}"
> supc++)
>
> # Link against the in-tree libc++abi
> -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "intree-libcxxabi")
> add_library(libcxx-abi-headers INTERFACE)
> target_link_libraries(libcxx-abi-headers INTERFACE cxxabi-headers)
> target_compile_definitions(libcxx-abi-headers INTERFACE
> "-DLIBCXX_BUILDING_LIBCXXABI")
> @@ -111,7 +111,7 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> endif()
>
> # Link against a system-provided libc++abi
> -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
> +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> add_library(libcxx-abi-headers INTERFACE)
> import_private_headers(libcxx-abi-headers
> "${LIBCXX_CXX_ABI_INCLUDE_PATHS}" "cxxabi.h;__cxxabi_config.h")
> target_compile_definitions(libcxx-abi-headers INTERFACE
> "-DLIBCXX_BUILDING_LIBCXXABI")
> diff --git a/libcxx/cmake/caches/AIX.cmake b/libcxx/cmake/caches/AIX.cmake
> index 029b8deae3d7..e3f31aea5192 100644
> --- a/libcxx/cmake/caches/AIX.cmake
> +++ b/libcxx/cmake/caches/AIX.cmake
> @@ -13,4 +13,4 @@ set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
> set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
> set(LIBCXXABI_ENABLE_SHARED ON CACHE BOOL "")
> set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
> -set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
> +set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING "")
> diff --git a/libcxx/cmake/caches/Apple.cmake b/libcxx/cmake/caches/Apple.cmake
> index 4581d4d87b80..89087662b2f4 100644
> --- a/libcxx/cmake/caches/Apple.cmake
> +++ b/libcxx/cmake/caches/Apple.cmake
> @@ -7,7 +7,7 @@ set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
> set(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY OFF CACHE BOOL "")
> set(LIBCXX_ENABLE_STATIC ON CACHE BOOL "")
> set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
> -set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
> +set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING "")
> set(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT ON CACHE BOOL "")
> set(LIBCXX_ENABLE_DEBUG_MODE_SUPPORT OFF CACHE BOOL "")
> set(LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS ON CACHE BOOL "")
> diff --git a/libcxx/cmake/caches/MinGW.cmake b/libcxx/cmake/caches/MinGW.cmake
> index 14b887e64101..44a3899d025d 100644
> --- a/libcxx/cmake/caches/MinGW.cmake
> +++ b/libcxx/cmake/caches/MinGW.cmake
> @@ -1,6 +1,6 @@
> set(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY OFF CACHE BOOL "")
>
> -set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
> +set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING "")
> set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
>
> set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
> diff --git a/libcxx/docs/BuildingLibcxx.rst b/libcxx/docs/BuildingLibcxx.rst
> index 3f4e314382d8..cf42eadb6deb 100644
> --- a/libcxx/docs/BuildingLibcxx.rst
> +++ b/libcxx/docs/BuildingLibcxx.rst
> @@ -323,7 +323,7 @@ ABI Library Specific Options
>
> .. option:: LIBCXX_CXX_ABI:STRING
>
> - **Values**: ``none``, ``libcxxabi``, ``system-libcxxabi``, ``libcxxrt``,
> ``libstdc++``, ``libsupc++``, ``vcruntime``.
> + **Values**: ``none``, ``intree-libcxxabi``, ``libcxxabi``, ``libcxxrt``,
> ``libstdc++``, ``libsupc++``, ``vcruntime``.
>
> Select the ABI library to build libc++ against.
>
> diff --git a/libcxx/docs/ReleaseNotes.rst b/libcxx/docs/ReleaseNotes.rst
> index cbbe6c06ef40..64d02b9e8dfd 100644
> --- a/libcxx/docs/ReleaseNotes.rst
> +++ b/libcxx/docs/ReleaseNotes.rst
> @@ -139,3 +139,8 @@ Build System Changes
> or on a platform that used to be supported by the legacy testing
> configuration and isn't supported
> by one of the configurations in ``libcxx/test/configs``, please reach out
> to the libc++ developers
> to get your configuration supported officially.
> +
> +- If you previously specified ``LIBCXX_CXX_ABI=libcxxabi`` to build against
> the in-tree version of
> + libc++abi, please switch to ``LIBCXX_CXX_ABI=intree-libcxxabi``. Starting
> with this release,
> + ``LIBCXX_CXX_ABI=libcxxabi`` will now build against the system-provided
> libc++abi, for consistency
> + with the naming of other ABI library choices.
> diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
> index 41d07d15f03c..fa6f44f0e19f 100644
> --- a/libcxx/src/CMakeLists.txt
> +++ b/libcxx/src/CMakeLists.txt
> @@ -243,7 +243,7 @@ if (LIBCXX_ENABLE_SHARED)
> # Maybe re-export symbols from libc++abi
> # In particular, we don't re-export the symbols if libc++abi is merged
> statically
> # into libc++ because in that case there's no dylib to re-export from.
> - if (APPLE AND LIBCXX_CXX_ABI STREQUAL "libcxxabi"
> + if (APPLE AND LIBCXX_CXX_ABI STREQUAL "intree-libcxxabi"
> AND NOT DEFINED LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS
> AND NOT LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
> set(LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS ON)
> diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
> index 4ff71500465e..d4dfc715ce8d 100755
> --- a/libcxx/utils/ci/run-buildbot
> +++ b/libcxx/utils/ci/run-buildbot
> @@ -93,7 +93,7 @@ function generate-cmake-base() {
> function generate-cmake() {
> generate-cmake-base \
> -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind" \
> - -DLIBCXX_CXX_ABI=libcxxabi \
> + -DLIBCXX_CXX_ABI=intree-libcxxabi \
> "${@}"
> }
>
> @@ -493,7 +493,7 @@ legacy-project-build)
> -DCMAKE_BUILD_TYPE=RelWithDebInfo \
> -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
> -DLLVM_LIT_ARGS="-sv --show-unsupported --xunit-xml-output
> test-results.xml --timeout=1200" \
> - -DLIBCXX_CXX_ABI=libcxxabi
> + -DLIBCXX_CXX_ABI=intree-libcxxabi
> check-runtimes
> ;;
> aarch64)
> diff --git a/libcxx/utils/libcxx/test/config.py
> b/libcxx/utils/libcxx/test/config.py
> index eb55c03a7687..0f6130a44aeb 100644
> --- a/libcxx/utils/libcxx/test/config.py
> +++ b/libcxx/utils/libcxx/test/config.py
> @@ -378,12 +378,12 @@ class Configuration(object):
> self.cxx.link_flags += ['-lc++']
>
> def configure_link_flags_abi_library(self):
> - cxx_abi = self.get_lit_conf('cxx_abi', 'libcxxabi')
> + cxx_abi = self.get_lit_conf('cxx_abi', 'intree-libcxxabi')
> if cxx_abi == 'libstdc++':
> self.cxx.link_flags += ['-lstdc++']
> elif cxx_abi == 'libsupc++':
> self.cxx.link_flags += ['-lsupc++']
> - elif cxx_abi == 'libcxxabi':
> + elif cxx_abi == 'intree-libcxxabi':
> # If the C++ library requires explicitly linking to libc++abi, or
> # if we're testing libc++abi itself (the test configs are
> shared),
> # then link it.
> @@ -399,7 +399,7 @@ class Configuration(object):
> self.cxx.link_flags += [abs_path]
> else:
> self.cxx.link_flags += ['-lc++abi']
> - elif cxx_abi == 'system-libcxxabi':
> + elif cxx_abi == 'libcxxabi':
> self.cxx.link_flags += ['-lc++abi']
> elif cxx_abi == 'libcxxrt':
> self.cxx.link_flags += ['-lcxxrt']
>
> ```
Yes - if this changes the behaviour for all existing users who build with
`-DLIBCXX_CXX_ABI=libcxxabi`, I think it's not worth all the breakage it'd
cause.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120727/new/
https://reviews.llvm.org/D120727
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
