Page MenuHomePhabricator

[libc++] Alphabetize header inclusions. NFCI
ClosedPublic

Authored by Quuxplusone on May 19 2021, 9:00 AM.

Details

Summary

This should be a trivial NFC.
It's worth remarking that historically libc++ includes <__config> before all other headers (in case some header depends on <__config> macros). However, if all our headers include <__config>, then it doesn't really matter — maybe we include <__bit> first, but then <__bit> includes <__config>, so it all works out.

I'm not sure what's going on with the errors in https://buildkite.com/llvm-project/libcxx-ci/builds/3355 (on the Apple builds only).

Diff Detail

Unit TestsFailed

TimeTest
23,550 mslibcxx CI Apple back-deployment macosx10.15 > libc++.libcxx/modules::stds_include.sh.cpp
Script: -- : 'RUN: at line 29'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -v --target=x86_64-apple-macosx10.15 -include /tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.15/include/c++/v1 -I/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.15/projects/libcxx/include/c++build -I/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/private/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.15/projects/libcxx/test/libcxx/modules/Output/stds_include.sh.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fmodules -fcxx-modules -fsyntax-only -std=c++03 -DINVALIDATE_CACHE_CXX03 /private/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/libcxx/test/libcxx/modules/stds_include.sh.cpp
22,130 mslibcxx CI Apple back-deployment macosx10.9 > libc++.libcxx/modules::stds_include.sh.cpp
Script: -- : 'RUN: at line 29'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -v --target=x86_64-apple-macosx10.9 -include /tmp/buildkite-builds/Granger-local-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/tmp/buildkite-builds/Granger-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/include/c++/v1 -I/tmp/buildkite-builds/Granger-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/projects/libcxx/include/c++build -I/tmp/buildkite-builds/Granger-local-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/private/tmp/buildkite-builds/Granger-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/projects/libcxx/test/libcxx/modules/Output/stds_include.sh.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fmodules -fcxx-modules -fsyntax-only -std=c++03 -DINVALIDATE_CACHE_CXX03 /private/tmp/buildkite-builds/Granger-local-1/llvm-project/libcxx-ci/libcxx/test/libcxx/modules/stds_include.sh.cpp
23,320 mslibcxx CI Apple system > libc++.libcxx/modules::stds_include.sh.cpp
Script: -- : 'RUN: at line 29'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -v --target=x86_64-apple-darwin19.6.0 -include /tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system/include/c++/v1 -I/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system/projects/libcxx/include/c++build -I/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/private/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system/projects/libcxx/test/libcxx/modules/Output/stds_include.sh.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fmodules -fcxx-modules -fsyntax-only -std=c++03 -DINVALIDATE_CACHE_CXX03 /private/tmp/buildkite-builds/Weasley-local-1/llvm-project/libcxx-ci/libcxx/test/libcxx/modules/stds_include.sh.cpp
22,120 mslibcxx CI Apple system -fno-exceptions > libc++.libcxx/modules::stds_include.sh.cpp
Script: -- : 'RUN: at line 29'; /Library/Developer/CommandLineTools/usr/bin/clang++ -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -v --target=x86_64-apple-darwin20.4.0 -include /usr/local/var/buildkite-agent/builds/McGonagall-local-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/usr/local/var/buildkite-agent/builds/McGonagall-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-noexceptions/include/c++/v1 -I/usr/local/var/buildkite-agent/builds/McGonagall-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-noexceptions/projects/libcxx/include/c++build -I/usr/local/var/buildkite-agent/builds/McGonagall-local-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++20 -fno-exceptions -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/usr/local/var/buildkite-agent/builds/McGonagall-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-noexceptions/projects/libcxx/test/libcxx/modules/Output/stds_include.sh.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fmodules -fcxx-modules -fsyntax-only -std=c++03 -DINVALIDATE_CACHE_CXX03 /usr/local/var/buildkite-agent/builds/McGonagall-local-1/llvm-project/libcxx-ci/libcxx/test/libcxx/modules/stds_include.sh.cpp

Event Timeline

Quuxplusone created this revision.May 19 2021, 9:00 AM
Quuxplusone requested review of this revision.May 19 2021, 9:00 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMay 19 2021, 9:00 AM

Roll in "alphabetize all headers." Poke buildkite.

Quuxplusone retitled this revision from [libc++] Assume that __wrap_iter always wraps a fancy pointer. to [libc++] Alphabetize header inclusions. NFCI.
Quuxplusone edited the summary of this revision. (Show Details)

Merge the __wrap_iter part; keep this open for the alphabetize-headers part.

ldionne accepted this revision.May 25 2021, 7:09 AM

LGTM but let's fix the Apple issue before committing this.

This revision is now accepted and ready to land.May 25 2021, 7:09 AM
zoecarver accepted this revision.May 25 2021, 8:49 AM

Not exactly sure why the tests are failing... I couldn't reproduce locally :(

Anyway, this lgtm as well, assuming you fix the tests.

Move <__config> to the top of every file again (although I still don't see a physical reason that should matter). FWIW, I can't reproduce the Apple failures locally on my Macbook.
Poke buildbot.

git checkout main -- all of the headers involved in the Apple modules failure, https://buildkite.com/llvm-project/libcxx-ci/builds/3355
This presumably leaves some kind of trap for the next refactorer, but at least maybe it'll narrow down where the trap is.

Add __threading_support to the modulemap. I think this is going to work!

I wonder if the stds_include.sh.cpp should also use the -fmodules-local-submodule-visibility flag. The default -fmodules can't really handle top modules that include textual headers. We're using the same flag in LLVM to build since D74892 (see also rG82b47b2978405f802a33b00d046e6f18ef6a47be for why)

Quuxplusone added inline comments.May 29 2021, 1:16 PM
libcxx/include/module.modulemap
548 ↗(On Diff #348645)

@teemperor wrote:

I wonder if the stds_include.sh.cpp should also use the -fmodules-local-submodule-visibility flag. The default -fmodules can't really handle top modules that include textual headers. We're using the same flag in LLVM to build since D74892 (see also rG82b47b2978405f802a33b00d046e6f18ef6a47be for why)

I don't know enough modules to say whether that flag would help. However, I don't think the problem here is related to textual headers at all (I mean, it's not because of headers like <cassert>). The problem here seems to be that e.g. <mutex> and <thread> both #include <__threading_support>; and so the contents of header "__threading_support" were winding up duplicated in both module std.mutex and module std.thread. (We see random heisenbuggy duplication issues with other headers too, e.g. the partial specializations in "__iterator/iterator_traits.h" end up duplicated and thus ambiguous.)

What we really need is a way to "share" the common helper header __threading_support between mutex and thread (and so on). My impression is that the only(?) tool we have right now for doing that is to make __threading_support a whole module in its own right.

Personally I would be quite happy to replace all of module.modulemap with a single umbrella module std that simply includes everything; and then worry about "modularizing the STL" sometime down the road. This year in particular we seem to be having our hands full merely "headerizing" it! :)

teemperor added inline comments.May 29 2021, 2:32 PM
libcxx/include/module.modulemap
548 ↗(On Diff #348645)

Sorry, I think I should have made clear that I meant 'headers that are not in a module' when I said 'textual' (as those headers behave similar to the actual textual headers declared in a modulemap).

And yes, having all the headers (even helpers) in their own submodule would probably help a lot with the bugs caused by the duplication. I'm not familiar enough with libc++ to say how much work that would be. Just commenting as I pushed for that specific test and I don't just want to leave it to others to keep it green :)

This revision was landed with ongoing or failed builds.May 29 2021, 4:58 PM
This revision was automatically updated to reflect the committed changes.