[C++20] [Modules] Embed all source files for C++20 Modules #102444
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesClose #72383 The implementation rationale is, I don't want to pass Full diff: https://github.com/llvm/llvm-project/pull/102444.diff 5 Files Affected:
diff --git a/clang/include/clang/CodeGen/CodeGenAction.h b/clang/include/clang/CodeGen/CodeGenAction.h
index 186dbb43f01ef..461450d875ec5 100644
--- a/clang/include/clang/CodeGen/CodeGenAction.h
+++ b/clang/include/clang/CodeGen/CodeGenAction.h
@@ -57,7 +57,7 @@ class CodeGenAction : public ASTFrontendAction {
bool loadLinkModules(CompilerInstance &CI);
protected:
- bool BeginSourceFileAction(CompilerInstance &CI) override;
+ bool BeginInvocation(CompilerInstance &CI) override;
/// Create a new code generation action. If the optional \p _VMContext
/// parameter is supplied, the action uses it without taking ownership,
diff --git a/clang/include/clang/Frontend/FrontendActions.h b/clang/include/clang/Frontend/FrontendActions.h
index a620ddfc40447..e82f15f89b643 100644
--- a/clang/include/clang/Frontend/FrontendActions.h
+++ b/clang/include/clang/Frontend/FrontendActions.h
@@ -152,11 +152,13 @@ class GenerateModuleFromModuleMapAction : public GenerateModuleAction {
CreateOutputFile(CompilerInstance &CI, StringRef InFile) override;
};
+bool BeginInvocationForModules(CompilerInstance &CI);
+
/// Generates full BMI (which contains full information to generate the object
/// files) for C++20 Named Modules.
class GenerateModuleInterfaceAction : public GenerateModuleAction {
protected:
- bool BeginSourceFileAction(CompilerInstance &CI) override;
+ bool BeginInvocation(CompilerInstance &CI) override;
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
StringRef InFile) override;
diff --git a/clang/include/clang/Serialization/ModuleFile.h b/clang/include/clang/Serialization/ModuleFile.h
index 3e920c0f68360..30e7f6b3e57bd 100644
--- a/clang/include/clang/Serialization/ModuleFile.h
+++ b/clang/include/clang/Serialization/ModuleFile.h
@@ -88,13 +88,13 @@ class InputFile {
InputFile(FileEntryRef File, bool isOverridden = false,
bool isOutOfDate = false) {
- assert(!(isOverridden && isOutOfDate) &&
- "an overridden cannot be out-of-date");
unsigned intVal = 0;
- if (isOverridden)
- intVal = Overridden;
- else if (isOutOfDate)
+ // Make isOutOfDate with higher priority than isOverridden.
+ // It is possible if the recorded hash value mismatches.
+ if (isOutOfDate)
intVal = OutOfDate;
+ else if (isOverridden)
+ intVal = Overridden;
Val.setPointerAndInt(&File.getMapEntry(), intVal);
}
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index e87226e60297c..8900faf07eeaf 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -969,9 +969,10 @@ CodeGenerator *CodeGenAction::getCodeGenerator() const {
return BEConsumer->getCodeGenerator();
}
-bool CodeGenAction::BeginSourceFileAction(CompilerInstance &CI) {
+bool CodeGenAction::BeginInvocation(CompilerInstance &CI) {
if (CI.getFrontendOpts().GenReducedBMI)
- CI.getLangOpts().setCompilingModule(LangOptions::CMK_ModuleInterface);
+ return BeginInvocationForModules(CI);
+
return true;
}
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index e70210d55fe28..7758746f9a483 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -262,11 +262,20 @@ GenerateModuleFromModuleMapAction::CreateOutputFile(CompilerInstance &CI,
/*ForceUseTemporary=*/true);
}
-bool GenerateModuleInterfaceAction::BeginSourceFileAction(
- CompilerInstance &CI) {
+bool clang::BeginInvocationForModules(CompilerInstance &CI) {
+ // Embed all module files for named modules.
+ // See https://github.com/llvm/llvm-project/issues/72383 for discussion.
+ CI.getFrontendOpts().ModulesEmbedAllFiles = true;
CI.getLangOpts().setCompilingModule(LangOptions::CMK_ModuleInterface);
+ return true;
+}
- return GenerateModuleAction::BeginSourceFileAction(CI);
+bool GenerateModuleInterfaceAction::BeginInvocation(
+ CompilerInstance &CI) {
+ if (!BeginInvocationForModules(CI))
+ return false;
+
+ return GenerateModuleAction::BeginInvocation(CI);
}
std::unique_ptr<ASTConsumer>
|
You can test this locally with the following command:git-clang-format --diff 3b64ede096ce0a0230c4d3f77782e6fa18f2943a c61641a8901e99841916288d54a4e8d3d3894c89 --extensions cpp,cppm,h -- clang/include/clang/CodeGen/CodeGenAction.h clang/include/clang/Frontend/FrontendActions.h clang/include/clang/Serialization/ModuleFile.h clang/lib/CodeGen/CodeGenAction.cpp clang/lib/Frontend/FrontendActions.cpp clang/test/Modules/no-local-decl-in-reduced-bmi.cppm clang/test/Modules/reduced-bmi-empty-module-purview-std.cppm clang/test/Modules/reduced-bmi-empty-module-purview.cppm clang/test/Modules/unreached-static-entities.cppmView the diff from clang-format here.diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index 8c7b749fe8..0ca122742b 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -270,8 +270,7 @@ bool clang::BeginInvocationForModules(CompilerInstance &CI) {
return true;
}
-bool GenerateModuleInterfaceAction::BeginInvocation(
- CompilerInstance &CI) {
+bool GenerateModuleInterfaceAction::BeginInvocation(CompilerInstance &CI) {
if (!BeginInvocationForModules(CI))
return false;
|
ChuanqiXu9
force-pushed
the
EmbedAllFiles
branch
from
4b58a18 to
c61641a
Compare
|
Was this discussed/reviewed/motivated? There are drawbacks to this approach outlined in #72383 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/4498 Here is the relevant piece of the build log for the reference |
The motivation is in #72383 and I comment in #72383 (comment) This is not reviewed. I wait for several weeks but got no response. And I think it is good. So I choose to land it. |
Thank you for the link! (FWIW, there's no problems with these changes having been landed; @ChuanqiXu9 is the code owner for modules and the PR was up for three weeks without discussion. This is just typical post-commit review feedback.) There's a fair amount of discussion on that thread in opposition to this approach, and a comment thread on an issue is not really visible to many people. I think this warrants an RFC for a broader discussion, so I'd appreciate temporarily reverting this patch. Some thoughts for the RFC discussion:
|
|
FWIW, I meant to review this, but somehow this disappeared from my waiting for review list and I lost track of it :) |
Thank you, Aaron! But I like to not revert this until someone give some complaints on that thread. Since the problem in the high level looks clear to me and feel like the discussion in that thread looks like people there have a pretty good understanding to the issue self.
We can discuss this topic in the two aspects, a module specific one and a broader one not limited to modules (the general policy). For the module specific topic, since the BMI generated by clang and MSVC are not compatible, this change won't have any particular impact to the users of MSVC or
The internal reason is that, in clang, the source locations are super fundamental and sensitive. So that, if we don't embed the source files, the BMI became invalid after we change the corresponding source file. And for the broader one, should users expect
There shouldn't be any bad impact on other tools. Since all the tools have to consume the BMI via the compiler (in the same version!). Since this patch simply loose the restriction, I don't expect this have any impact on 3rd party tools.
The consensus of SG15 for BMI is, the BMI is not good for distributed. Since the BMI format are not compatible across compilers (even with different versions!). So we would think BMI as a by-product of a build process only. Although there are thoughts about, we can distribute the BMI in a homogeneous environment where we have stable compiler versions. But such environments are closed ended environment for some specific projects or organizations. So I don't think it may affect any thing for the security as the design doesn't want us to ship the BMIs.
Understood, I have the same experience too. Not meant to blame any one : ) |
Again, I don't think a comment thread on an issue has a wide enough audience. FWIW, I had no idea about that thread until I got pinged here.
I don't see why that's an issue; if the source code changes, not changing the BMI is a slight quality-of-implementation improvement (e.g., when user changes comments, but the actual semantics remain the same), but that hardly seems worth the concerns I have below.
Yeah, that broader discussion is one we should have; and we should have the same conversation about GCC compatibility as we're 1) not compatible, 2) drifting farther apart, 3) still claim we're GCC 4.2, and 4) have never explicitly determined just how much compatibility we need/want as a community (it was an organic thing that grew out of the needs of the early developers of Clang)
Good to know; but this is a small part of why I'd like an RFC -- tools vendors should be made aware so they can raise concerns if they have them.
That's SG15. This change has serious implications for IP and I don't feel comfortable making that decision as quietly as you have. This is the primary reason why I'm insisting on an RFC. As a concrete example, large organizations often have contractors who utilize libraries written by the parent company. If the company produces a BMI for their framework (because they control the environment used by the contractor anyway, so this is safe and reasonable to do), they may accidentally ship their sensitive IP along with it. So I continue to ask for this to be reverted and an RFC posted. |
|
Sorry I have not made much review here - although did comment on #72383 ..
From an engineering perspective it seems that we (clang) needs to find a better solution? I would agree with @AaronBallman that this needs a higher level design discussion (I have no strong personal opinion on whether the merge is reverted - but then a decision should be taken about how this is to be implemented before any release is made - and given that this might be quite heavy lifting - it might be wise to revert it in case it cannot be resolved in time - to avoid churn in user's expectations). |
|
FWIW: As [clang, GCC, MSVC] are currently implementing modules, there is no question of compatibility at the exchange of BMIs level (so that is currently a non-issue). It does not seem that this is something in the "compatible with gcc-4.2.1" layer.... However, it is a tooling issue if one toolchain does not require sources distributed along with the BMIs but others do - so that is a potential reason to want to coordinate. |
|
Thanks for the write-up. I've reverted and I'll try to open a RFC to get a wider discussion. |
|
@AaronBallman @iains I've sent the RFC here: https://discourse.llvm.org/t/rfc-modules-should-we-embed-sources-to-the-bmi/81029 |
Close #72383
The implementation rationale is, I don't want to pass
-fmodules-embed-all-filesall the time since we can't test it in lit tests (we're usingclang_cc1). So I tried to set it in FrontendActions for modules.