[clang] [C++20] [Modules] Warn for duplicated decls in mutliple module units (PR #105799)
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 23 01:42:38 PDT 2024
https://github.com/ChuanqiXu9 created https://github.com/llvm/llvm-project/pull/105799
It is a long standing issue that the duplicated declarations in multiple module units would cause the compilation performance to get slowed down. And there are many questions or issue reports. So I think it is better to add a warning for it.
And given this is not because the users' code violates the language specification or any best practices, the warning is disabled by default even if `-Wall` is specified. The users need to specify the warning explcitly or use `Weverything`.
The documentation will add separately.
>From 3b5ee579f1201d6891147026f0d71d91303e59bd Mon Sep 17 00:00:00 2001
From: Chuanqi Xu <yedeng.yd at linux.alibaba.com>
Date: Fri, 23 Aug 2024 16:34:34 +0800
Subject: [PATCH] [C++20] [Modules] Warn for duplicated decls in mutliple
module units
It is a long standing issue that the duplicated declarations in multiple
module units would cause the compilation performance to get slowed down.
And there are many questions or issue reports. So I think it is better
to add a warning for it.
And given this is not because the users' code violates the language
specification or any best practices, the warning is disabled by default
even if `-Wall` is specified. The users need to specify the warning
explcitly or use `Weverything`.
---
.../Basic/DiagnosticSerializationKinds.td | 6 ++
clang/include/clang/Serialization/ASTReader.h | 6 ++
clang/lib/Serialization/ASTReader.cpp | 39 +++++++++
clang/lib/Serialization/ASTReaderDecl.cpp | 21 ++++-
...warn-duplicated-decls-in-module-units.cppm | 83 +++++++++++++++++++
5 files changed, 152 insertions(+), 3 deletions(-)
create mode 100644 clang/test/Modules/warn-duplicated-decls-in-module-units.cppm
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 9854972cbfe7e4..253a955431997b 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -134,6 +134,12 @@ def warn_module_system_bit_conflict : Warning<
"as a non-system module; any difference in diagnostic options will be ignored">,
InGroup<ModuleConflict>;
+def warn_decls_in_multiple_modules : Warning<
+ "declaration %0 is detected to be defined in multiple module units, first is from '%1' and second is from '%2'; "
+ "the compiler may not be good at merging the definitions. ">,
+ InGroup<DiagGroup<"decls-in-multiple-modules">>,
+ DefaultIgnore;
+
def err_failed_to_find_module_file : Error<
"failed to find module file for module '%0'">;
} // let CategoryName
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 4593213c5f43ce..2d8952ddbd71df 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -648,6 +648,12 @@ class ASTReader
/// performed deduplication.
llvm::SetVector<NamedDecl *> PendingMergedDefinitionsToDeduplicate;
+ /// The duplicated definitions in module units which are pending to be warned.
+ /// We need to delay it to wait for the loading of definitions since we don't
+ /// want to warn for forward declarations.
+ llvm::SmallVector<std::pair<Decl *, Decl *>>
+ PendingWarningForDuplicatedDefsInModuleUnits;
+
/// Read the record that describes the lexical contents of a DC.
bool ReadLexicalDeclContextStorage(ModuleFile &M,
llvm::BitstreamCursor &Cursor,
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index fa9b815239dbb6..be83805f1e92b9 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9955,6 +9955,45 @@ void ASTReader::finishPendingActions() {
}
PendingDefinitions.clear();
+ for (auto [D, Previous] : PendingWarningForDuplicatedDefsInModuleUnits) {
+ auto hasDefinitionImpl = [this](Decl *D, auto hasDefinitionImpl) {
+ if (auto *VD = dyn_cast<VarDecl>(D))
+ return VD->isThisDeclarationADefinition() ||
+ VD->isThisDeclarationADemotedDefinition();
+
+ if (auto *TD = dyn_cast<TagDecl>(D))
+ return TD->isThisDeclarationADefinition() ||
+ TD->isThisDeclarationADemotedDefinition();
+
+ if (auto *FD = dyn_cast<FunctionDecl>(D))
+ return FD->isThisDeclarationADefinition() || PendingBodies.count(FD);
+
+ if (auto *RTD = dyn_cast<RedeclarableTemplateDecl>(D))
+ return hasDefinitionImpl(RTD->getTemplatedDecl(), hasDefinitionImpl);
+
+ // Conservatively return false here.
+ return false;
+ };
+
+ auto hasDefinition = [this, &hasDefinitionImpl](Decl *D) {
+ return hasDefinitionImpl(D, hasDefinitionImpl);
+ };
+
+ // It is not good to prevent multiple declarations since the forward
+ // declaration is common. Let's try to avoid duplicated definitions
+ // only.
+ if (!hasDefinition(D) || !hasDefinition(Previous))
+ continue;
+
+ Module *PM = Previous->getOwningModule();
+ Module *DM = D->getOwningModule();
+ Diag(D->getLocation(), diag::warn_decls_in_multiple_modules)
+ << cast<NamedDecl>(Previous) << PM->getTopLevelModuleName()
+ << (DM ? DM->getTopLevelModuleName() : "global module");
+ Diag(Previous->getLocation(), diag::note_also_found);
+ }
+ PendingWarningForDuplicatedDefsInModuleUnits.clear();
+
// Load the bodies of any functions or methods we've encountered. We do
// this now (delayed) so that we can be sure that the declaration chains
// have been fully wired up (hasBody relies on this).
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 4d9d024796716e..d1b77358d0cde4 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -320,6 +320,9 @@ class ASTDeclReader : public DeclVisitor<ASTDeclReader, void> {
static void attachPreviousDecl(ASTReader &Reader, Decl *D, Decl *Previous,
Decl *Canon);
+ static void checkMultipleDefinitionInNamedModules(ASTReader &Reader, Decl *D,
+ Decl *Previous);
+
template <typename DeclT>
static void attachLatestDeclImpl(Redeclarable<DeclT> *D, Decl *Latest);
static void attachLatestDeclImpl(...);
@@ -3690,8 +3693,9 @@ static void inheritDefaultTemplateArguments(ASTContext &Context,
// [basic.link]/p10:
// If two declarations of an entity are attached to different modules,
// the program is ill-formed;
-static void checkMultipleDefinitionInNamedModules(ASTReader &Reader, Decl *D,
- Decl *Previous) {
+void ASTDeclReader::checkMultipleDefinitionInNamedModules(ASTReader &Reader,
+ Decl *D,
+ Decl *Previous) {
// If it is previous implcitly introduced, it is not meaningful to
// diagnose it.
if (Previous->isImplicit())
@@ -3721,8 +3725,19 @@ static void checkMultipleDefinitionInNamedModules(ASTReader &Reader, Decl *D,
return;
// We only forbids merging decls within named modules.
- if (!M->isNamedModule())
+ if (!M->isNamedModule()) {
+ // Try to warn the case that we merged decls from global module.
+ if (!M->isGlobalModule())
+ return;
+
+ if (D->getOwningModule() &&
+ M->getTopLevelModule() == D->getOwningModule()->getTopLevelModule())
+ return;
+
+ Reader.PendingWarningForDuplicatedDefsInModuleUnits.push_back(
+ {D, Previous});
return;
+ }
// It is fine if they are in the same module.
if (Reader.getContext().isInSameModule(M, D->getOwningModule()))
diff --git a/clang/test/Modules/warn-duplicated-decls-in-module-units.cppm b/clang/test/Modules/warn-duplicated-decls-in-module-units.cppm
new file mode 100644
index 00000000000000..f9156497bc6b17
--- /dev/null
+++ b/clang/test/Modules/warn-duplicated-decls-in-module-units.cppm
@@ -0,0 +1,83 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/m1.cppm -emit-module-interface -o %t/m1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/m2.cppm -emit-module-interface -o %t/m2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/use.cc -fsyntax-only \
+// RUN: -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/m1.cppm -Wall -emit-module-interface -o %t/m1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/m2.cppm -Wall -emit-module-interface -o %t/m2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/use.cc -fsyntax-only \
+// RUN: -verify -Wall
+//
+// RUN: %clang_cc1 -std=c++20 %t/m1.cppm -Wdecls-in-multiple-modules -emit-module-interface -o %t/m1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/m2.cppm -Wdecls-in-multiple-modules -emit-module-interface -o %t/m2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/use.cc -fsyntax-only \
+// RUN: -verify -Wdecls-in-multiple-modules -DWARNING
+
+//--- foo.h
+#ifndef FOO_H
+#define FOO_H
+
+enum E { E1, E2 };
+
+int a = 43;
+
+class foo {
+public:
+ void consume(E, int);
+};
+
+inline void func() {}
+
+void fwd_decl();
+
+#endif
+
+//--- m1.cppm
+module;
+#include "foo.h"
+export module m1;
+export {
+ using ::foo;
+ using ::a;
+ using ::func;
+ using ::fwd_decl;
+ using ::E;
+}
+
+//--- m2.cppm
+module;
+#include "foo.h"
+export module m2;
+export {
+ using ::foo;
+ using ::a;
+ using ::func;
+ using ::fwd_decl;
+ using ::E;
+}
+
+//--- use.cc
+import m1;
+import m2;
+void use();
+void use() {
+ E e = E1;
+ foo f;
+ f.consume(e, a);
+ func();
+ fwd_decl();
+}
+
+#ifndef WARNING
+// expected-no-diagnostics
+#else
+// expected-warning@* {{declaration 'E' is detected to be defined in multiple module units}}
+// expected-warning@* {{declaration 'foo' is detected to be defined in multiple module units}}
+// expected-warning@* {{declaration 'a' is detected to be defined in multiple module units}}
+// expected-warning@* {{declaration 'func' is detected to be defined in multiple module units}}
+// expected-note@* 1+ {{}}
+#endif
More information about the cfe-commits
mailing list