[libcxxabi] Allow tests to link with static libc++abi/libc++ even if the shared version is present

[broadwaylamb]

Summary

Right now the only way to force libc++abi tests to link with the static version of libc++abi is to set LIBCXXABI_ENABLE_SHARED to OFF. However, this doesn't work when libc++abi is built as standalone project because of this.

This change allows specifying the version of the library for tests to link with.

This is useful for remote testing, for example, with SSHExecutor, where we _have_ to link with libc++abi statically.

Two new CMake options are introduced here: LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI and LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX. They can be set to OFF to tell the test utility to link tests with the static libraries.

It shouldn't break anything, because the default values of these options are set such that the test utility will behave the same way.

[broadwaylamb]

libcxxabi/CMakeLists.txt

240

This is not dangerous: earlier we by default set LIBCXXABI_LIBCXX_LIBRARY_PATH to empty string and cache it. If the user overrides this setting with some value, this code path is never hit.

However, this change is required if the user doesn't override this variable: since we already cached the default value (the empty string), this code path is hit, but it doesn't do anything, because earliest CACHE wins, unless we use FORCE.

libcxxabi/test/lit.site.cfg.in

19–21

Note that LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI and LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX are by default set to LIBCXXABI_ENABLE_SHARED and LIBCXX_ENABLE_SHARED respectively, which makes this change non-breaking.

[broadwaylamb]

Could anyone please verify this patch?

[jroelofs]

LGTM

[phosek]

libcxxabi/CMakeLists.txt

99

Have you considered using cmake_dependent_option? That way you don't need to do the validation below yourself.

[ldionne]

This commit broke testing when LIBCXX_ENABLE_STATIC is OFF. This includes the configuration in Apple's CMake cache. It also seems to assume that we want to carry on supporting standalone builds of libc++abi, which isn't our intention (unless something's changed recently).

Also, you didn't wait for one of the usual maintainers to sign off on this.

I'm tempted to revert this but I won't do it immediately.

Can you show me how you're building libc++abi which makes it _not work_ when you set LIBCXXABI_ENABLE_SHARED to OFF?

libcxxabi/CMakeLists.txt

101

Here, LIBCXX_ENABLE_SHARED will be OFF by default (even though it's normally ON by default in libc++'s CMake), because libc++abi's CMake is loaded before libc++'s CMake.

[broadwaylamb]

Set LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX to ON if LIBCXX_ENABLE_SHARED is not defined

[broadwaylamb]

In D71894#1848378, @ldionne wrote:

This commit broke testing when LIBCXX_ENABLE_STATIC is OFF. This includes the configuration in Apple's CMake cache.

I've updated the revision. I've run the tests using the Apple cache (and I undid 93cdd310e16ec57efd4a93886dd3b71ea4cf8183 to verify that the issue has indeed been fixed) and it seems to work.

It also seems to assume that we want to carry on supporting standalone builds of libc++abi, which isn't our intention (unless something's changed recently).

Not necessarily. What if we want to build both static and dynamic versions, but only test against the static one? It seemed weird that the test configuration did not provide a way to explicitly specify that.

But I admit that the inability to build static libc++abi in non-standalone mode for testing was the main reason for this patch. I wanted to invest some time in fixing this, but I've looked through the git history of those lines, which were added 6 years ago and still no one fixed it, so I figured that I, with my very limited libc++/libc++abi experience, am of no help there.

Also, you didn't wait for one of the usual maintainers to sign off on this.

I'm tempted to revert this but I won't do it immediately.

Thanks for giving a chance to address your comments. I'll be more careful next time.

Can you show me how you're building libc++abi which makes it _not work_ when you set LIBCXXABI_ENABLE_SHARED to OFF?

My primary use case is building a Windows to ARM Linux cross-toolchain for this buildbot. But the issue can be reproduced on macOS as well (assuming we're inside the build directory):

cmake -G Ninja -DLLVM_ENABLE_RUNTIMES="libcxxabi" -DLIBCXXABI_ENABLE_SHARED=OFF ${MONOREPO_ROOT}/llvm
ninja cxxabi

This produces the warning:

CMake Warning at /Users/sergej/Developer/llvm-project/libcxxabi/CMakeLists.txt:545 (message):
  The libc++abi tests aren't valid when libc++abi is built standalone (i.e.
  outside of llvm/projects/libcxxabi ) and is built without a shared library.
  Either build a shared library, build libc++abi at the same time as you
  build libc++, or do without testing.  No check target will be available!

and I can't do ninja check-cxxabi.

[broadwaylamb]

libcxxabi/CMakeLists.txt

99

I have considered it, but I couldn't come up with proper conditions that would encode this logic.

[broadwaylamb]

Move the new CMake options from libcxxabi/CMakeLists.txt to libcxxabi/test/CMakeLists.txt.

