[C++20] [Modules] Offer -fmodules-embed-all-files option #107194
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChuanqiXu9
requested review from
zygoloid,
cor3ntin,
iains,
AaronBallman and
erichkeane
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesSee Close #72383 Full diff: https://github.com/llvm/llvm-project/pull/107194.diff 4 Files Affected:
diff --git a/clang/docs/StandardCPlusPlusModules.rst b/clang/docs/StandardCPlusPlusModules.rst
index 2b757a807a0e6e..02ca0db3f937b3 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -462,6 +462,33 @@ Currently, Clang accepts the above example, though it may produce surprising
results if the debugging code depends on consistent use of ``NDEBUG`` in other
translation units.
+Source Files Consistency
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+Clang may open the input files (*) of a BMI during the compilation. It implies that
+when we consume a BMI, all the input files need to be present with the same path
+and the same contents.
+
+To overcome the requirements and simplify cases like distributed builds and sandboxed
+builds, users can use ``-fmodules-embed-all-files`` flag to embed all input files
+into the BMI so that clang won't ask to open the corresponding file on disk.
+
+Input files (*): The source files that took part in the compilation of the BMI.
+For example,
+
+.. code-block:: c++
+
+ // M.cppm
+ module;
+ #include "foo.h"
+ export module M;
+
+ // foo.h
+ #pragma once
+ #include "bar.h"
+
+The ``M.cppm``, ``foo.h`` and ``bar.h`` are input files for the BMI of ``M.cppm``.
+
Object definition consistency
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -484,6 +511,13 @@ fragment is disabled by default. These checks can be enabled by specifying
and you encounter incorrect or missing diagnostics, please report them via the
`community issue tracker <https://github.com/llvm/llvm-project/issues/>`_.
+Privacy Issue
+-------------
+
+BMIs are not and should not be treated as an information hiding mechanism.
+They should always be assumed to contain all the information that was used to
+create them, in a recoverable form.
+
ABI Impacts
-----------
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 83cf753e824845..af0edcf7b61d28 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3162,6 +3162,12 @@ def modules_reduced_bmi : Flag<["-"], "fexperimental-modules-reduced-bmi">,
HelpText<"Generate the reduced BMI">,
MarshallingInfoFlag<FrontendOpts<"GenReducedBMI">>;
+def fmodules_embed_all_files : Joined<["-"], "fmodules-embed-all-files">,
+ Visibility<[ClangOption, CC1Option, CLOption]>,
+ HelpText<"Embed the contents of all files read by this compilation into "
+ "the produced module file.">,
+ MarshallingInfoFlag<FrontendOpts<"ModulesEmbedAllFiles">>;
+
def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, Group<i_Group>,
Visibility<[ClangOption, CC1Option]>, MetaVarName<"<seconds>">,
HelpText<"Specify the interval (in seconds) between attempts to prune the module cache">,
@@ -7656,10 +7662,6 @@ def fmodules_embed_file_EQ : Joined<["-"], "fmodules-embed-file=">,
HelpText<"Embed the contents of the specified file into the module file "
"being compiled.">,
MarshallingInfoStringVector<FrontendOpts<"ModulesEmbedFiles">>;
-def fmodules_embed_all_files : Joined<["-"], "fmodules-embed-all-files">,
- HelpText<"Embed the contents of all files read by this compilation into "
- "the produced module file.">,
- MarshallingInfoFlag<FrontendOpts<"ModulesEmbedAllFiles">>;
defm fimplicit_modules_use_lock : BoolOption<"f", "implicit-modules-use-lock",
FrontendOpts<"BuildingImplicitModuleUsesLock">, DefaultTrue,
NegFlag<SetFalse>,
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index baac1215215b91..1f204d97da667e 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4214,6 +4214,9 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
Args.ClaimAllArgs(options::OPT_fmodule_output_EQ);
}
+ if (Args.hasArg(options::OPT_fmodules_embed_all_files))
+ CmdArgs.push_back("-fmodules-embed-all-files");
+
return HaveModules;
}
diff --git a/clang/test/Driver/fmodules-embed-all-files.cpp b/clang/test/Driver/fmodules-embed-all-files.cpp
new file mode 100644
index 00000000000000..f8e232f792d540
--- /dev/null
+++ b/clang/test/Driver/fmodules-embed-all-files.cpp
@@ -0,0 +1,2 @@
+// RUN: %clang -std=c++20 %s -fmodules-embed-all-files -### 2>&1 | FileCheck %s
+// CHECK: -fmodules-embed-all-files
|
ChuanqiXu9
commented
Sep 4, 2024
| @@ -3162,6 +3162,12 @@ def modules_reduced_bmi : Flag<["-"], "fexperimental-modules-reduced-bmi">, | |||
| HelpText<"Generate the reduced BMI">, | |||
| MarshallingInfoFlag<FrontendOpts<"GenReducedBMI">>; | |||
|
|
|||
| def fmodules_embed_all_files : Joined<["-"], "fmodules-embed-all-files">, | |||
There was a problem hiding this comment.
I moved the position since the original position is enclosed by a group assigning the CC1Option.
| @@ -0,0 +1,2 @@ | |||
| // RUN: %clang -std=c++20 %s -fmodules-embed-all-files -### 2>&1 | FileCheck %s | |||
There was a problem hiding this comment.
I'd appreciate a second RUN line that passes no option and checks that the cc1 option is not passed.
Comment on lines
468
to
470
| Clang may open the input files (*) of a BMI during the compilation. It implies that | ||
| when we consume a BMI, all the input files need to be present with the same path | ||
| and the same contents. |
There was a problem hiding this comment.
Suggested change
| Clang may open the input files (*) of a BMI during the compilation. It implies that | |
| when we consume a BMI, all the input files need to be present with the same path | |
| and the same contents. | |
| Clang may open the input files (*) of a BMI during the compilation. This implies that | |
| when Clang consumes a BMI, all the input files need to be present in the original path | |
| and with the original contents. |
Comment on lines
472
to
474
| To overcome the requirements and simplify cases like distributed builds and sandboxed | ||
| builds, users can use ``-fmodules-embed-all-files`` flag to embed all input files | ||
| into the BMI so that clang won't ask to open the corresponding file on disk. |
There was a problem hiding this comment.
Suggested change
| To overcome the requirements and simplify cases like distributed builds and sandboxed | |
| builds, users can use ``-fmodules-embed-all-files`` flag to embed all input files | |
| into the BMI so that clang won't ask to open the corresponding file on disk. | |
| To overcome these requirements and simplify cases like distributed and sandboxed | |
| builds, users can use the ``-fmodules-embed-all-files`` flag to embed all input files | |
| into the BMI so that Clang does not need to open the corresponding file on disk. |
Comment on lines
476
to
477
| Input files (*): The source files that took part in the compilation of the BMI. | ||
| For example, |
There was a problem hiding this comment.
Suggested change
| Input files (*): The source files that took part in the compilation of the BMI. | |
| For example, | |
| :sup:`*`Input files: The source files which took part in the compilation of the BMI. | |
| For example: |
| Source Files Consistency | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Clang may open the input files (*) of a BMI during the compilation. It implies that |
There was a problem hiding this comment.
Suggested change
| Clang may open the input files (*) of a BMI during the compilation. It implies that | |
| Clang may open the input files\ :sup:`*` of a BMI during the compilation. It implies that |
erichkeane
reviewed
Sep 18, 2024
|
|
||
| BMIs are not and should not be treated as an information hiding mechanism. | ||
| They should always be assumed to contain all the information that was used to | ||
| create them, in a recoverable form. |
There was a problem hiding this comment.
I might expand on this a bit with something like, "While the -fmodules_embed_all_files flag explicitly emits the source code into the BMI file, the contents of the BMI file contain a sufficiently verbose(word choice?) representation to reproduce the original source file."
I'd like to make this warning as clear as possible: If you ship a BMI, you mind as well just ship the source code.
There was a problem hiding this comment.
Done by adding the suggested text to the above section about -fmodules-embed-all-files. I feel it is odd to mention -fmodules-embed-all-files in the current section since it is mostly about clarifying that hiding information is not a goal for BMIs. It is true with and without -fmodules-embed-all-files.
ChuanqiXu9
force-pushed
the
EmbedAllFiles
branch
from
8d0f918 to
d539f51
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See
https://discourse.llvm.org/t/rfc-modules-should-we-embed-sources-to-the-bmi/81029 for details.
Close #72383