Page MenuHomePhabricator

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

Authored by broadwaylamb on Dec 26 2019, 2:53 AM.

Details

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.

Event Timeline

broadwaylamb created this revision.Dec 26 2019, 2:53 AM
broadwaylamb retitled this revision from libcxxabi WIP to [libcxxabi] Allow tests to link with static libc++abi/libc++ even if the shared version is present.Dec 26 2019, 3:26 AM
broadwaylamb edited the summary of this revision. (Show Details)
broadwaylamb marked 2 inline comments as done.Dec 26 2019, 3:35 AM
broadwaylamb added inline comments.
libcxxabi/CMakeLists.txt
212

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–20 ↗(On Diff #235333)

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.

Could anyone please verify this patch?

jroelofs accepted this revision.Jan 9 2020, 7:21 AM

LGTM

This revision is now accepted and ready to land.Jan 9 2020, 7:21 AM
This revision was automatically updated to reflect the committed changes.
phosek added inline comments.Jan 14 2020, 11:49 AM
libcxxabi/CMakeLists.txt
99

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

ldionne reopened this revision.Wed, Jan 29, 4:37 PM

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.

This revision is now accepted and ready to land.Wed, Jan 29, 4:37 PM
ldionne requested changes to this revision.Wed, Jan 29, 4:39 PM
This revision now requires changes to proceed.Wed, Jan 29, 4:39 PM

Set LIBCXXABI_LINK_TESTS_WITH_SHARED_LIBCXX to ON if LIBCXX_ENABLE_SHARED is not defined

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 marked an inline comment as done.Thu, Jan 30, 2:21 AM
broadwaylamb added inline comments.
libcxxabi/CMakeLists.txt
99

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

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.

@ldionne could you please take a look?

@ldionne would you have some time to verify this?