This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][module-map] creates submodules for private headers
ClosedPublic

Authored by cjdb on Jun 2 2021, 12:53 PM.

Details

Summary

Most of our private headers need to be treated as submodules so that
Clang modules can export things correctly. Previous commits that split
monolithic headers into smaller chunks were unaware of this requirement,
and so this is being addressed in one fell swoop. Moving forward, most
new headers will need to have their own submodule (anything that's
conditionally included is exempt from this rule, which means __support
headers aren't made into submodules).

This hasn't been marked NFC, since I'm not 100% sure that's the case.

Diff Detail

Event Timeline

cjdb requested review of this revision.Jun 2 2021, 12:53 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 12:53 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 349357.Jun 2 2021, 12:55 PM

fixes benign typo

Seems surprisingly painless.
You did miss the other "libc++ classic style" helper headers, e.g. __functional_03; did you want to get those in this same PR?

libcxx/include/module.modulemap
303

requires cplusplus20? or what's the rationale for adding that on various headers?
E.g. I see <latch> is marked with requires cplusplus14, even though it's also new-in-C++20.

469–476

I like this one-line-per-header formatting. Please also use it for __utility and __format and __algorithm.

ldionne accepted this revision.Jun 2 2021, 1:55 PM

This LGTM if CI passes, and with Arthur's comments. Thanks a lot for fixing this. As we just discussed, I will add a CI configuration that runs the test suite and used -fmodules -fcxx-modules, which should allow us to catch even more issues, and catch them before they break other people in the future.

This revision is now accepted and ready to land.Jun 2 2021, 1:55 PM
cjdb updated this revision to Diff 349393.Jun 2 2021, 3:05 PM
  • removes __algorithm/unwrap_iter.h
  • modularises most global __headers
cjdb updated this revision to Diff 349394.Jun 2 2021, 3:08 PM
cjdb marked 2 inline comments as done.

rebases

cjdb added a comment.Jun 2 2021, 3:08 PM

Seems surprisingly painless.
You did miss the other "libc++ classic style" helper headers, e.g. __functional_03; did you want to get those in this same PR?

Done for all but __functional_(base_)?03 since they're designed to be textual includes.
I think adjusting those deserves its own patch.

libcxx/include/module.modulemap
303

Nice catch. For latch specifically, see D68480. Looks like libc++ supports it from C++14 onward.

469–476

Thanks, didn't notice I missed a few.

cjdb updated this revision to Diff 349444.Jun 2 2021, 8:33 PM

rebases to reactivate CI

Thanks a lot for working on this! And thanks @ldionne for setting up a CI to catch these issues earlier!

libcxx/include/module.modulemap
377

Just for my understanding. This header is C++20. Do headers in the internal module not set their own requires?

cjdb updated this revision to Diff 349570.Jun 3 2021, 8:56 AM

rebases to activate CI

This revision was automatically updated to reflect the committed changes.
cjdb added a comment.Jun 4 2021, 9:44 AM

Answered @Mordante's question (I forgot to press submit down here).

libcxx/include/module.modulemap
377

It's unclear to me whether or not we can put requires cplusplus20 on because we unconditionally include these headers.

phosek added a subscriber: phosek.Jun 6 2021, 1:49 PM

After this change landed, we started seeing a number of crashes in our CI when using the just built Clang to run libc++ tests (that is using the runtimes build):

Failed Tests (6):
  libc++ :: libcxx/modules/cinttypes_exports.compile.pass.cpp
  libc++ :: libcxx/modules/clocale_exports.compile.pass.cpp
  libc++ :: libcxx/modules/cstdint_exports.compile.pass.cpp
  libc++ :: libcxx/modules/inttypes_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stdint_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stds_include.sh.cpp

They all fail with the same error:

clang++: clang/lib/Serialization/ASTWriter.cpp:2502: unsigned int clang::ASTWriter::getSubmoduleID(clang::Module *): Assertion `(ID || !Mod) && "asked for module ID for non-local, non-imported module"' failed.
clang++: llvm/include/llvm/Bitstream/BitstreamWriter.h:124: llvm::BitstreamWriter::~BitstreamWriter(): Assertion `BlockScope.empty() && CurAbbrevs.empty() && "Block imbalance"' failed.
clang-13: error: clang frontend command failed with exit code 134 (use -v to see invocation)
cjdb added a comment.Jun 6 2021, 1:57 PM

After this change landed, we started seeing a number of crashes in our CI when using the just built Clang to run libc++ tests (that is using the runtimes build):

Failed Tests (6):
  libc++ :: libcxx/modules/cinttypes_exports.compile.pass.cpp
  libc++ :: libcxx/modules/clocale_exports.compile.pass.cpp
  libc++ :: libcxx/modules/cstdint_exports.compile.pass.cpp
  libc++ :: libcxx/modules/inttypes_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stdint_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stds_include.sh.cpp

They all fail with the same error:

clang++: clang/lib/Serialization/ASTWriter.cpp:2502: unsigned int clang::ASTWriter::getSubmoduleID(clang::Module *): Assertion `(ID || !Mod) && "asked for module ID for non-local, non-imported module"' failed.
clang++: llvm/include/llvm/Bitstream/BitstreamWriter.h:124: llvm::BitstreamWriter::~BitstreamWriter(): Assertion `BlockScope.empty() && CurAbbrevs.empty() && "Block imbalance"' failed.
clang-13: error: clang frontend command failed with exit code 134 (use -v to see invocation)

Interesting. Our scheduled Runtimes build is passing, so I don't know what's up there. Could you please link to one of the failing builds? I'd like to read the diagnostics.

cjdb added a comment.Jun 6 2021, 3:38 PM

Is this for Chromium or Fuchsia? Either way, I'll take a look at it tomorrow morning.

phosek added a comment.Jun 7 2021, 9:50 AM

Is this for Chromium or Fuchsia? Either way, I'll take a look at it tomorrow morning.

It's for Fuchsia (we use Chromium infrastructure hence the URL).