This is an archive of the discontinued LLVM Phabricator instance.

[libc++][modules] Adds the C++23 std module.
ClosedPublic

Authored by Mordante on May 31 2023, 8:52 AM.

Details

Summary

The patch is based on D144994.

D151030 added the module definitions for the module std.
This patch wires in the module and enables the basic testing.

Some notable features are missing:

  • There is no test that libc++ can be fully imported as a module.
  • This lacks the parts for the std.compat module.
  • The module is not shipped with libc++.

Implements parts of

  • P2465R3 Standard Library Modules std and std.compat

Diff Detail

Event Timeline

Mordante created this revision.May 31 2023, 8:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 8:52 AM
Mordante added inline comments.May 31 2023, 10:29 AM
libcxx/test/libcxx/module_std.sh.cpp
7

This file needs to be rewritten once D151654 lands.

Mordante published this revision for review.Jun 1 2023, 8:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 8:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
aaronmondal added inline comments.Jun 1 2023, 12:36 PM
libcxx/CMakeLists.txt
108

For future compatiblity maybe this should be LIBCXX_ENABLE_STD_MODULES (with S) or LIBCXX_ENABLE_MODULES instead? At the moment only std is supported, but when std.compat is added later the current flag name could be misleading.

Or should there be different flags for std and std.compat? IMO they should both be covered by the same flag.

libcxx/docs/Modules.rst
66

As a safeguard we might want to add the note that the BMIs shouldn't be distributed at the moment. AFAIK BMIs still contain absolute paths to their build environment. I.e. if you build at /my/custom/build/machine this path will be printed if you run strings on the BMIs. This could leak information from a distributors build environment to the BMI.

libcxx/modules/CMakeLists.txt.in
63

I believe only https://github.com/llvm/llvm-project/issues/62844 breaks -Werror when building with C++23. Alternatively, -Wno-deprecated-declarations might also allow use of -Werror.

libcxx/utils/ci/run-buildbot
191–192

build2 encourages the extensions mxx and mpp. Adding those here might save them a downstream patch :D

libcxx/utils/generate_header_tests.py
85

Unrelated, to this patch. Some styleguides consider nasty as "unlikely profanity". There might be a better way to name this.

libcxx/utils/libcxx/test/dsl.py
446–447

Typo? Looks like this should fall back to std = '23' or just be std = '26'?

Thanks for the review!

libcxx/CMakeLists.txt
108

They indeed should be the same flag. The reason why this is an option is due to the additional version requirements of CMake, ninja, and Clang. These are the same for std and std.compat. The intention is to remove this option at some point in the future.

I should add a TODO MODULES to make that clear.

libcxx/docs/Modules.rst
66

I'm not sure that is needed. I feel there are already quite some warnings that BMIs are not portable. At least on Debian in the past, debug packages "leaked" the same information, which didn't seem to be an issue.

Do you have reasons why this is a real issue?

libcxx/modules/CMakeLists.txt.in
63

-Werror should be removed, that's left from testing. Nice catch!

I really don't want this since users could add other compiler flags that will cause warnings to be issued.

libcxx/utils/ci/run-buildbot
191–192

This only tests formatting extensions used in libc++. For example I recently removed hxx and cxx since we don't use them. The main reason to specify these manually is that we want to format files without any extension. For example string.

libcxx/utils/libcxx/test/dsl.py
446–447

No this is intended, hence the TODO. It seems modules and C++26 fail. I haven't gotten around to investigate this further and possibly file a bug. After this patch has landed it's easier to create a nice reproducer.

aaronmondal added inline comments.Jun 1 2023, 4:31 PM
libcxx/docs/Modules.rst
66

If you have a build that indexes cached artifacts by hash this can lead to confusing cache behavior for incremental builds. Having the cache poisoned by this essentially forces you to completely delete it and rebuild everything. For LLVM this is roughly 200GB of wasted cache transfer and ~10 hours of wasted single thread CPU minutes. This probably isn't an issue for most users though and since modules are experimental anyways it's probably a non-issue.

Thanks! This looks good overall.

