Page MenuHomePhabricator

[libcxxabi] Use cxx-headers target to consume libcxx headers
ClosedPublic

Authored by phosek on Mar 10 2021, 12:39 PM.

Details

Summary

Rather than including libc++ include dir, use the cxx-headers target.

Diff Detail

Event Timeline

phosek created this revision.Mar 10 2021, 12:39 PM
phosek requested review of this revision.Mar 10 2021, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 12:39 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek updated this revision to Diff 329741.Mar 10 2021, 12:41 PM
Harbormaster completed remote builds in B93149: Diff 329741.
phosek updated this revision to Diff 329796.Mar 10 2021, 3:58 PM
phosek updated this revision to Diff 329843.Mar 10 2021, 9:55 PM
compnerd added inline comments.
libcxxabi/CMakeLists.txt
163–175

Why not make this if (NOT TARGET cxx-headers)? You don't really care so much about the standalone target as much as you do about the cxx-headers target existing.

168

I believe that with the suggestion below, this check is unnecessary as CMake will verify that the path exists and is a directory.

170–174

There is no need for this complicated special handling, why not just let CMake handle it?

phosek added inline comments.Mar 16 2021, 2:06 PM
libcxxabi/CMakeLists.txt
163–175

The issue is that depending on the order in which users put projects in LLVM_ENABLE_PROJECTS or LLVM_ENABLE_RUNTIMES, cxx-headers target may not yet exist, see also D97314 for a related discussion.

170–174

We tried to use that approach in the past but it broke the runtimes build with what appeared to be a CMake bug where it would simply drop the include on the floor on certain platforms like macOS, see D82702 for some of the background discussion.

We could try and revisit this but I'd prefer to do it in a follow up change.

@ldionne do you have any thoughts on this change? This was split from D97572.

ldionne accepted this revision.Mar 18 2021, 10:07 AM
ldionne added inline comments.
libcxxabi/CMakeLists.txt
170–174

Can you please create that follow-up change right now and make it a child of this patch?

This revision is now accepted and ready to land.Mar 18 2021, 10:07 AM
ldionne requested changes to this revision.Mar 18 2021, 10:08 AM

Actually, I accepted a bit too fast. I'm fine with the patch but there seems to be a problem with it, since CI is failing.

This revision now requires changes to proceed.Mar 18 2021, 10:08 AM
phosek updated this revision to Diff 331950.Mar 19 2021, 11:18 AM

Actually, I accepted a bit too fast. I'm fine with the patch but there seems to be a problem with it, since CI is failing.

It seems to be passing now.

ldionne accepted this revision.Mar 22 2021, 9:15 AM
This revision is now accepted and ready to land.Mar 22 2021, 9:15 AM

Hi,

I don't know what is happening at all, but I noticed that when I do "ninja check-runtimes" with this patch it fails with

[0/1] Running all regression tests
llvm-lit: /repo/uabelho/master-github/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libcxxabi/test/lit.site.cfg:60: note: Using configuration variant: libcxxabi
llvm-lit: /repo/uabelho/master-github/libcxx/utils/libcxx/test/config.py:632: note: Setting target_triple to x86_64-unknown-linux-gnu
llvm-lit: /repo/uabelho/master-github/libcxxabi/test/libcxxabi/test/config.py:67: fatal: cxx_headers='/repo/uabelho/master-github/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/include/c++/v1' is not a directory.

The directory
/repo/uabelho/master-github/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/include/c++/v1
doesn't exist. The dir
/repo/uabelho/master-github/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins
exists, but there is no "include" in it.
If I search for "v1" in my build dir I find

build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_x86_64/include/c++/v1
build-all-builtins/include/c++/v1

Any idea what's up?

bjope added a subscriber: bjope.Mar 24 2021, 4:05 AM
bjope added inline comments.
libcxxabi/test/libcxxabi/test/config.py
65

Looking at the same problem as @uabelho asked about. It works better if changing this line to
os.path.join(self.cxx_library_root, '..', '..', '..', 'include', 'c++', 'v1')
but maybe that isn't getting the intended headers?

What's your CMake configuration? I'd like to try and reproduce this locally so I can investigate.

Hi @phosek ,

we got the same thing on the win-x-arm cross toolchain builders as described by @uabelho

cmake --build . --target check-cxxabi
...

