This is an archive of the discontinued LLVM Phabricator instance.

[libc++][Modules] Make top level modules for all C++ headers with OS/clang versions
ClosedPublic

Authored by iana on Feb 18 2023, 12:14 AM.

Details

Summary

The headers that include_next compiler and OS headers need to be in different top level modules in order to avoid module cycles. e.g. libc++'s stdlib.h will #include_next stdlib.h from the compiler and then the C library. Either of those are likely to include stddef.h, which will come back up to the libc++ module map and create a module cycle. Putting stdlib.h and stddef.h (and the rest of the C standard library headers) in top level modules resolves this by letting the order go cxx_stdlib_h -> os_stdlib_h -> cxx_stddef_h -> os_stddef_h.

All of those headers' dependencies then need to be moved into top level modules themselves to avoid module cycles between the new top level level cstd modules. This starts to get complicated, as the libc++ C headers, by standard, have to include many of the C++ headers, which include the private detail headers, which are intertwined. e.g. some __algorithm headers include __memory headers and vice versa.

Make top level modules for all of the libc++ headers to easily guarantee that the modules aren't cyclic.

Add enough module exports to fix check-cxx and run-buildbot generic-modules.

__stop_token/intrusive_shared_ptr.h uses __atomic/atomic.h but has no include path to it. Add that include.
math.h absorbs bits/atomic_wide_counter.h on some platforms that don't have modules, work around that by including math.h in __threading_support.
<mutex> doesn't actually require threads, there are a few pieces like once_flag that work without threads. Remove the requirement from its module.
AIX is no longer able to support modular builds.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
iana updated this revision to Diff 534098.Jun 23 2023, 3:32 PM

Update for D153213

iana updated this revision to Diff 534105.Jun 23 2023, 3:51 PM

Pick up changes from D153213

iana updated this revision to Diff 534157.Jun 23 2023, 9:49 PM

Update with D153212 changes

iana updated this revision to Diff 534158.Jun 23 2023, 10:18 PM

Updated a comment in the module map generator

iana updated this revision to Diff 535181.Jun 27 2023, 3:58 PM

Fix more CI failures

iana updated this revision to Diff 535643.Jun 28 2023, 11:34 PM

Update private module attributes with changes from D153832

iana updated this revision to Diff 535909.Jun 29 2023, 10:57 AM

Update private module attributes with changes from D153211

iana updated this revision to Diff 536371.Jun 30 2023, 12:23 PM

Update private module attributes with changes from D153832

iana updated this revision to Diff 536462.Jun 30 2023, 3:37 PM

Take D153216 out of the stack

iana updated this revision to Diff 536517.Jun 30 2023, 11:50 PM

Fix the tests after removing D153216 from the stack.

iana updated this revision to Diff 536537.Jul 1 2023, 10:07 AM

Try making everything export *

iana updated this revision to Diff 536792.Jul 3 2023, 9:11 AM

LIBCXX_GENERATED_INCLUDE_TARGET_DIR isn't always set.

iana updated this revision to Diff 536901.Jul 3 2023, 2:27 PM

Rebase

iana updated this revision to Diff 536952.Jul 3 2023, 10:46 PM

Fix a syntax error in the json file (forgot to enclose a string in quotes)
Make the module map generator script compatible with Python 3.8 (use typing.List[xxx] instead of list[xxx])
Dump stderr when the clang command is failing to get header includes

iana updated this revision to Diff 537209.Jul 4 2023, 9:04 PM

Fix a typing error in the generator script

iana updated this revision to Diff 537223.Jul 4 2023, 10:54 PM

Rebase, and put back on top of D153216

iana updated this revision to Diff 537404.Jul 5 2023, 10:03 AM

