ldionne accepted this revision as: libc++.
ldionne marked an inline comment as done.
ldionne added inline comments.
This revision is now accepted and ready to land.
================
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}.")
----------------
phosek wrote:
> mstorsjo wrote:
> > 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.
> Do we know how many users set `-DLIBCXX_CXX_ABI=libcxxabi` explicitly rather
> than simply relying on the default value? I wouldn't expect it to be very
> common, but I agree that it may not be worth the breakage.
>
> A potential solution would be to introduce a new value `libc++abi` and print
> a warning if users set `libcxxabi` and fallback to the existing behavior. In
> any case, this is something that can be done in a follow up change.
I don't know how frequent it is, but I have seen a couple in the monorepo (per
the patch above). Yes, I agree we could rename to something entirely instead
and it would avoid silent breakage. I also agree on pursuing that as a follow
up change if we care strongly enough.
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
