Page MenuHomePhabricator

[libc++] Use a proper CMake target to represent libc++ headers
ClosedPublic

Authored by ldionne on Jun 27 2020, 4:20 AM.

Details

Summary

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++.

Diff Detail

Event Timeline

ldionne created this revision.Jun 27 2020, 4:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2020, 4:20 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@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.

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.

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

phosek added inline comments.Jul 12 2020, 9:06 PM
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.

EricWF accepted this revision.Mon, Jul 13, 10:30 AM
EricWF added a subscriber: EricWF.

IDK if I know what's causing the issue, but this change LGTM.

This revision is now accepted and ready to land.Mon, Jul 13, 10:30 AM
ldionne updated this revision to Diff 277815.Tue, Jul 14, 6:51 AM

Update with Petr's workaround

This revision was automatically updated to reflect the committed changes.

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.

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8874769072373415488/+/steps/gclient_runhooks/0/stdout

Is this intended?

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.

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8874769072373415488/+/steps/gclient_runhooks/0/stdout

Is this intended?

This looks the issue I've been investigating, but it seems like the workaround I suggested doesn't help here.

Is it ok to revert this then?

Is it ok to revert this then?

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?

Is it ok to revert this then?

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?

Is it ok to revert this then?

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?

See https://bugs.chromium.org/p/chromium/issues/detail?id=1067216#c7 for background :)

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?

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.