• Those options are only relevant for testing, so it seems like libcxxabi/test/CMakeLists.txt is a better place for them.
• libcxxabi/test/CMakeLists.txt already defines LIBCXX_ENABLE_SHARED to ON if it hasn't been defined, so the issue pointed out by @ldionne naturally goes away.

[broadwaylamb]

@ldionne could you please take a look?

[broadwaylamb]

@ldionne would you have some time to verify this?

[broadwaylamb]

@ldionne ping

[ldionne]

This LGTM as a band-aid, however I believe we need to dissociate the core logic of building libc++ and libc++abi from how we test it. That's because there's way too many ways to test libc++/libc++abi, and having all of it in the CMake files creates a lot of complexity.

But this is just hand-waving at this point, so let's move forward with this until we are closer to addressing the real problem I'm thinking about.

[broadwaylamb]

In D71894#1917762, @ldionne wrote:

This LGTM as a band-aid, however I believe we need to dissociate the core logic of building libc++ and libc++abi from how we test it. That's because there's way too many ways to test libc++/libc++abi, and having all of it in the CMake files creates a lot of complexity.

But this is just hand-waving at this point, so let's move forward with this until we are closer to addressing the real problem I'm thinking about.

Thanks.

Please let me know if I can be of help in that effort, as I'm also interested in making the logic in these projects more straightforward.

[broadwaylamb]

This revision now requires review to proceed.

@ldionne who else has to review it before it can be committed?

[EricWF]

Your good to commit.

The libc++abi blocking review is a side-effect of its recent addition. i.e. just a quirk.

[ldionne]

In D71894#1918260, @EricWF wrote:

Your good to commit.

The libc++abi blocking review is a side-effect of its recent addition. i.e. just a quirk.

@broadwaylamb Yes, you're good to go! But thanks for asking :-)

libcxxabi/CMakeLists.txt

 # the libc++abi that is being built. There are two problems with testing a
 # static libc++abi. In the case of a standalone build, the tests will link the
 # system's libc++, which might not have been built against our libc++abi. In the
 # case of an in tree build, libc++ will prefer a dynamic libc++abi from the
 # system over a static libc++abi from the output directory.
 option(LIBCXXABI_ENABLE_SHARED "Build libc++abi as a shared library." ON)
 option(LIBCXXABI_ENABLE_STATIC "Build libc++abi as a static library." ON)
 
+option(LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI
+       "Whether the libc++abi tests should link with the shared libc++abi library"
+       ${LIBCXXABI_ENABLE_SHARED})
+
+option(LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX
+       "Whether the libc++abi tests should link with the shared libc++ library"
+       ${LIBCXX_ENABLE_SHARED})
+
 cmake_dependent_option(LIBCXXABI_INSTALL_STATIC_LIBRARY
   "Install the static libc++abi library." ON
   "LIBCXXABI_ENABLE_STATIC;LIBCXXABI_INSTALL_LIBRARY" OFF)
 cmake_dependent_option(LIBCXXABI_INSTALL_SHARED_LIBRARY
   "Install the shared libc++abi library." ON
   "LIBCXXABI_ENABLE_SHARED;LIBCXXABI_INSTALL_LIBRARY" OFF)
 
 cmake_dependent_option(LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_STATIC_LIBRARY
   "Statically link the LLVM unwinder to static library" ON
   "LIBCXXABI_ENABLE_STATIC_UNWINDER;LIBCXXABI_ENABLE_STATIC" OFF)
 cmake_dependent_option(LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY
   "Statically link the LLVM unwinder to shared library" ON
   "LIBCXXABI_ENABLE_STATIC_UNWINDER;LIBCXXABI_ENABLE_SHARED" OFF)
 
 option(LIBCXXABI_BAREMETAL "Build libc++abi for baremetal targets." OFF)
 # The default terminate handler attempts to demangle uncaught exceptions, which
 # causes extra I/O and demangling code to be pulled in.
 option(LIBCXXABI_SILENT_TERMINATE "Set this to make the terminate handler default to a silent alternative" OFF)
 
 if (NOT LIBCXXABI_ENABLE_SHARED AND NOT LIBCXXABI_ENABLE_STATIC)
   message(FATAL_ERROR "libc++abi must be built as either a shared or static library.")
 endif()
 
