Page MenuHomePhabricator

[libcxxabi,libunwind] support running tests in standalone mode
ClosedPublic

Authored by gargaroff on Aug 25 2020, 7:14 AM.

Details

Reviewers
ldionne
phosek
EricWF
compnerd
hintonda
mstorsjo
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rG8c03fdf34a65: [libcxxabi,libunwind] support running tests in standalone mode
Summary

Remove check for standalone and shared library mode in libcxxabi to
allow including tests in said mode. This check prevented running the
tests in standalone mode with static libraries, which is the case for
baremetal targets.

Fix check-unwind target trying to use a non-existent llvm-lit executable
in standalone mode. Copy the HandleOutOfTreeLLVM logic from libcxxabi to
libunwind in order to make the tests work in standalone mode.

Diff Detail

Event Timeline

gargaroff created this revision.Aug 25 2020, 7:14 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 25 2020, 7:14 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
gargaroff requested review of this revision.Aug 25 2020, 7:14 AM

We require this patch for our downstream baremetal target to be able to run the test suites of both libraries on our target.

I'm not sure whether the check in libcxxabi was simply outdated or if we will require some type of check there.

As for the libunwind part: I mostly copy-pasted the HandleOutOfTreeLLVM.cmake from libcxxabi and adapted it to libunwind in places where it was necessary. I compared the previous logic with the new one and although I did find some differences, mostly related to llvm-lit, most of them did not seem to be actually required.

Please note however that I was only able to test our downstream use-case. I hope this doesn't break anything. Please let me know if you spot anything and how we could fix that.

abidh added a comment.Aug 25 2020, 7:51 AM

We require this patch for our downstream baremetal target to be able to run the test suites of both libraries on our target.

I'm not sure whether the check in libcxxabi was simply outdated or if we will require some type of check there.

I also maintain a downstream baremetal target and have a similar local change to side step this check. Although I did not remove it entirely as I was not sure what it does and instead used NOT LIBCXXABI_BAREMETAL. Anyways, It will be nice to have this fixed upstream.

ldionne requested changes to this revision.Sep 1 2020, 7:54 AM
ldionne added inline comments.
libunwind/cmake/Modules/HandleOutOfTreeLLVM.cmake
1 ↗(On Diff #287659)

Instead of copy-pasting this, it should use the one from libcxx.

libunwind, libcxxabi and libcxx are all required in the tree at all times. A Standalone build refers to where the root of the CMake invocation is, not whether libunwind is actually checked out on its own. Because of this, it should be possible to reuse HandleOutOfTreeLLVM.cmake from libcxx instead of duplicating it.

libunwind/test/CMakeLists.txt
35

Where is this defined?

This revision now requires changes to proceed.Sep 1 2020, 7:54 AM
gargaroff planned changes to this revision.Sep 1 2020, 11:54 PM

Thanks for the feedback! I'm currently busy with a release, but I'll get back to this afterwards. Don't expect anything for the next two or three weeks.

gargaroff updated this revision to Diff 293152.Sep 21 2020, 6:21 AM
gargaroff marked 2 inline comments as done.

Rebase.
Reuse HandleOutOfTreeLLVM.cmake from libcxx in libunwind.
Define LIBUNWIND_TEST_PARAMS

@ldionne I only made libunwind reuse libcxx's HandleOutOfTreeLLVM.cmake. Libcxxabi is still using a libcxxabi specific one. I'm planning on making libcxxabi also reuse the libcxx one but first wanted to make sure that the approach for libunwind is looking good.

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2020, 6:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

The approach looks good, with comments.

libcxx/cmake/Modules/HandleOutOfTreeLLVM.cmake
28

I think it might be better to instead provide the LLVM_LIT_OUTPUT_DIR as a "parameter" of this module. Basically, set it before including it in various projects. WDYT?

libunwind/CMakeLists.txt
36

There's already a LIBUNWIND_LIBCXX_PATH defined somewhere in libunwind -- I think we should merge the two variables into one.

gargaroff marked 2 inline comments as done.

Reuse LIBUNWIND_LIBCXX_PATH.
Unify HandleOutOfTreeLLVM handling in libcxxabi and libunwind by reusing the one from libcxx.
Define {lib}_STANDALONE_BUILD outside of HandleOutOfTreeLLVM.
Define LLVM_LIT_OUTPUT_DIR outside of HandleOutOfTreeLLVM.

libcxx/cmake/Modules/HandleOutOfTreeLLVM.cmake
28

Yeah sounds good. In that case it would also make sense to do the same for {lib}_STANDALONE_BUILD. That way we can completely remove the package name check from here.

gargaroff added inline comments.Sep 22 2020, 12:57 AM
libcxx/CMakeLists.txt
33

Technically only LIBCXX_BINARY_DIR is required up here, but I wanted to keep those three together

I love this. Some comments but looks almost ready to go.

libcxxabi/CMakeLists.txt
40

This check isn't necessary -- we already have a similar one at the very top of the file.

libunwind/CMakeLists.txt
38–39

Can you add this at the very top of the file instead?

if (NOT IS_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/../libcxx")
  message(FATAL_ERROR "libunwind requires being built in a monorepo layout with libcxx available")
endif()
ldionne accepted this revision.Oct 13 2020, 8:43 AM

Actually, please feel free to ship this with the changes I requested.

gargaroff marked 2 inline comments as done.

Address final comments.

I'll land this once the build bots give me the green light. Thanks for the review Louis!

...or not. Seems like the pre-merge checks are down at the moment, so I'll just land it right away. If anything breaks, we can always revert it anyway.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 14 2020, 12:10 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.