Page MenuHomePhabricator

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

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.

Diff Detail

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.Jan 29 2020, 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.Jan 29 2020, 4:37 PM
ldionne requested changes to this revision.Jan 29 2020, 4:39 PM
This revision now requires changes to proceed.Jan 29 2020, 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.Jan 30 2020, 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?

ldionne accepted this revision.Mar 11 2020, 12:22 PM

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.

This revision is now accepted and ready to land.Mar 11 2020, 12:22 PM
broadwaylamb added a comment.EditedMar 11 2020, 3:30 PM

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.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 11 2020, 3:30 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Mar 11 2020, 3:30 PM

This revision now requires review to proceed.

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

EricWF accepted this revision.Mar 11 2020, 3:36 PM

Your good to commit.

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

This revision is now accepted and ready to land.Mar 11 2020, 3:36 PM

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 :-)

This revision was automatically updated to reflect the committed changes.