+if(LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI AND NOT LIBCXXABI_ENABLE_SHARED)
+  message(FATAL_ERROR "LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI being ON requires LIBCXXABI_ENABLE_SHARED to be ON")
+endif()
+
+if(NOT LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI AND NOT LIBCXXABI_ENABLE_STATIC)
+  message(FATAL_ERROR "LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI being OFF requires LIBCXXABI_ENABLE_STATIC to be ON")
+endif()
+
+if(DEFINED LIBCXX_ENABLE_SHARED 
+   AND LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX
+   AND NOT LIBCXX_ENABLE_SHARED)
+  message(FATAL_ERROR "LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX being ON requires LIBCXX_ENABLE_SHARED to be ON")
+endif()
+
+if(DEFINED LIBCXX_ENABLE_STATIC
+   AND NOT LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX 
+   AND NOT LIBCXX_ENABLE_STATIC)
+  message(FATAL_ERROR "LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX being OFF requires LIBCXX_ENABLE_STATIC to be ON")
+endif()
+
 if (LLVM_EXTERNAL_LIBCXX_SOURCE_DIR)
   set(LIBCXXABI_LIBCXX_SRC_DIRS ${LLVM_EXTERNAL_LIBCXX_SOURCE_DIR})
 else()
   set(LIBCXXABI_LIBCXX_SRC_DIRS
     "${LLVM_MAIN_SRC_DIR}/projects/libcxx"
     "${LLVM_MAIN_SRC_DIR}/runtimes/libcxx"
     "${LLVM_MAIN_SRC_DIR}/../libcxx"
     )

 set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${LIBCXXABI_LIBRARY_DIR})
 set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${LIBCXXABI_LIBRARY_DIR})
 set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${LIBCXXABI_LIBRARY_DIR})
 
 # By default, for non-standalone builds, libcxx and libcxxabi share a library
 # directory.
 if (NOT LIBCXXABI_LIBCXX_LIBRARY_PATH)
   set(LIBCXXABI_LIBCXX_LIBRARY_PATH "${LIBCXXABI_LIBRARY_DIR}" CACHE PATH
-      "The path to libc++ library.")
+      "The path to libc++ library." FORCE)
 endif()
 
 # Check that we can build with 32 bits if requested.
 if (CMAKE_SIZEOF_VOID_P EQUAL 8 AND NOT WIN32)
   if (LIBCXXABI_BUILD_32_BITS AND NOT LLVM_BUILD_32_BITS) # Don't duplicate the output from LLVM
     message(STATUS "Building 32 bits executables and libraries.")
   endif()
 elseif(LIBCXXABI_BUILD_32_BITS)

libcxxabi/test/CMakeLists.txt

 pythonize_bool(LIBCXX_ENABLE_SHARED)
 pythonize_bool(LIBCXXABI_ENABLE_SHARED)
 pythonize_bool(LIBCXXABI_ENABLE_THREADS)
 pythonize_bool(LIBCXXABI_ENABLE_EXCEPTIONS)
 pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
 pythonize_bool(LIBCXXABI_USE_COMPILER_RT)
 pythonize_bool(LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY)
 pythonize_bool(LIBCXX_ENABLE_PARALLEL_ALGORITHMS)
+pythonize_bool(LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX)
+pythonize_bool(LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI)
 set(LIBCXXABI_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
     "TargetInfo to use when setting up test environment.")
 set(LIBCXXABI_EXECUTOR "None" CACHE STRING
     "Executor to use when running tests.")
 
 set(AUTO_GEN_COMMENT "## Autogenerated by libcxxabi configuration.\n# Do not edit!")
 
 if (LIBCXXABI_ENABLE_SHARED)

libcxxabi/test/lit.site.cfg.in

 config.llvm_unwinder            = @LIBCXXABI_USE_LLVM_UNWINDER@
 config.builtins_library         = "@LIBCXXABI_BUILTINS_LIBRARY@"
 config.enable_threads           = @LIBCXXABI_ENABLE_THREADS@
 config.use_sanitizer            = "@LLVM_USE_SANITIZER@"
 config.sanitizer_library        = "@LIBCXXABI_SANITIZER_LIBRARY@"
 config.enable_32bit             = @LIBCXXABI_BUILD_32_BITS@
 config.target_info              = "@LIBCXXABI_TARGET_INFO@"
 config.executor                 = "@LIBCXXABI_EXECUTOR@"
-config.libcxxabi_shared         = @LIBCXXABI_ENABLE_SHARED@
-config.enable_shared            = @LIBCXX_ENABLE_SHARED@
+config.libcxxabi_shared         = @LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXXABI@
+config.enable_shared            = @LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX@
 config.enable_exceptions        = @LIBCXXABI_ENABLE_EXCEPTIONS@
 config.host_triple              = "@LLVM_HOST_TRIPLE@"
 config.target_triple            = "@TARGET_TRIPLE@"
 config.use_target               = bool("@LIBCXXABI_TARGET_TRIPLE@")
 config.sysroot                  = "@LIBCXXABI_SYSROOT@"
 config.gcc_toolchain            = "@LIBCXXABI_GCC_TOOLCHAIN@"
 config.cxx_ext_threads          = @LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY@
 
 config.pstl_src_root            = "@ParallelSTL_SOURCE_DIR@" if @LIBCXX_ENABLE_PARALLEL_ALGORITHMS@ else None
 config.pstl_obj_root            = "@ParallelSTL_BINARY_DIR@" if @LIBCXX_ENABLE_PARALLEL_ALGORITHMS@ else None
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@LIBCXXABI_SOURCE_DIR@/test/lit.cfg")