[libcxx-commits] [PATCH] D144994: [Draft][libc++][modules] Adds std module.
Ben Boeckel via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Mar 13 09:24:12 PDT 2023
ben.boeckel added inline comments.
================
Comment at: libcxx/CMakeLists.txt:108-111
+option(LIBCXX_ENABLE_STD_MODULE
+ "Whether to enable the building the C++23 std module. This feature is experimental
+ and far from usable. Only enable this when interested in testing and developing
+ this module." OFF)
----------------
This help string has embedded newlines and indentation (though I see other options do this…). I don't think the UI editors show the help well in this case (testing with `ccmake` seems to bear this out).
================
Comment at: libcxx/CMakeLists.txt:422
set(LIBCXX_GENERATED_INCLUDE_DIR "${LLVM_BINARY_DIR}/include/c++/v1")
+ set(LIBCXX_GENERATED_MODULE_DIR "${LLVM_BINARY_DIR}/include/c++/modules")
set(LIBCXX_GENERATED_INCLUDE_TARGET_DIR "${LLVM_BINARY_DIR}/include/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1")
----------------
Mordante wrote:
> aaronmondal wrote:
> > This seems like a good opportunity to finally give `v1` a friend 😊
> >
> > Is there some resource on why/how the `v1` path came to be in the first place?
> >
> > I'd expect this to {break/be incompatible with} quite a few {distros/setups/build sytems} as the path to `v1` is probably one of the most often hardcoded paths in history 😅.
> >
> > Whatever the final name of this `modules` directory is, it would probably be a good idea to have some kind of "reasoning" behind it so that adopters (and other c++ standard libraries?) don't feel like this name was chosen arbitrarily.
> ABI v1.
>
> I actually think the current location is horrible ;-) As documented and commented below the final location needs a bit more thought. However for testing it now it's "good enough". So I fully share your concerns, but for testing I need *a* location. For the real product we need to consider *the proper* location.
>
>
Somewhere under `libdir` would be good unless we know these files are actually arch-independent.
================
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.
+
----------------
Note that buildsystems should handle this. I don't know how viable the "just run the compiler by hand" is in the long-term…
================
Comment at: libcxx/docs/Modules.rst:51-54
+ * There is no `P1689 style output <https://wg21.link/P1689>`_ yet.
+ The file generated by clang-scan-deps contains absolute paths to the files
+ in the libc++ build directory. (These should be relative paths or paths to
+ the installation directory.)
----------------
Nor should there be IMO. Note that a single build tree may need *multiple* compilations of the module, so I don't think a single "scanning output" is suitable to ship.
================
Comment at: libcxx/docs/Modules.rst:71
+ `Patch <https://llvm.org/D145596>`__
+ * The ``-pthread`` flag is not used a compile flag. This flag affects
+ the preprocessor so it should be a compile and linker flag.
----------------
"not used as a compile flag" perhaps?
================
Comment at: libcxx/docs/Modules.rst:116
+ that a different solution needs to be used.
+ * Should P1689 style output files be shipped with libc++?
+
----------------
IMO, no.
================
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.
----------------
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.
================
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
----------------
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.
================
Comment at: libcxx/docs/Modules.rst:65
+ to discuss that approach before spending effort in it.
+ * Currently the ``cppm`` file and the generated ``CMakeLists.txt`` are not
+ installed. Before doing that it would be good to determine what the best
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > h-vetinari wrote:
> > > Mordante wrote:
> > > > aaronmondal wrote:
> > > > > We intend to distribute all `.cppm` files, i.e. the partitions as well, right?
> > > > Yes I want to distribute them, in fact we must distribute them else users can't use modules.
> > > >
> > > > But we need to find the "proper" location to store them. Since this is something new this probably needs some involvement of our packagers. I don't expect the Linux Standard Base has an entry for C++20 modules.
> > > > But we need to find the "proper" location to store them. Since this is something new this probably needs some involvement of our packagers. I don't expect the Linux Standard Base has an entry for C++20 modules.
> > >
> > > As mentioned already, I think there are strong parallels with distributing headers. The bare minimum should be that they are automatically discoverable by the toolchain that produced them (should be somewhere under INSTALL_PREFIX obviously), but ideally it's in a standard location that's uniform across toolchains (again, relative to a PREFIX), like the headers and libs are too. I'm not sure if SG21 has had discussions about this, or if CMake has already thought about this.
> > >
> > > Personally, I think the best might be a separate folder like `$PREFIX/something_module_related/foo.cppm`, but I wouldn't care much or at all if it's under `$PREFIX/include/something_module_related/foo.cppm` or `$PREFIX/lib/<libname>/foo.cppm`, etc., as long as it's discoverable and works.
> > >
> > > CC @mgorny as another packager & @ben.boeckel for the CMake side.
> > > I'm not sure if SG21 has had discussions about this, or if CMake has already thought about this.
> >
> > As far as I know, sg15 has discussed this for a long time. (If I recall this correctly,) sg15 don't get the final consensus yet. But @ruoso has already made a lot of proposals. We can look at them later.
> >
> > > Personally, I think the best might be a separate folder like $PREFIX/something_module_related/foo.cppm, but I wouldn't care much or at all if it's under $PREFIX/include/something_module_related/foo.cppm or $PREFIX/lib/<libname>/foo.cppm, etc., as long as it's discoverable and works.
> >
> > I think we need to discuss this in the discourse. Because the patch is going to be a large patch and the topic about distribution is another big topic. It looks not good to discuss two big things in the same page. The direction may be lost.
> I created https://discourse.llvm.org/t/rfc-about-the-default-location-for-std-modules/69191 and let's discuss this there.
> Personally, I think the best might be a separate folder like $PREFIX/something_module_related/foo.cppm, but I wouldn't care much or at all if it's under $PREFIX/include/something_module_related/foo.cppm or $PREFIX/lib/<libname>/foo.cppm, etc., as long as it's discoverable and works.
Note that libstdc++ is also going to ship `std.cppm` (possibly with a different extension), so the `libname` being part of the path sounds best to me. I'll jump in over on Discourse.
================
Comment at: libcxx/stdmodules/CMakeLists.txt:43
+ -stdlib=libc++
+ -fprebuilt-module-path=${PREBUILT_MODULE_PATH}
+ -pthread
----------------
Mordante wrote:
> aaronmondal wrote:
> > aaronmondal wrote:
> > > I remember running into issues with `-fprebuilt-module-path` as it assumes a very specific output layout during the build. For build systems that sandbox compilations the (absolute) path to the output file (and to the headers it included?) will be embedded in the PCM and aggregating all BMIs in one directory after precompilation might lead to "dependency not found"-type errors.
> > >
> > > Explicitly passing the files via `-fmodule-file=<path/to/BMI>` or `-fmodule-file=<module_name>=<path/to/BMI>` seems more robust to me as this takes the situation into account where BMI outputs are potentially temporarily scattered across directories.
> > >
> > > I think for CMake the current `-fprebuilt-module-path` should work fine. I'm not sure about gn, and I know for sure that something like this won't work in Bazel.
> > Amend: `-fmodule-file=<path/to/BMI>` will probably be deprecated (https://github.com/llvm/llvm-project/issues/60824), so something like `-fmodule-file=std:header=<path/to/header>` seems like the way to go.
> I agree. This was mainly to show the difference between manually crafting pcm files vs letting cmake doing it. This was the work I did last weekend.
>
> After yesterday's meeting I think this entire approach in CMake is flawed, this approach assumes we're going to distribute the `.pcm` files. Instead we need to focus on distributing `.cppm` files and offer the tools for the user to create the `.pcm` files for their project. This is something I will investigate further.
> Instead we need to focus on distributing .cppm files and offer the tools for the user to create the .pcm files for their project. This is something I will investigate further.
I think this is more the job of CMake since asking users to do
```cmake
if (<is_clang>)
setup_clang_libcpp()
elseif (<is_gcc>)
setup_gcc_libstdcpp()
elseif (<is_msvc>)
setup_msvc_stl()
endif ()
```
is awfully narrow IMO. Highly related question: what do you expect to happen in the case of clang with libstdc++ (as many distros end up doing)?
================
Comment at: libcxx/stdmodules/CMakeLists.txt.in:7
+# CMake specific magic
+# In the future this probably is not needed and will be part of CMake itself.
+
----------------
3.26 should have this.
================
Comment at: libcxx/stdmodules/CMakeLists.txt.in:9
+
+set(CMAKE_EXPERIMENTAL_CXX_MODULE_CMAKE_API "2182bf5c-ef0d-489a-91da-49dbc3090d2a")
+set(CMake_TEST_CXXModules_UUID "a246741c-d067-4019-a8fb-3d16b0c9d1d3")
----------------
This UUID changes…regularly. There will probably need to be a table to do this "properly".
================
Comment at: libcxx/stdmodules/CMakeLists.txt.in:10
+set(CMAKE_EXPERIMENTAL_CXX_MODULE_CMAKE_API "2182bf5c-ef0d-489a-91da-49dbc3090d2a")
+set(CMake_TEST_CXXModules_UUID "a246741c-d067-4019-a8fb-3d16b0c9d1d3")
+
----------------
This is specific to CMake's test suite and shouldn't be relevant.
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