There is no test that libc++ can be fully imported as a module.

What's the plan to add some test to use import std; in the source code? I remember you had a patch to use import std; for the coroutines tests. But I can't find it now. For me, it is better to have at least one import std; in the test of the tree. (But this is not necessary to land it in this patch, of course)

libcxx/docs/Contributing.rst
38

nit: in my imagination, in the future, this can be solved automatically:

#if ...
import std;
#else
#include <stl_headers>
#endif

use of new decl... // error if the author forget to touch std modules.

(this is not a requirement to change the note)

libcxx/docs/Modules.rst
8

nit: a silly English question. Is it ok to use *s are and an together in a sentence? I got confused several times before.

28
134–135

This was mitigated recently: https://github.com/llvm/llvm-project/blob/15a719de01b92da7de4b8381660525b622c2c292/clang/lib/Driver/ToolChains/Clang.cpp#L3688-L3697. But personally I prefer -std=c++xx instead of -std=gnu++xx.

165

This is not what we talked about implicit paths. The implicit paths is that the BMI will contain paths to their dependency implicitly. e.g,

// b.cppm
export module b;
export int b() {
    return 43;
}

// a.cppm
export module a;
import b;
export int a() {
    return b() + 43;
}

// user.cpp
import a;
int use() {
    return a();
}

Now it is ok to compiler user.cpp without specify the path to the BMI of module b. It is different from disabling -fprebuilt-module-path.

libcxx/docs/ReleaseNotes.rst
147

nit: maybe it is helpful to mention the above change. Otherwise the readers who don't read the whole document may get confused.

libcxx/modules/CMakeLists.txt
3

nit: consider to specify 3.26 clearly in the message?

libcxx/modules/CMakeLists.txt.in
2

nit: CMake3.25 only has the support MSVC and the support for clang was added in 3.26.

Arthapz added a subscriber: Arthapz.Jun 2 2023, 1:53 AM

Hello, i'll add the support of this patch in XMake today :)

(BTW, we lack a [ in the title.)

Mordante retitled this revision from libc++][modules] Adds the C++23 std module. to [libc++][modules] Adds the C++23 std module..Jun 3 2023, 4:18 AM
Mordante marked an inline comment as done.Jun 3 2023, 4:27 AM

Hello, i'll add the support of this patch in XMake today :)

Great, thanks :-)