-- Build files have been written to: C:/buildbot/temp/build/runtimes/runtimes-bins
[4/5] cmd.exe /C "cd /D C:\buildbot\temp\build\runtimes\runtimes-bin...uild/runtimes/runtimes-bins/ --target check-cxxabi --config Release"
[0/1] Running libcxxabi tests
llvm-lit.py: c:\buildbot\temp\build\runtimes\runtimes-bins\libcxxabi\test\lit.site.cfg:60: note: Using configuration variant: libcxxabi
llvm-lit.py: C:\buildbot\temp\llvm-project\libcxx\utils\libcxx\test\target_info.py:152: note: inferred target_info as: 'libcxx.test.target_info.LinuxRemoteTI'
llvm-lit.py: C:\buildbot\temp\llvm-project\libcxx\utils\libcxx\test\config.py:632: note: Setting target_triple to aarch64-linux-gnu
llvm-lit.py: C:\buildbot\temp\llvm-project\libcxxabi\test\libcxxabi\test\config.py:67: fatal: cxx_headers='C:/buildbot/temp/build/runtimes/runtimes-bins\include\c++\v1' is not a directory.
FAILED: libcxxabi/test/CMakeFiles/check-cxxabi
cmd.exe /C "cd /D C:\buildbot\temp\build\runtimes\runtimes-bins\libcxxabi\test && C:\Python38\python.exe C:/buildbot/temp/build/./bin/llvm-lit.py -vv -v -vv C:/buildbot/temp/build/runtimes/runtimes-bins/libcxxabi/test"
ninja: build stopped: subcommand failed.
FAILED: runtimes/CMakeFiles/check-cxxabi
cmd.exe /C "cd /D C:\buildbot\temp\build\runtimes\runtimes-bins && "C:\Program Files\CMake\bin\cmake.exe" --build C:/buildbot/temp/build/runtimes/runtimes-bins/ --target check-cxxabi --config Release"
ninja: build stopped: subcommand failed.

The <build_root>/runtimes/runtimes-bins folder does not contain include/c++/v1. There is only <build-root>/include/c++/v1.

Builders:
aarch64: https://lab.llvm.org/buildbot/#/builders/119/builds/2894
armv7: https://lab.llvm.org/buildbot/#/builders/60/builds/2484

uabelho added a comment.EditedWed, Mar 24, 11:23 PM

What's your CMake configuration? I'd like to try and reproduce this locally so I can investigate.

For me it fails with

CC='/proj/flexasic/app/llvm/8.0/bin/clang -march=corei7' CXX='/proj/flexasic/app/llvm/8.0/bin/clang++ -march=corei7' LDFLAGS='-L/proj/flexasic/app/llvm/8.0/lib64 -Wl,-R/proj/flexasic/app/llvm/8.0/lib64:/proj/bbi_twh/wh_bbi/x86_64-Linux3/z3/4.8.8-1/lib64' PATH=/proj/flexasic/app/ninja/1.8.2/SLED11-64/bin:$PATH  /app/vbuild/RHEL7-x86_64/cmake/3.16.4/bin/cmake /repo/uabelho/master-github/llvm --debug-trycompile -G Ninja -DCMAKE_MAKE_PROGRAM=/proj/flexasic/app/ninja/1.8.2/SLED11-64/bin/ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_INSTALL_PREFIX=/compiler-clang -DLLVM_APPEND_VC_REV=OFF -DLLVM_CCACHE_BUILD=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_WERROR=ON -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;lld' -DLLVM_BUILTIN_TARGETS=default -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi;libunwind;compiler-rt' -DRUNTIMES_x86_64-unknown-linux-gnu_SANITIZER_USE_STATIC_LLVM_UNWINDER=ON -DRUNTIMES_x86_64-unknown-linux-gnu_SANITIZER_USE_STATIC_CXX_ABI=ON -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_USE_BUILTINS_LIBRARY=ON -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_CAN_EXECUTE_TESTS=OFF -DRUNTIMES_x86_64-unknown-linux-gnu_LIBCXX_HAS_ATOMIC_LIB=0 -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_BUILD_XRAY=OFF -DLLVM_RUNTIME_TARGETS='x86_64-unknown-linux-gnu' -DLLVM_ENABLE_Z3_SOLVER=ON -DLLVM_Z3_INSTALL_DIR=/proj/bbi_twh/wh_bbi/x86_64-Linux3/z3/4.8.8-1 -DLLVM_ENABLE_LIBPFM=OFF -DCLANG_TOOLING_BUILD_AST_INTROSPECTION=OFF

ninja check-runtimes

Since there are also buildbots failing, perhaps we can revert while investigating unless there is a very quick fix?

phosek reopened this revision.Thu, Mar 25, 3:48 PM
phosek updated this revision to Diff 333444.

@ldionne the latest diff should address the runtimes build test failure.

This revision is now accepted and ready to land.Thu, Mar 25, 3:48 PM
phosek updated this revision to Diff 333477.Thu, Mar 25, 5:56 PM

Hi @phosek,
I have tested the updated diff on Windows-x-ARM cross builder and it works fine. Thank you.

ldionne accepted this revision.Fri, Mar 26, 6:45 AM