This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi][cmake] Allow building without libcxx again
ClosedPublic

Authored by Ericson2314 on Aug 20 2022, 8:09 AM.

Details

Summary

This allows -DLLVM_ENABLE_RUNTIMES=libcxxabi to work.

This functionality was removed in D125561 (among other such removals) as
dead code, because it was only available as part of the standalone build
before.

The functionality as added back as it was, except target_include_directories is used instead of the cruder target_compile_options (with an MSVC vs not MSVC conditional split). That is just wholly better.

Diff Detail

Event Timeline

Ericson2314 created this revision.Aug 20 2022, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 8:09 AM
Herald added a subscriber: mgorny. · View Herald Transcript
Ericson2314 requested review of this revision.Aug 20 2022, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 8:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Ah I would have to check both lists to support the legacy case. Ew.

D132324 removes support for the deprecated legacy case, in which case this will pass CI.

phosek added inline comments.Aug 20 2022, 7:34 PM
libcxxabi/CMakeLists.txt
143–147

Can yo use target_include_directories instead?

Ericson2314 added inline comments.Aug 20 2022, 7:36 PM
libcxxabi/CMakeLists.txt
143–147

I had gotten simply by adding back what was removed, but yes target_include_directories would be much better.

Use target_include_directories

phosek accepted this revision.Aug 20 2022, 7:41 PM

LGTM

This revision is now accepted and ready to land.Aug 20 2022, 7:41 PM

The legacy build is no longer CI'd so the one failure here is no longer an issue.

This revision was landed with ongoing or failed builds.Aug 21 2022, 5:23 AM
This revision was automatically updated to reflect the committed changes.

this is breaking builds with:
cmake -G Ninja ../llvm "-DLLVM_ENABLE_PROJECTS=all" "-DCMAKE_BUILD_TYPE=Release" "-DCMAKE_C_COMPILER=clang" "-DCMAKE_CXX_COMPILER=clang++"

Looking at https://libcxx.llvm.org/BuildingLibcxx.html, I assume these have been deprecated for a while but there were no warnings from CMake. Moreover LIBCXXABI_LIBCXX_INCLUDES was not being used or required previously by default at all (and LLVM_ENABLE_RUNTIMES).

After this change it enforces either having LLVM_ENABLE_RUNTIMES or LIBCXXABI_LIBCXX_INCLUDES set in the build configuration to build libcxx. Since I am not familiar with libcxx I can't really say if that's the desired behavior. Can you explicitly verify that's the case (and if so tell me how the builds with the config mentioned above was working above? they weren't able to build libcxx at all?)

libcxxabi/CMakeLists.txt
136

all is also a possible value for LLVM_ENABLE_RUNTIMES (here and below, in test/CMakeLists.txt) can you get that fixed? see

Looks like the error messages didn't show up because expansion of all happened after the check. sent out D132454 to fix it.

since libcxx(abi) can't be build through llvm_enable_projects, should we exclude it from the list in https://github.com/llvm/llvm-project/blob/main/llvm/CMakeLists.txt#L112 ?

libcxxabi/CMakeLists.txt
136

okay this was happening because LLVM_ENABLE_PROJECTS=all would also include this directory, without going through runtimes, which performs the expansion.

Thanks for that sleuthing, @kadircet.

Also see D132411 which should help fix some things too.