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

Thu Jun 29 11:12:02 PDT 2023

davidstone added inline comments.

================
Comment at: libcxx/utils/use_modules_in_test.py:169-176
+DISABLE_DEPRECATION_WARNINGS_REGEXES = [
+    re.compile(
+        r"//\s*ADDITIONAL_COMPILE_FLAGS:.*-D_LIBCPP_DISABLE_DEPRECATION_WARNINGS"
+    ),
+    re.compile(r"//\s*ADDITIONAL_COMPILE_FLAGS:.*-D_LIBCPP_ENABLE_ASSERTIONS"),
+    re.compile(r"//\s*ADDITIONAL_COMPILE_FLAGS:.*-D_LIBCPP_HAS_NO_UNICODE"),
+    re.compile(r"//\s*ADDITIONAL_COMPILE_FLAGS:.*-fsized-deallocation"),
----------------
ldionne wrote:
> ChuanqiXu wrote:
> > ldionne wrote:
> > > The fact that we need to disable these tests is quite interesting. I think it surfaces a pretty big problem with modules (as a language feature). Today, we use macros and compiler flags to customize the behavior of the library on a per translation-unit basis. That is an extremely important property of some aspects of the library (*). If we want everything to transition to modules (and we do), we need a way to satisfy those use cases. I don't mind whether that's done by defining macros, via some compiler flags or some other way, but those use cases are valid and we need to figure out how we're going to support them. Otherwise, we'll be left with large parts of our user base that can't switch over to C++ modules because they don't support some required use cases.
> > > 
> > > (*) For example, we use `_LIBCPP_ENABLE_ASSERTIONS` to control whether assertions are enabled or not on a per-TU basis. This is extremely important: when converting code bases to assertions, users might want to prevent assertions in some performance critical parts of the code, while still enabling assertions everywhere else. This approach allows for an incremental adoption of the feature and that's an important aspect of it being deployable in the real world.
> > > 
> > > I would like us to discuss avenues we could use to satisfy these use cases. For example, could we ship multiple "versions" of the library, each configured differently, and then the compiler would use a different version based on the compiler flags it is passed? I think pretty much all vendors are going to have similar questions, and that's going to severely limit how useful C++ modules can be in the real world. @jwakely @CaseyCarter I assume that's also something that you folks have thought about?
> > > 
> > > CC @ChuanqiXu 
> > (I am not 100% sure that I understand your question)
> > 
> > A background (I guess you already know this. But I'd like to repeat it again to avoid misunderstandings): we're going to ship (*.cppm) files. And the compilation of the std modules would happen in the users' machine. Also the compilation of the std modules may occur multiple times due to different configuration of the build. For example, the Release build and the Debug build will compile std modules twice with different options. And ideally the process would be controlled by the build systems (cmake).
> > 
> > The consensus in SG15 now is that we need to ship source files (*.cppm files) and a metadata file to describe the requirement/information of the library/module. Note that SG15 doesn't have a consensus (or even a feeling as far as I know) how the metadata should be.
> > 
> > So a temporary conclusion here is that possibly there is not an off-the-shelf practice that we can take directly. We would have to explore the method during the practice.
> > 
> > Also, I strongly suggest to send this question to SG15 and there are multiple experts who studied the problem for a relative long time. Maybe it will be more helpful than the small group discussion here.
> > 
> > CC: @ruoso @ben.boeckel 
> > we're going to ship (*.cppm) files. And the compilation of the std modules would happen in the users' machine.
> 
> Yes, we're on the same page. I'm not sure why I mentioned shipping various versions of the library, I was thinking about the need to pre-build multiple versions of the library for testing purposes (which is not related). Let me try to clarify my concern, and you can let me know whether I'm getting agitated for no good reason :-). Let's take for example `_LIBCPP_ENABLE_ASSERTIONS`. Today, that's a macro you define as `-D_LIBCPP_ENABLE_ASSERTIONS=1` on the command-line or `#define _LIBCPP_ENABLE_ASSERTIONS 1` as the first thing in your file, and the result is that libc++ functions like `operator[]` will be compiled with some assertions inside. This can be controlled on a per-TU basis today using the `#include` model we all know.
> 
> How do we do that in modules world if you're using `import std` instead? You can still pass `-D_LIBCPP_ENABLE_ASSERTIONS=1` on the command-line, however that won't do anything because the compiler is using the BMI files that have already been compiled and `_LIBCPP_ENABLE_ASSERTIONS` was set to `0` when those were compiled. Right?
> 
> You can also still use `#define _LIBCPP_ENABLE_ASSERTIONS=1` as the first thing in your file, but that also won't do anything because `import std;` doesn't have any way (or desire) to somehow resolve to a different BMI depending on the current macro environment. Right?
> 
> Basically, my question is: "What's the WG21-blessed alternative for this use case"? Are we expected to lift this mechanism from `-D_LIBCPP_ENABLE_ASSERTIONS=1` to some other compiler flag like `-flibcxx-enable-assertions` (with a better name)? Then users would tell their build-system to build specific TUs using that flag, and that would result in different BMIs being compiled and used for those TUs? I think that would work, but it would create some challenges for sure.
> 
> If that's the expectation, then it means that we either need to build multiple configurations of the library BMIs during CMake invocation for the test suite to reuse, or `lit` needs to be able to somehow rebuild BMIs (or ask CMake to do it) based on the requirement of various tests in the test suite. For example, you'd tell `lit` that you're using `-fsized-deallocation` in your test, so it would go and rebuild the BMIs for libc++ with that set of compiler flags, and then set the appropriate substitution for that test to use the right BMIs for `-fsized-deallocation`. Possible, but fun to implement :-)
> 
The point that you would define `_LIBCPP_ENABLE_ASSERTIONS=1` is when you compile the module in which you want to enable assertions. In other words, you can control it on a per-TU basis for the code that has the assertions, but not for the code using the code that has assertions. In general, you get ODR violations if the user's code is using multiple versions of library code.

So the user would not define the macro in their source file, nor would they (in CMake terms) associate the macro with their own target, they would either define the macro globally in their build system or associate it (somehow) with the standard library build target.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144994