Instead of having complex logic around how to include the libc++ headers
and config_site, handle that by defining cxx-headers as an INTERFACE
library and linking against it. After this patch, linking against cxx-headers
is sufficient to get the right config_site include and include paths
for libc++.
Details
- Reviewers
phosek EricWF - Group Reviewers
Restricted Project - Commits
- rGefa40eb19491: [libc++] Use a proper CMake target to represent libc++ headers
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
@phosek I know this change breaks the runtimes build, but I can't figure out why. I spent some time trying to understand why, but I'd welcome some help.
I spent some time debugging this, and to be honest, I don't know what's going on. The problem is that in the multilib variant case, the header includes are missing. So normally, the generated Ninja entries would look like this:
build libcxx/src/CMakeFiles/cxx_static.dir/filesystem/operations.cpp.obj: CXX_COMPILER__cxx_static /src/clang-llvm/llvm-project/libcxx/src/filesystem/operations.cpp || cmake_object_order_depends_target_cxx_static DEFINES = -DNDEBUG -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS DEP_FILE = libcxx/src/CMakeFiles/cxx_static.dir/filesystem/operations.cpp.obj.d FLAGS = --target=x86_64-unknown-fuchsia -I/usr/local/google/home/phosek/clang-llvm/sdk/pkg/fdio/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -ffunction-sections -fdata-sections -ffile-prefix-map=/src/clang-llvm/llvm-build/fuchsia/runtimes/runtimes-x86_64-unknown-fuchsia-bins=../llvm-build/fuchsia/runtimes/runtimes-x86_64-unknown-fuchsia-bins -ffile-prefix-map=/src/clang-llvm/llvm-project/= -no-canonical-prefixes -O2 -g -DNDEBUG -DLIBCXX_BUILDING_LIBCXXABI -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wno-user-defined-literals -Wno-covered-switch-default -Wno-ignored-attributes -Wno-error -fvisibility-global-new-delete-hidden -include /src/clang-llvm/llvm-build/fuchsia/runtimes/runtimes-x86_64-unknown-fuchsia-bins/libcxx/__config_site -std=c++14 INCLUDES = -Ilibcxx/include/c++build -I/src/clang-llvm/llvm-build/fuchsia/include/c++/v1 OBJECT_DIR = libcxx/src/CMakeFiles/cxx_static.dir OBJECT_FILE_DIR = libcxx/src/CMakeFiles/cxx_static.dir/filesystem
but with this patch, we would end up with this:
build libcxx/src/CMakeFiles/cxx_static.dir/filesystem/operations.cpp.obj: CXX_COMPILER__cxx_static /src/clang-llvm/llvm-project/libcxx/src/filesystem/operations.cpp || cmake_object_order_depends_target_cxx_static DEFINES = -DNDEBUG -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS DEP_FILE = libcxx/src/CMakeFiles/cxx_static.dir/filesystem/operations.cpp.obj.d FLAGS = --target=x86_64-unknown-fuchsia -I/usr/local/google/home/phosek/clang-llvm/sdk/pkg/fdio/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fno-omit-frame-pointer -fsanitize=address -fsanitize-address-use-after-scope -ffunction-sections -fdata-sections -ffile-prefix-map=/src/clang-llvm/llvm-build/fuchsia/runtimes/runtimes-x86_64-unknown-fuchsia+asan+noexcept-bins=../llvm-build/fuchsia/runtimes/runtimes-x86_64-unknown-fuchsia+asan+noexcept-bins -ffile-prefix-map=/src/clang-llvm/llvm-project/= -no-canonical-prefixes -O2 -g -DNDEBUG -DLIBCXX_BUILDING_LIBCXXABI -fno-omit-frame-pointer -gline-tables-only -fsanitize=address -fsanitize=address -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wno-user-defined-literals -Wno-covered-switch-default -Wno-ignored-attributes -Wno-error -fno-exceptions -include /src/clang-llvm/llvm-build/fuchsia/runtimes/runtimes-x86_64-unknown-fuchsia+asan+noexcept-bins/libcxx/__config_site -std=c++14 INCLUDES = -Ilibcxx/include/c++build OBJECT_DIR = libcxx/src/CMakeFiles/cxx_static.dir OBJECT_FILE_DIR = libcxx/src/CMakeFiles/cxx_static.dir/filesystem
See the INCLUDES variable, in the latter case, -I/src/clang-llvm/llvm-build/fuchsia/include/c++/v1 is missing so compiler fails to find libc++ headers.
I have no idea why that happens. I inserted debugging prints into various places and I haven't seen any obvious issue. The only difference between the two runtime builds is (aside from ASan instrumentation) that in the latter case, compiler-rt isn't being built as part of runtimes, but I still don't see why that would should matter here.
One more thing I've noticed is that both of those invocations include __config_site which is added by the cxx-headers interface library, so that library must be used, it's just as if target_include_directories(${CXX_HEADER_TARGET} INTERFACE ${output_dir}) was ignored when building the instrumented version.
Another thing I just found out, this is only affecting paths that end with include/c++/v1, if I add any other path like include/c++/v2, it's works fine. So it seems likes something is filtering out paths that end with include/c++/v1 from include directories. I'm starting to suspect a bug in CMake (I'm using version 3.16.3 for reference).
libcxx/include/CMakeLists.txt | ||
---|---|---|
239 | When I replace this line with: if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC") target_compile_options(${CXX_HEADER_TARGET} INTERFACE /I "${output_dir}") else() target_compile_options(${CXX_HEADER_TARGET} INTERFACE -I "${output_dir}") endif() everything seems to be working. So whatever is filtering out the include path seems to be only affecting target_include_directories. |
Hi, I think this is causing our Mac bots to fail when building libcxx, with
/opt/s/w/ir/cache/xcode_ios_11a1027.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DNDEBUG -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/libcxx/src -I/opt/s/w/ir/cache/builder/src/third_party/llvm/libcxx/src -I/opt/s/w/ir/cache/xcode_ios_11a1027.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/libxml2 -Iinclude -I/opt/s/w/ir/cache/builder/src/third_party/llvm/llvm/include -Iprojects/libcxx/include/c++build -DLLVM_FORCE_HEAD_REVISION -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -O3 -isysroot /opt/s/w/ir/cache/xcode_ios_11a1027.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.14 -DLIBCXX_BUILDING_LIBCXXABI -UNDEBUG -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wno-user-defined-literals -Wno-covered-switch-default -Wno-ignored-attributes -Wno-error -std=c++14 -MD -MT projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o -MF projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o.d -o projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o -c /opt/s/w/ir/cache/builder/src/third_party/llvm/libcxx/src/experimental/memory_resource.cpp
/opt/s/w/ir/cache/builder/src/third_party/llvm/libcxx/src/experimental/memory_resource.cpp:9:10: fatal error: 'experimental/memory_resource' file not found
#include "experimental/memory_resource"
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Is this intended?
This looks the issue I've been investigating, but it seems like the workaround I suggested doesn't help here.
No.
The issue is that your builder builds with -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_STATIC=OFF, but LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY is implicitly set to ON. In other words, you're trying to build the libc++ experimental library without building the static or the shared library. If you're trying to disable libc++ from the build, why include it in LLVM_ENABLE_PROJECTS?
Ah, thanks for the explanation.
Does it make sense for LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY to only be implicitly set to ON if either of LIBCXX_ENABLE_SHARED or LIBCXX_ENABLE_STATIC are on?
See https://bugs.chromium.org/p/chromium/issues/detail?id=1067216#c7 for background :)
Either that, or we could have cxx_experimental link against cxx-headers, which would make the build work (even thought that's not what you want). Or we could issue an error to make it clearer.
I think the most consistent thing to do is to link cxx_experimental against cxx-headers, since it does use the libc++ headers after all.
Fair enough.
I'm troubleshooting a build failure that happens when this commit lands on a downstream builder. The symptom looks similar to the one reported by @aeubanks where the includes aren't present but I've ruled out the EXPERIMENTAL_LIBRARY (explicitly set OFF in the failing build).
Can someone share a link to a reference bot that is still working well after this change? Maybe I have the wrong recipe.
I was hoping that I could use the premerge check associated with this review as a reference builder but that one seems to fail as well (but a different symptom: attempt to explicitly link to "cxxabi_shared" fails -- old cmake? or related?)
https://buildkite.com/llvm-project/premerge-checks/builds/3740#35262bb7-c20c-4117-9f03-1811a95be68b
When I replace this line with:
everything seems to be working. So whatever is filtering out the include path seems to be only affecting target_include_directories.