From 3cca522d21876da36145655bc14f334035b4265d Mon Sep 17 00:00:00 2001 From: Chuanqi Xu Date: Fri, 23 Aug 2024 17:42:47 +0800 Subject: [PATCH] [C++20] [Modules] Warn for duplicated decls in mutliple module units (#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. --- .../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; +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>, + 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 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> + 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(D)) + return VD->isThisDeclarationADefinition() || + VD->isThisDeclarationADemotedDefinition(); + + if (auto *TD = dyn_cast(D)) + return TD->isThisDeclarationADefinition() || + TD->isThisDeclarationADemotedDefinition(); + + if (auto *FD = dyn_cast(D)) + return FD->isThisDeclarationADefinition() || PendingBodies.count(FD); + + if (auto *RTD = dyn_cast(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(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 { static void attachPreviousDecl(ASTReader &Reader, Decl *D, Decl *Previous, Decl *Canon); + static void checkMultipleDefinitionInNamedModules(ASTReader &Reader, Decl *D, + Decl *Previous); + template static void attachLatestDeclImpl(Redeclarable *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