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

Thu May 12 12:33:06 PDT 2022

ldionne marked 5 inline comments as done.
ldionne 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}.")
----------------
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']

```


================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:76-77
+
+  add_library(libcxx-abi-shared SHARED IMPORTED GLOBAL)
+  target_link_libraries(libcxx-abi-shared INTERFACE libcxx-abi-headers)
+  import_shared_library(libcxx-abi-shared "${LIBCXX_CXX_ABI_LIBRARY_PATH}" stdc++)
----------------
phosek wrote:
> I'd consider moving these two lines into `import_shared_library` to further reduce duplication.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

