phosek accepted this revision.
phosek added a comment.
LGTM
================
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}.")
----------------
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.
================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:60
+# It imports a library named <name> searched at the given <path>.
+function(imported_library target KIND path name)
+ add_library(${target} ${KIND} IMPORTED GLOBAL)
----------------
Nit: I'd prefer using lowercase for all argument names.
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