(BTW, we lack a [ in the title.)

Thanks, fixed.

libcxx/docs/Contributing.rst
38

This is about adding new declarations to the .cppm files. For example when somebody implements parts of mdspan these new classes need to be added to the .cppm files. If you forget to do that, the CI will fail.

libcxx/docs/Modules.rst
134–135

Thanks for the info. I too prefer to disable extensions. I'll remove these two lines.

165

Interesting. When I removed the fprebuilt-module-path I got these warnings.

Arthapz added a comment.EditedJun 3 2023, 3:39 PM

do you already know where the modules will be installed (i know modules are not installed atm, it's for simulation purpose :D) ? (like /usr/modules, /usr/module, etc...)

Mordante marked 14 inline comments as done.Jun 4 2023, 2:53 AM

do you already know where the modules will be installed (i know modules are not installed atm, it's for simulation purpose :D) ? (like /usr/modules, /usr/module, etc...)

It has already been discussed, but there's no conclusion yet. I want to revisit that discussion soon. That's the main reason not to install modules in this patch.

libcxx/docs/Modules.rst
8

I think it's correct, but I'm not a native speaker.

28

I think the current text is correct.

66

That seems like a non-typical use case. I prefer to leave that information out.

134–135

Actually it turns our this doesn't work correctly with libc++. There is an issue with the new partition. This is named __new since new is a keyword. For now I require extensions of and blame libc++ instead of clang.

This should be fixed, but for now I keep it as is and add it to the list of known limitations.

165

I removed the comment and left the compiler flag.

libcxx/test/libcxx/module_std.sh.cpp
7

This file will be removed a module_std.gen.py is available. It's still here since the new file is not tested in the CI yet.

Mordante updated this revision to Diff 528198.Jun 4 2023, 2:59 AM
Mordante marked 3 inline comments as done.

Addresses review comments
Rewritten the module verification test, using the new generated lit style.

Mordante updated this revision to Diff 528202.Jun 4 2023, 4:08 AM

CI fixes.

LGTM now. Thanks!

aaronmondal accepted this revision.Jun 5 2023, 9:13 AM
ldionne requested changes to this revision.Jun 6 2023, 10:26 AM
ldionne added a subscriber: var-const.

Thanks, this looks really good but I recorded the discussion we had just now in comments.

libcxx/test/libcxx/module_std.gen.py
46 ↗(On Diff #528202)

I would make this a list, and then use ' '.join(SkipDeclarations[header] when you need to output it.

58–60 ↗(On Diff #528202)

I really question what black is doing here with formatting. I don't think this is sensible:

SkipDeclarations[
    "filesystem"
]

If that's what the tool generates, I argue the tool's not configured properly.

ldionne added inline comments.Jun 6 2023, 10:26 AM
libcxx/docs/Modules.rst
59
74–76
95–96
110–111
libcxx/docs/ReleaseNotes.rst
114–120

I would move this to Build System Changes. Also, I think we can just change it, no need for a deprecation period here. We can consider our Lit test suite as something internal -- even though some vendors may have to make a few changes, those are easy to make and they don't affect end-users. Let's not go through more hoops than we need to.

147–149

This can go.

libcxx/test/libcxx/module_std.gen.py
66 ↗(On Diff #528202)
91–93 ↗(On Diff #528202)
95 ↗(On Diff #528202)
128 ↗(On Diff #528202)

This makes it a bit clearer what the file contains.

150 ↗(On Diff #528202)
156 ↗(On Diff #528202)
157 ↗(On Diff #528202)

I think you can just test for if skip_declarations since an empty list/string is false-y in Python.

161 ↗(On Diff #528202)
194 ↗(On Diff #528202)
223–229 ↗(On Diff #528202)

I don't think this is needed at all.

libcxx/test/libcxx/modules_include.sh.cpp
1

I think you need to rebase onto main.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
24
61–70

We should figure out why std::views::split(*list, ' ') stopped working at some point and use that instead. You can loop in @var-const when you have a reproducer. Ditto below.

If this ends up needing a fix in <ranges>, I won't block the review on this comment of course but it would be nice to at least see if it's just a dumb mistake.

135

Per our discussion, I don't think this is an issue.

181–190

I think we can do

return name.starts_with("std::__") || (name.starts_with("std::_) && std::isupper(name[6]));
libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.hpp
12

I think we should IWYU properly here.

17

IDK what's the usual convention here, but do we want this to be explicit?

libcxx/utils/ci/buildkite-pipeline.yml
149
libcxx/utils/ci/run-buildbot
407–408

Can we do this in buildkite-pipeline.yml instead? Otherwise we can't run this job locally.

libcxx/utils/generate_header_tests.py
76

This will go away upon rebasing.

libcxx/utils/libcxx/test/dsl.py
446
448
libcxx/utils/libcxx/test/params.py
80–84

Not necessary.

138–142

I think we could simplify the CMake/Lit integration quite a bit by instead just building the BMIs from the top-level CMake invocation instead of from within Lit itself. Then we'd only need to pass the value for -fprebuilt-module-path to Lit and we wouldn't need AddModule(), the bridge changes, and also we might not need to use absolute paths everywhere.

I think this is worth investigating since it would simplify this patch quite a bit.

This revision now requires changes to proceed.Jun 6 2023, 10:26 AM
Mordante marked 28 inline comments as done.Jun 7 2023, 11:06 AM

Thanks for the reviews!

libcxx/test/libcxx/module_std.gen.py
58–60 ↗(On Diff #528202)

It's indeed black's doing. Changing this to lists improves the formatting.

91–93 ↗(On Diff #528202)

I just added it directly to the list, without an append. The comment can be in the list.

156 ↗(On Diff #528202)

This doesn't work, it might try to add [] to a string.
Instead I assigned the joined list to skip_declarations.

libcxx/test/libcxx/modules_include.sh.cpp
1

I will do that shortly before our next review, then I can move the changes to the .cppm files in their own review.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
61–70

Odd taking the code from D144994 now works again :-/ I didn't investigate why. I much rather use that code, there is a reason why I used that code in the first place.

181–190

I tried that and it doesn't work. Three are elements in std::ranges too. I updated the comment.

libcxx/utils/ci/buildkite-pipeline.yml
149

As mention during review I kept std in the name since std.compat needs its own job.

libcxx/utils/libcxx/test/dsl.py
446–447

@aaronmondal actually I think I know what the issue is. CMake does not know CMAKE_CXX_STANDARD 26. The current development version of CMake has this support for Clang.

libcxx/utils/libcxx/test/params.py
138–142

I tried that and it doesn't work. The module needs to be build with the flags given to lit and the standard version used. Since this can change when lit is invoked we need to do that during the test.

What I did is building a module from CMake and then rebuilt it from the lit test. That way I need 2 instead of 4 new elements in the bridge. I feel the {modules} directory is not too bad to have, it matches {includes}. Only CMake is an oddity, I hope/expect that can be removed once CMake has proper module support.

Mordante updated this revision to Diff 529375.Jun 7 2023, 11:11 AM
Mordante marked 4 inline comments as done.

Addresses review comments.

Mordante updated this revision to Diff 529436.Jun 7 2023, 2:29 PM

CI fixes.

Mordante updated this revision to Diff 529507.Jun 7 2023, 10:55 PM

CI fixes.

Adds some notes for reviewing.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
61–70

I turns out that this code isn't compiled against libc++, but against the system C++ library. So that might explain the earlier failure I saw; I probably tested it on a system with an older GCC. (I noticed it when looking at the output of the most recent CI failure.)

62

The code is not used for Clang < 17 since these compilers lack module support. However there is a working implementation for these versions of Clang. The original code fails in https://buildkite.com/llvm-project/libcxx-ci/builds/25748. Since we'll drop LLVM 15 support rather soon, I prefer this work-around for the short term.

Looking at the build log, it seems to be an issue with concepts:

clang_tidy_checks/header_exportable_declarations.cpp:9:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyCheck.h:12:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyDiagnosticConsumer.h:12:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyOptions.h:12:

In file included from /usr/lib/llvm-17/include/llvm/ADT/IntrusiveRefCntPtr.h:66:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/memory:66:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tempbuf.h:61:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_construct.h:61:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_iterator_base_types.h:71:

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/iterator_concepts.h:999:13: error: no matching function for call to '__begin'

        = decltype(ranges::__cust_access::__begin(std::declval<_Tp&>()));

                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/ranges_base.h:513:5: note: in instantiation of template type alias '__range_iter_t' requested here

    using iterator_t = std::__detail::__range_iter_t<_Tp>;

    ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/ranges_util.h:131:36: note: in instantiation of template type alias 'iterator_t' requested here

      requires contiguous_iterator<iterator_t<_Derived>>

                                   ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1134:29: note: in instantiation of template class 'std::ranges::view_interface<std::ranges::ref_view<llvm::StringRef>>' requested here

    class ref_view : public view_interface<ref_view<_Range>>

                            ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:38: note: in instantiation of template class 'std::ranges::ref_view<llvm::StringRef>' requested here

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                            ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:38: note: in instantiation of requirement here

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:27: note: (skipping 8 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                 ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3717:6: note: while substituting template arguments into constraint expression here

          = requires { split_view(std::declval<_Range>(), std::declval<_Pattern>()); };

            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3723:11: note: while checking the satisfaction of concept '__can_split_view<llvm::StringRef &, char>' requested here

        requires __detail::__can_split_view<_Range, _Pattern>

                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3723:11: note: while substituting template arguments into constraint expression here

        requires __detail::__can_split_view<_Range, _Pattern>

                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Mordante added inline comments.
libcxx/modules/std/string.cppm
74

Except for this change all other changes in this directory should be in a separate commit. For now I keep them here. @H-G-Hristov is making great progress on P1614 (the spaceship paper). This means a lot of changes to the to the modules. After this patch is accepted I will coordinate with @H-G-Hristov regarding splitting this patch and avoid conflicts with the spaceship patches.

Mordante updated this revision to Diff 530523.Jun 12 2023, 8:50 AM

Rebased.
Added CMake 3.27 suppprt, by adding the new experimental UUID.
Fixed the changes in the upstream headers. The ongoing spaceship patches change
a lot of modules.

This pretty much looks good to me. I'd like to see it one last time after applying the comments but I think we can ship it once those comments have been applied. Thanks for all the work you did on this!

libcxx/docs/index.rst
121–125

I think I would drop this note. Modules are very experimental at the moment, and adding a ref to that from our main documentation seems unnecessary and perhaps even undesirable.

libcxx/modules/CMakeLists.txt.in
42–44

This has been rolled up into LIBCXX_ENABLE_FILESYSTEM now, so LIBCXX_ENABLE_FSTREAM doesn't exist anymore.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
61–70

The question at hand is really what version of standard libraries do we support in our build tools. We've never had that question because we have very little C++ in our build tools (only the clang tidy checks) -- everything else is pretty much Python. I think what we want to do here is enforce that our build tools (that means anything we use in order to build or test the library) are written using the following requirements:

  1. C++17 (like the rest of non-libc++ LLVM)
  2. The compilers supported by libc++ (which are stricter than those supported by LLVM)
  3. The standard libraries supported by the rest of non-libc++ LLVM

Concretely, this means that we basically lower all of our restrictions to be the same as non-libc++ LLVM for our build tools. I think this makes sense and should have little impact on us.

We could document that under tools/README.md or in our other rst documentation around contributing. Let's make it clear that this is only for build tools, though. I would also do this on a best effort basis, i.e. I wouldn't go crazy and try to add a build job that checks our minimum requirement, that seems overkill (especially given the load on our CI).

libcxx/utils/libcxx/test/dsl.py
440
462–463
libcxx/utils/libcxx/test/params.py
134

I don't think this comment is relevant anymore.

147–152

I think what you meant here is AddLinkFlag(os.path.join(cfg.test_exec_root, "__config_module__/libc++std.a")).

Also, it would be interesting to understand why your tests worked without linking against the std module as-is.

Mordante marked 9 inline comments as done.Jun 13 2023, 10:11 PM

This pretty much looks good to me. I'd like to see it one last time after applying the comments but I think we can ship it once those comments have been applied. Thanks for all the work you did on this!

Thanks a lot for your review!

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
61–70

I will do that in a followup patch.

libcxx/utils/libcxx/test/params.py
147–152

It seems to work without it. It might be something was/is needed for the std.compat module. For now I removed it.

Mordante updated this revision to Diff 531170.Jun 13 2023, 10:14 PM
Mordante marked an inline comment as done.

Rebased.
Removed more operator!=
Addresses review comments

ldionne accepted this revision.Jun 15 2023, 9:01 AM
ldionne added inline comments.
libcxx/test/libcxx/modules_include.gen.py
17 ↗(On Diff #531170)

We should clean up the naming of these files in a separate patch to make it clear what's testing C++20 modules and what's testing Clang modules.

This revision is now accepted and ready to land.Jun 15 2023, 9:01 AM
Mordante marked an inline comment as done.Jun 15 2023, 9:14 AM
Mordante added inline comments.
libcxx/modules/std/string.cppm
74

I checked there are no open operator!= patches.

libcxx/test/libcxx/modules_include.gen.py
17 ↗(On Diff #531170)

Good point. I'll make a followup patch for the renaming after landing this patch.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.
Mordante marked an inline comment as done.Jun 15 2023, 10:21 AM
Mordante added inline comments.
libcxx/test/libcxx/modules_include.gen.py
17 ↗(On Diff #531170)

I've created D153042

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
61–70

I've created D152377.

ldionne added inline comments.Jun 15 2023, 10:47 AM
libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
61–70

I think that's referencing the wrong patch.

bruno added a subscriber: bruno.Jun 16 2023, 4:27 AM