Page MenuHomePhabricator

[libc++] Overhaul how we select the ABI library
ClosedPublic

Authored by ldionne on Mar 1 2022, 7:03 AM.

Details

Reviewers
phosek
Group Reviewers
Restricted Project
Restricted Project
Commits
rGa80e65e00ada: [libc++] Overhaul how we select the ABI library
Summary

This patch overhauls how we pick up the ABI library. Instead of setting
ad-hoc flags, it creates interface targets that can be linked against by
the rest of the build, which is easier to follow and extend to support
new ABI libraries. It also adds a new ABI library called "system-libcxxabi",
which represents linking against a LLVM libc++abi already installed on the
system, and solves existing issues when trying to build against a system
libc++abi outside of the now-unsupported standalone build.

This is intended to be a NFC change, however there are some additional
simplifications and improvements we can make in the future that would
require a slight behavior change.

Diff Detail

Event Timeline

ldionne created this revision.Mar 1 2022, 7:03 AM
ldionne requested review of this revision.Mar 1 2022, 7:03 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 1 2022, 7:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 412191.Mar 1 2022, 11:42 AM

Rebase onto main.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 1:24 PM

FWIW I think D116689 interacts with this somewhat. I think D116689 is useful for the fixes I want to do wrt libcxxabi integration on Windows (but I haven't studied it, nor this one, closely yet though). @phosek Do you see anything in this one that isn't compatible with D116689?

FWIW I think D116689 interacts with this somewhat. I think D116689 is useful for the fixes I want to do wrt libcxxabi integration on Windows (but I haven't studied it, nor this one, closely yet though). @phosek Do you see anything in this one that isn't compatible with D116689?

It does interact a lot, in fact. If we land this patch, I suspect that the libc++abi parts of D116689 may not be necessary anymore. And I also had a plan (not concrete) to use a similar approach for deciding how the unwind library is picked up, which may be worth exploring. As far as I can tell, the main thing that is nicer with D116689's approach over this patch is that we can get rid of logic around "-Wl,-force_load" for merging the static library into libc++, but I think the approach proposed here would be compatible with that too.

The thing I like about this approach is that it simplifies the amount of logic we need by properly encoding dependencies in the libcxx-abi-shared|static targets.

libcxx/src/CMakeLists.txt
234–240

This part would become nicer if we had an OBJECT library like in D116689. I think this approach does not preclude improving this in the future by using an object library.

ldionne updated this revision to Diff 427721.Fri, May 6, 12:47 PM

Rebase onto main

ldionne updated this revision to Diff 428108.Mon, May 9, 9:33 AM

Try addressing CI issues.

Herald added a project: Restricted Project. · View Herald TranscriptMon, May 9, 9:33 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 428361.Tue, May 10, 7:00 AM

Rebase to poke CI. Any interest in reviewing this @phosek @mstorsjo?

I gave it a quick readthrough, but I don't think I can give a qualified review of the bulk of it... I noticed a couple minor details though.

libcxx/CMakeLists.txt
254–255

Couldn't these lines be replaced with just a set(LIBCXX_CXX_ABI ${LIBCXX_DEFAULT_ABI_LIBRARY})?

libcxxabi/CMakeLists.txt
201 ↗(On Diff #428361)

Unrelated to the rest of the patch?

ldionne updated this revision to Diff 428658.Wed, May 11, 7:22 AM
ldionne marked 2 inline comments as done.

Rebase and address review comments.

libcxxabi/CMakeLists.txt
201 ↗(On Diff #428361)

Indeed, let me do that separately as a NFC.

phosek added inline comments.Thu, May 12, 1:22 AM
libcxx/CMakeLists.txt
250

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?

libcxx/cmake/Modules/HandleLibCXXABI.cmake
92–93

I'd consider moving these two lines into import_shared_library to further reduce duplication.

96–97

The same here, I'd consider moving these two lines into import_static_library.

98

I think this should be static?

102

The indentation here is off.

ldionne marked 5 inline comments as done.Thu, May 12, 12:32 PM
ldionne added inline comments.
libcxx/CMakeLists.txt
250

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']
libcxx/cmake/Modules/HandleLibCXXABI.cmake
92–93

I agree with moving the creation of the target to import_xx_library, but I think it makes sense to keep the reference to libcxx-abi-headers here since it doesn't exist at the point where import_xx_library is defined. I'll also refactor this a tad more to reduce duplication by passing SHARED|STATIC into the import_xx_library function.

mstorsjo added inline comments.Thu, May 12, 12:36 PM
libcxx/CMakeLists.txt
250

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.

ldionne updated this revision to Diff 429069.Thu, May 12, 2:05 PM
ldionne marked 2 inline comments as done.

Address review comments.

phosek accepted this revision.Thu, May 12, 4:58 PM

LGTM

libcxx/CMakeLists.txt
250

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.

libcxx/cmake/Modules/HandleLibCXXABI.cmake
60

Nit: I'd prefer using lowercase for all argument names.

ldionne accepted this revision as: Restricted Project.Fri, May 13, 5:31 AM
ldionne marked an inline comment as done.
ldionne added inline comments.
libcxx/CMakeLists.txt
250

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.

This revision is now accepted and ready to land.Fri, May 13, 5:31 AM
This revision was automatically updated to reflect the committed changes.

Hi, we're seeing some failures in Fuchsia's Clang CI. Our runtimes build seems to be unable to find cxxabi.h.

The failing bot can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814278370664903633/overview

pcc added a subscriber: pcc.Fri, May 13, 1:57 PM

Hi, we're seeing some failures in Fuchsia's Clang CI. Our runtimes build seems to be unable to find cxxabi.h.

The failing bot can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814278370664903633/overview

Looks like we're seeing the same issue on the sanitizer CI: https://lab.llvm.org/buildbot/#/builders/37/builds/13232