[libcxx-commits] [PATCH] D144994: [Draft][libc++][modules] Adds std module.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 21 09:38:52 PDT 2023
Mordante marked 5 inline comments as done.
Mordante added a comment.
In D144994#4208434 <https://reviews.llvm.org/D144994#4208434>, @ChuanqiXu wrote:
>> Add CI jobs to test modules with parts disabled.
>
> So, is the CI going to be ready?
Not at the moment. There are some CI issues not related to modules which should be resolved first. However I am testing with the CI configuration locally. I'm still fixing bugs in the modules and investigating issue which might be Clang bugs.
I'm also still busy to remove symbols from the global namespace so I can start to test the `std` module, for now I only test with the `std.compat` module.
================
Comment at: libcxx/docs/Modules.rst:25-26
+depend on the compiler used and its compilation flags. Therefore there needs to
+be a way to distribute the ``.cppm`` files to the user and offer a way for them
+to build and use the ``.pcm`` files.
+
----------------
ben.boeckel wrote:
> Note that buildsystems should handle this. I don't know how viable the "just run the compiler by hand" is in the long-term…
I fully agree. For now we will also ship a CMake file to aid users. But once build systems catch up that CMake file will be removed. At the moment we're not even sure where our `.cppm` files will be installed, so asking buildsystems to support libc++ is a bit premature. Having this information makes it possible for early adaptors to get started.
================
Comment at: libcxx/docs/Modules.rst:162-164
+The libraries and/or executables that use the ``std`` module need to link
+against the ``std`` library. This is needed for CMake to get the proper module
+dependencies.
----------------
ben.boeckel wrote:
> Mordante wrote:
> > ben.boeckel wrote:
> > > Is this intended to be a stable interface? IMO, support for std modules needs to come from CMake because this is going to be different for every compiler otherwise.
> > I agree this is not the way users should use it. But without build system support this is how it works now.
> > So I feel this is "good enough" for early adapters. However when we (libc++) know how we want to distribute it
> > we should reach out to build system vendors and ask them to add proper support in their tools.
> >
> > I'm not even sure what direction we want to take. For the experimental library we have a Clang flag which
> > automatically link that library. Do we want to do something similar here or leave it entirely in the hands
> > of the build system?
> I think that some build-system agnostic data (similar to the MSVC STL JSON file linked to on Discourse is best). I don't think every compiler trying to chase umpteen build tool integrations (as compilers tend to be *far* harder to upgrade in the Real World than build system tools IME).
>
> ISO C++'s SG15 is the best place for communication IMO.
I will consider attending in SG15 to discuss this.
================
Comment at: libcxx/docs/Modules.rst:136
+The modules will shipped in a directory structure similar to the include.
+Includes are stored in ``<PREFIX>/modules/c++/v1`` modules will be stored in
+``<PREFIX>/modules/c++/v1/``.
----------------
ruoso wrote:
> ruoso wrote:
> > aaronmondal wrote:
> > > tschuett wrote:
> > > > I believe this is confusing. I thought that you want to store everything underneath `c++`? Additionally you use the same path for includes and modules.
> > > Typo? Should probably be "Includes are stored in `<prefix>/include/c++/v1`, modules in `<prefix>/modules/c++/v1`".
> > >
> > > I think this makes sense. Modules can't be `#include`d anyways so there is a need for them to live in an `include/` directory. And for those using a different ABIs this leaves the possibility to have
> > > ```
> > > .../include/c++/v1
> > > .../include/c++/v2
> > > .../modules/c++/v1
> > > .../modules/c++/v2
> > > ```
> > > or similar to clearly differentiate the headers/module interfaces.
> > `$PREFIX/modules` is a new path that doesn't exist in the Filesystem Hierarchy Standard.
> >
> > The source files for the module units is indeed a arch-independent resource, therefore the correct directory would be something under `$PREFIX/share`
> >
> > If we ever intend to ship BMI files, they would belong in `$libdir`.
> >
> > Here's how it would look like if we go in that direction:
> >
> > ```
> > $PREFIX/
> > $libdir/
> > libc++.so
> > libc++.so.module-info
> > c++/
> > modules/
> > libc++/
> > std.gcm.deadbeef1234
> > share/
> > c++/
> > modules/
> > libc++/
> > std.cppm
> > ```
> >
> > the `libc++.so.module-info` file would have the metadata necessary for someone to understand how to produce their own BMI as well as potentially reuse the shipped BMI if it just so happen that they can.
> There is one important bit that worries about splitting off the source files, tho.
>
> The module metadata shipped alongside the library itself needs to reference those source files, and I am concerned that requiring the use of `../../../../share/` in order to address the source location can lead to fragility in the deployment.
>
> So, even though it's technically arch-independent, I would also consider the following option:
>
> ```
> $PREFIX/
> $libdir/
> libc++.so
> libc++.so.module-info
> c++/
> modules/
> libc++/
> std.gcm.deadbeef1234
> std.cppm
> ```
>
> Because at that point, the metadata could reference a relative path from the library location without the awkwardness that using `../` can cause when directories are symlinks.
>
This is indeed a typo.
================
Comment at: libcxx/docs/Modules.rst:42
+ not handle that case properly.
+ * There is no `P1689 style output <https://wg21.link/P1689>`_ yet.
+ * The experimental library (read <format>) does not work as expected
----------------
ben.boeckel wrote:
> Mordante wrote:
> > aaronmondal wrote:
> > > ChuanqiXu wrote:
> > > > Mordante wrote:
> > > > > ChuanqiXu wrote:
> > > > > > Mordante wrote:
> > > > > > > ChuanqiXu wrote:
> > > > > > > > P1689 format is supported in clang-scan-deps. What's the reason we can't make it now?
> > > > > > > We can make it, that's no problem. However that file needs the use the installation path and not the path in the build directory. So the output needs a search and replace, but I don't know yet what the proper replace value will be.
> > > > > > >
> > > > > > > I put this item here so we don't forget about including it.
> > > > > > I still don't understand. No matter where the source files is, the output of P1689 format should be generated during the building and I think we shouldn't port them.
> > > > > Using
> > > > > `clang-scan-deps-17 -format=p1689 -compilation-database compile_commands.json`
> > > > > The output looks like
> > > > > ```
> > > > > {
> > > > > "revision": 0,
> > > > > "rules": [
> > > > > {
> > > > > "primary-output": "CMakeFiles/std.dir/std-coroutine.cppm.o",
> > > > > "provides": [
> > > > > {
> > > > > "is-interface": true,
> > > > > "logical-name": "std:coroutine",
> > > > > "source-path": "<build-dir>/include/c++/modules/std-coroutine.cppm"
> > > > > }
> > > > > ]
> > > > > },
> > > > > ```
> > > > > Where `<build-dir>` is the place where I copied the `cppm` files to. This output seems quite useless, since it's tied to a specific machine. Do you have suggestions how to get less my-system-specific paths?
> > > > Yeah, the `<build-dir>` is tied to specific machine. But why is it a problem? From my mind, we will distribute the `*.cppm` files like the header files. Then the users' build system will generate the `P1689` output according to the `*.cppm` files in their machine. I think the key point here is that the `P1689` output is a by-product of the building process and we (users) don't need to care about it.
> > > >
> > > Hmm `P1689` states that these files assume
> > >
> > > > uniformity of the environment between creation and usage; only used within one build of a project
> > >
> > > AFAIU the `P1689` output is not supposed to be distributed to users. It's supposed to be generated and consumed during the same build.
> > Let's discuss this further in an upcoming meeting. I don't object against not shipping it.
> You cannot ship the P1689 *output*, but it needs to be generated by anything *consuming* it so that the collation step can know to add the right flags (`-fmodule-file=…`) to the consuming TU compilation.
I've removed the section.
================
Comment at: libcxx/modules/std.compat/cassert.cppm:11-17
+module;
+#include <cassert>
+
+export module std.compat:cassert;
+export {
+ // This module exports nothing.
+} // export
----------------
ChuanqiXu wrote:
> What's the meaning for the file? It looks useless.
It kind of is, but it's easier to have tooling that makes sure every header is a module. This way there need to be no exceptions. Some of the "empty" modules may get symbols in the future.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144994/new/
https://reviews.llvm.org/D144994
More information about the libcxx-commits
mailing list