[libcxx-commits] [PATCH] D120727: [libc++] Overhaul how we select the ABI library

Fri May 13 05:31:18 PDT 2022

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