[libcxx-commits] [PATCH] D144994: [Draft][libc++][modules] Adds std module.

Tue Mar 14 11:41:42 PDT 2023

Mordante 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)
----------------
ben.boeckel wrote:
> 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).
Yes I indeed copied this from the other scripts. Are there better ways in CMake to have multi-line options?

================
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")
----------------
ben.boeckel wrote:
> 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.
This is used for the `.cppm` source files so they are arch independent.

================
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:
> 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?

================
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
----------------
ben.boeckel wrote:
> 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.
> > 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.

Do you have a paper number?

> > 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 agree, this list is mainly added for documentation so we know what the status is. It was not intended to be used for discussions during the review.

================
Comment at: libcxx/stdmodules/CMakeLists.txt:43
+    -stdlib=libc++
+    -fprebuilt-module-path=${PREBUILT_MODULE_PATH}
+    -pthread
----------------
ben.boeckel wrote:
> 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)?
I agree this is narrow and not great. However this patch's main focus it to be able to get modules working in libc++ in the first place.
We definitely need to think about the distribution in a user-friendly way.

I'm not sure whether libc++ needs to consider the configuration of Clang + libstdc++. Do you have something specific in mind?
I expect there need to be things done on the Clang side.

================
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.
+
----------------
ben.boeckel wrote:
> 3.26 should have this.
Thanks for the information! I see it landed recently in CMake.

================
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")
----------------
ben.boeckel wrote:
> This UUID changes…regularly. There will probably need to be a table to do this "properly".
Is there a public available table for these UUIDs? The last time I needed the one for CMake 3.25 I looked in the CMake git history.

================
Comment at: libcxx/stdmodules/std-cstddef.cppm:13
+#include <cstddef>
+
+export module std:cstddef;
----------------
tahonermann wrote:
> ldionne wrote:
> > Would this make sense to avoid users having to define all the config site macros? `#include <__config_site>`
> Named modules don't export/leak macros when imported, so the addition of the `#include` would have no effect on importers of the module.
The goal is not to leak the macros, but to use the macros to conditionally export symbols. For example in the next file in the patch I use `#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS` to avoid exporting wide character functions when the user configured libc++ without wide character support.

When building the BMI we need to know the configuration of libc++. In a similar fashion we need to disable symbols based on the language version used. For now there are some examples for C++20 vs C++23. Not that we have decided that we want to back port this feature, but we will need this for C++23 vs C++26.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144994/new/

https://reviews.llvm.org/D144994