Page MenuHomePhabricator

[libc++] Make sure that all headers can be included with modules enabled
ClosedPublic

Authored by ldionne on May 10 2022, 12:28 PM.

Details

Summary

This commit ensures that we can include all libc++ headers with modules
enabled. It adds a test to ensure that this doesn't regress, which is
necessary because our modules CI job does not build in all Standard modes.

Diff Detail

Unit TestsFailed

TimeTest
6,040 mslibcxx CI No filesystem > llvm-libc++-shared-cfg-in.libcxx::modules_include.sh.cpp
Script: -- : 'RUN: at line 44'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/a5ae1490ec91-1/llvm-project/libcxx-ci/libcxx/test/libcxx/modules_include.sh.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/a5ae1490ec91-1/llvm-project/libcxx-ci/build/generic-no-filesystem/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/a5ae1490ec91-1/llvm-project/libcxx-ci/build/generic-no-filesystem/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/a5ae1490ec91-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules -fcxx-modules -fsyntax-only -DTEST_0
6,190 mslibcxx CI No threads > llvm-libc++-shared-cfg-in.libcxx::modules_include.sh.cpp
Script: -- : 'RUN: at line 44'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/9fde9fa6d7e0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/modules_include.sh.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/9fde9fa6d7e0-1/llvm-project/libcxx-ci/build/generic-singlethreaded/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/9fde9fa6d7e0-1/llvm-project/libcxx-ci/build/generic-singlethreaded/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/9fde9fa6d7e0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules -fcxx-modules -fsyntax-only -DTEST_0
5,610 mslibcxx CI No wide characters > llvm-libc++-shared-cfg-in.libcxx::modules_include.sh.cpp
Script: -- : 'RUN: at line 44'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/0a1d6a0b6e5b-1/llvm-project/libcxx-ci/libcxx/test/libcxx/modules_include.sh.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0a1d6a0b6e5b-1/llvm-project/libcxx-ci/build/generic-no-wide-characters/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0a1d6a0b6e5b-1/llvm-project/libcxx-ci/build/generic-no-wide-characters/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0a1d6a0b6e5b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules -fcxx-modules -fsyntax-only -DTEST_0

Event Timeline

ldionne created this revision.May 10 2022, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 12:28 PM
ldionne requested review of this revision.May 10 2022, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 12:28 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.May 11 2022, 2:44 PM

Will ship once CI is complete, I tested and this fixes the LLDB issues on Green Dragon.

This revision is now accepted and ready to land.May 11 2022, 2:44 PM
ldionne updated this revision to Diff 428936.May 12 2022, 7:18 AM
ldionne retitled this revision from [libc++] Encode the dependency of <stdatomic.h> on C++23 in the modulemap to [libc++] Fix dependency on C++ Standards in the modulemap.
ldionne edited the summary of this revision. (Show Details)

Expand patch to fix incorrect annotations for other headers.

ldionne updated this revision to Diff 428956.May 12 2022, 8:35 AM

Fix C++03

ldionne updated this revision to Diff 428963.May 12 2022, 9:08 AM

Try to fix Apple CI by adding missing include

ldionne updated this revision to Diff 428990.May 12 2022, 10:13 AM

Unsupport test on Windows, where system headers appear to not be modules-friendly.

ldionne updated this revision to Diff 429232.May 13 2022, 7:13 AM

More UNSUPPORTED to get CI to pass

ldionne updated this revision to Diff 431661.May 24 2022, 6:52 AM
ldionne retitled this revision from [libc++] Fix dependency on C++ Standards in the modulemap to [libc++] Make sure that all headers can be included with modules enabled.
ldionne edited the summary of this revision. (Show Details)

Hopefully fix CI.

ldionne updated this revision to Diff 431716.May 24 2022, 10:28 AM

Fix CI with bootstrapping build -- hopefully this doesn't break another job.

EricWF accepted this revision.May 24 2022, 11:22 AM
EricWF added a subscriber: EricWF.
EricWF added inline comments.
libcxx/include/barrier
54 ↗(On Diff #431716)

As a side note, I really don't love that headers are conditionally included. It just makes things weirder for our users, and even for us (it's an easy way to write portability bugs).

ldionne marked an inline comment as done.May 24 2022, 11:54 AM
ldionne added inline comments.
libcxx/include/barrier
54 ↗(On Diff #431716)

I agree -- I think we should only conditionally include headers when including the headers *doesn't* work (i.e. including iostream on a platform that doesn't have localization). Adding a TODO to my list.

This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.
phosek added a subscriber: phosek.EditedMay 26 2022, 5:04 PM

FYI our builders have started running out of disk space, and while investigating the issue we pinpointed it to this change. The problem is that Clang started writing files to [CACHE]/clang/ModuleCache (on Linux that's either ${HOME}/.cache/clang/ModuleCache or ${XDG_CACHE_HOME}/clang/ModuleCache when XDG_CACHE_HOME is set), which on our builders is located on a partition with limited disk space, and a single run of libc++ test suite now generates 3.1G of data. I don't think we need to do anything for this change since Clang is behaving as intended, instead we'll need to make sure that ${XDG_CACHE_HOME} is on a volume with enough space, but I'm still reporting this since it was unexpected and others might run into this issue as well.