Make the module map generator script compatible with Python 3.7 (don't use typing.TypedDict)

iana updated this revision to Diff 537583.Jul 5 2023, 10:22 PM
iana marked 2 inline comments as done.

More Python 3.7 fixes (removeprefix/removesuffix)
Adjust for the changes from D153216

iana updated this revision to Diff 537586.Jul 5 2023, 10:38 PM

Actually fix the generator script for Python 3.7
Run black to format it

iana added a comment.Jul 5 2023, 10:42 PM

We had a meeting at Apple today including @ldionne and he's of the opinion that this script will be too difficult to maintain. We agreed to keep the static module map, but split it so that every header is a top level module. Depending on how that approach tests, especially with build performance, we might come back to the script, so I'm trying to run it through the CI just to make sure it runs properly on the builders with some older Python versions. Plus the other CI errors are from splitting the module and not necessarily from the specifics of how they end up split. I'll try to get this updated for the static module map tomorrow anyway.

libcxx/docs/ReleaseNotes.rst
86

Maybe? If anyone does @import std; in Objective-C++ then they'll notice

libcxx/include/__threading_support
43

No, this is only necessary when the mega module gets split. This one comes from a unit test, and I'm not sure how practical it is. I don't know how many people are trying to use libc++ with clang modules on OSes that don't have any clang modules in the OS headers. It's not really a supported configuration, clang modules have the expectation that modular headers only include modular headers.

What happens today is that libc++ has a single module, and if nothing in the OS has a module than the libc++ module will absorb all of the OS headers it includes into one huge gigantic mega module that has the whole world in it. In that case, everything can see everything else. But when the mega module breaks up, the non-modular OS headers will be randomly assigned, in this case the CI indicates that math.h is absorbing pthread.h/atomic_wide_counter.h

iana updated this revision to Diff 537856.Jul 6 2023, 1:23 PM

Rebase

iana updated this revision to Diff 537987.Jul 6 2023, 10:07 PM

Fix a bad merge

iana updated this revision to Diff 538209.Jul 7 2023, 11:05 AM

Add quotes to the CMake variable

iana updated this revision to Diff 538276.Jul 7 2023, 2:50 PM

Fix the CMake again

iana updated this revision to Diff 538293.Jul 7 2023, 4:01 PM

Fix a bug in the clang error debugging

iana updated this revision to Diff 538327.Jul 8 2023, 12:28 AM

Fix for Windows and parts disabled.

iana updated this revision to Diff 538381.Jul 8 2023, 11:42 AM

Debug the script on Windows

iana updated this revision to Diff 538388.Jul 8 2023, 12:59 PM

Remove bad export

iana updated this revision to Diff 538737.Jul 10 2023, 10:42 AM

More Window script debugging

iana updated this revision to Diff 538857.Jul 10 2023, 3:40 PM

Try to fix the script on Windows by forcing posixpath

iana updated this revision to Diff 539415.Jul 12 2023, 12:36 AM

Fix the module generation script on Windows

iana updated this revision to Diff 539627.Jul 12 2023, 10:19 AM

ios can't see char_traits even though it's included via __locale -> string and everything exports *

iana updated this revision to Diff 539695.Jul 12 2023, 1:02 PM

Re-uploading to see if that makes the tests run this time

iana updated this revision to Diff 539697.Jul 12 2023, 1:06 PM

Run black on the Python generator script

iana updated this revision to Diff 539802.Jul 12 2023, 5:17 PM

There are concerns of maintainability with generating the module map. Fall back on making a top level module for every single header.

iana edited the summary of this revision. (Show Details)Jul 12 2023, 5:17 PM
iana updated this revision to Diff 540224.Jul 13 2023, 4:48 PM

requires aren't inherited from parent modules, undo that translation.
__threading_support is running into problems from time.h on AIX.

iana updated this revision to Diff 540505.Jul 14 2023, 11:15 AM

Pick up changes from D155116 and D153935

iana updated this revision to Diff 540602.Jul 14 2023, 5:32 PM

Fix an include order linter issue

iana updated this revision to Diff 541103.Jul 17 2023, 9:50 AM

The <time.h> workaround didn't work, mark AIX as unsupported for modules.
<mutex> has a few pieces that are needed even when threading is disabled.

iana edited the summary of this revision. (Show Details)Jul 17 2023, 9:55 AM
ldionne added inline comments.Jul 17 2023, 1:26 PM
libcxx/test/libcxx/strings/char.traits/char.traits.specializations/arbitrary_char_type.deprecated.verify.cpp
9–10 ↗(On Diff #541103)

We should have a Clang bug report number associated to this, and it should be XFAIL: modules-build && clang-<version>.

iana updated this revision to Diff 541273.Jul 17 2023, 5:04 PM

Review feedback, fix the CI

iana edited the summary of this revision. (Show Details)Jul 17 2023, 5:05 PM
iana marked 3 inline comments as done.Jul 17 2023, 8:06 PM
iana edited the summary of this revision. (Show Details)Jul 17 2023, 11:36 PM
iana updated this revision to Diff 541331.Jul 17 2023, 11:37 PM

Apparently arbitrary_char_type.deprecated.verify.cpp is working now...

iana added a comment.Jul 18 2023, 10:49 PM

Does this error make sense to anyone?

/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/f1-1-macminivault-com/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1/__utility/pair.h:368:17: error: 'std::pair<const char *, const char *>::operator=' from module 'std_private_utility_pair_fwd' is not present in definition of 'std::pair<const char *, const char *>' in module 'std_private_utility_pair_fwd'

    const pair& operator=(pair const& __p) const

                ^
ldionne added inline comments.Jul 19 2023, 11:53 AM
libcxx/test/libcxx/private_headers.gen.py
1 ↗(On Diff #541733)

Please find instances of -Wno-private-header in the test suite and remove them, since this diagnostic won't get triggered anymore.

iana marked an inline comment as done.Jul 19 2023, 11:57 AM
iana updated this revision to Diff 542154.EditedJul 19 2023, 12:33 PM

__stop_token/intrusive_shared_ptr.h can't include __stop_token/stop_state.h when threads are disabled, directly include __atomic/atomic.h

iana edited the summary of this revision. (Show Details)Jul 19 2023, 12:34 PM
iana updated this revision to Diff 542189.Jul 19 2023, 2:16 PM

Update transitive includes

iana updated this revision to Diff 542218.Jul 19 2023, 3:23 PM

More transitive includes

iana updated this revision to Diff 542294.Jul 19 2023, 9:11 PM

Yet more transitive includes

ldionne accepted this revision.Jul 20 2023, 12:45 PM

Thanks for all the work on this. I know this has been extremely difficult: this is the worst kind of change since all other changes conflict with it, but you kept moving forward and you're finally there.

Please merge this change as soon as possible. It is probable that other changes will conflict with it in the meantime. If someone gets here because of a failed CI or similar, please let us know but let us handle this without eagerly reverting. There is no way to merge this change but to accept that we might break the trunk for a few hours, otherwise we'd have to basically halt all other libc++ development while we wait for this to go through CI and land.

@iana So please land this ASAP and monitor this thread. I'll also keep an eye out for any issues in the next few hours.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 20 2023, 12:49 PM
This revision was automatically updated to reflect the committed changes.

If someone gets here because of a failed CI or similar, please let us know but let us handle this without eagerly reverting. There is no way to merge this change but to accept that we might break the trunk for a few hours, otherwise we'd have to basically halt all other libc++ development while we wait for this to go through CI and land.

I missed it last week, but this did break the LLDB bots:

https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/2953/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/57861/

Could you take a look please?

iana added a comment.Jul 24 2023, 11:31 AM

If someone gets here because of a failed CI or similar, please let us know but let us handle this without eagerly reverting. There is no way to merge this change but to accept that we might break the trunk for a few hours, otherwise we'd have to basically halt all other libc++ development while we wait for this to go through CI and land.

I missed it last week, but this did break the LLDB bots:

https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/2953/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/57861/

Could you take a look please?

Looking today

libcxx/test/libcxx/lint/lint_headers.sh.py