This is an archive of the discontinued LLVM Phabricator instance.

[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

Event Timeline

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)

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.