This is an archive of the discontinued LLVM Phabricator instance.

[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

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.

tra added a subscriber: tra.Oct 25 2022, 10:35 AM

Would it be possible to split modules_include.sh.cpp and possibly other tests into multiple files, so lit could run them in parallel?

Running all of these test cases sequentially often results in this test being the lonely remaining test that continues to run long after all other tests in ninja check are done. Splitting it would noticeably reduce ninja check wall time for many users.

Would it be possible to split modules_include.sh.cpp and possibly other tests into multiple files, so lit could run them in parallel?

Running all of these test cases sequentially often results in this test being the lonely remaining test that continues to run long after all other tests in ninja check are done. Splitting it would noticeably reduce ninja check wall time for many users.

This is currently impossible because we generate the test and want to keep it in a single file. However, it would be possible to implement a test format where Lit will actually create one Lit test per run line and then those could run in parallel. So -- not impossible, but requires some work in Lit.