From a0e1cc0aefbc879cb2861003e487576e9a6b3c93 Mon Sep 17 00:00:00 2001 From: Chuanqi Xu Date: Mon, 29 Jan 2024 11:42:08 +0800 Subject: [PATCH] [C++20] [Modules] Don't perform ODR checks in GMF Close https://github.com/llvm/llvm-project/issues/79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too. --- clang/docs/ReleaseNotes.rst | 5 ++ clang/include/clang/Serialization/ASTReader.h | 4 ++ clang/lib/Serialization/ASTReader.cpp | 3 ++ clang/lib/Serialization/ASTReaderDecl.cpp | 37 +++++++++---- clang/lib/Serialization/ASTWriter.cpp | 8 ++- clang/lib/Serialization/ASTWriterDecl.cpp | 13 +++-- clang/test/Modules/concept.cppm | 14 ++--- clang/test/Modules/no-eager-load.cppm | 53 ------------------- clang/test/Modules/polluted-operator.cppm | 8 ++- clang/test/Modules/pr76638.cppm | 6 +-- 10 files changed, 68 insertions(+), 83 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 0e4bda7d8f35..ce94b3e44b95 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -188,6 +188,11 @@ C++20 Feature Support This feature is still experimental. Accordingly, ``__cpp_nontype_template_args`` was not updated. However, its support can be tested with ``__has_extension(cxx_generalized_nttp)``. +- Clang won't perform ODR checks for decls in the global module fragment any + more to ease the implementation and improve the user's using experience. + This follows the MSVC's behavior. + (`#79240 `_). + C++23 Feature Support ^^^^^^^^^^^^^^^^^^^^^ - Implemented `P0847R7: Deducing this `_. Some related core issues were also diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index dd1451bbf2d2..ba06ab0cd450 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -2452,6 +2452,10 @@ private: uint32_t CurrentBitsIndex = ~0; }; +inline bool isFromExplicitGMF(const Decl *D) { + return D->getOwningModule() && D->getOwningModule()->isExplicitGlobalModule(); +} + } // namespace clang #endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index fecd94e875f6..e91c5fe08a04 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9743,6 +9743,9 @@ void ASTReader::finishPendingActions() { if (!FD->isLateTemplateParsed() && !NonConstDefn->isLateTemplateParsed() && + // We only perform ODR checks for decls not in the explicit + // global module fragment. + !isFromExplicitGMF(FD) && FD->getODRHash() != NonConstDefn->getODRHash()) { if (!isa(FD)) { PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn); diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index a149d8215303..7697f29b9054 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -804,8 +804,10 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) { ED->setScopedUsingClassTag(EnumDeclBits.getNextBit()); ED->setFixed(EnumDeclBits.getNextBit()); - ED->setHasODRHash(true); - ED->ODRHash = Record.readInt(); + if (!isFromExplicitGMF(ED)) { + ED->setHasODRHash(true); + ED->ODRHash = Record.readInt(); + } // If this is a definition subject to the ODR, and we already have a // definition, merge this one into it. @@ -827,7 +829,9 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) { Reader.MergedDeclContexts.insert(std::make_pair(ED, OldDef)); ED->demoteThisDefinitionToDeclaration(); Reader.mergeDefinitionVisibility(OldDef, ED); - if (OldDef->getODRHash() != ED->getODRHash()) + // We don't want to check the ODR hash value for declarations from global + // module fragment. + if (!isFromExplicitGMF(ED) && OldDef->getODRHash() != ED->getODRHash()) Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED); } else { OldDef = ED; @@ -866,6 +870,9 @@ ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) { void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) { VisitRecordDeclImpl(RD); + // We should only reach here if we're in C/Objective-C. There is no + // global module fragment. + assert(!isFromExplicitGMF(RD)); RD->setODRHash(Record.readInt()); // Maintain the invariant of a redeclaration chain containing only @@ -1094,8 +1101,10 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) { if (FD->isExplicitlyDefaulted()) FD->setDefaultLoc(readSourceLocation()); - FD->ODRHash = Record.readInt(); - FD->setHasODRHash(true); + if (!isFromExplicitGMF(FD)) { + FD->ODRHash = Record.readInt(); + FD->setHasODRHash(true); + } if (FD->isDefaulted()) { if (unsigned NumLookups = Record.readInt()) { @@ -1971,9 +1980,12 @@ void ASTDeclReader::ReadCXXDefinitionData( #include "clang/AST/CXXRecordDeclDefinitionBits.def" #undef FIELD - // Note: the caller has deserialized the IsLambda bit already. - Data.ODRHash = Record.readInt(); - Data.HasODRHash = true; + // We only perform ODR checks for decls not in GMF. + if (!isFromExplicitGMF(D)) { + // Note: the caller has deserialized the IsLambda bit already. + Data.ODRHash = Record.readInt(); + Data.HasODRHash = true; + } if (Record.readInt()) { Reader.DefinitionSource[D] = @@ -2134,6 +2146,10 @@ void ASTDeclReader::MergeDefinitionData( } } + // We don't want to check ODR for decls in the global module fragment. + if (isFromExplicitGMF(MergeDD.Definition)) + return; + if (D->getODRHash() != MergeDD.ODRHash) { DetectedOdrViolation = true; } @@ -3498,10 +3514,13 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) { // If this declaration is from a merged context, make a note that we need to // check that the canonical definition of that context contains the decl. // + // Note that we don't perform ODR checks for decls from the global module + // fragment. + // // FIXME: We should do something similar if we merge two definitions of the // same template specialization into the same CXXRecordDecl. auto MergedDCIt = Reader.MergedDeclContexts.find(D->getLexicalDeclContext()); - if (MergedDCIt != Reader.MergedDeclContexts.end() && + if (MergedDCIt != Reader.MergedDeclContexts.end() && !isFromExplicitGMF(D) && MergedDCIt->second == D->getDeclContext()) Reader.PendingOdrMergeChecks.push_back(D); diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 03bddfe0f504..b64a6abb90f8 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -6022,8 +6022,12 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) { Record->push_back(DefinitionBits); - // getODRHash will compute the ODRHash if it has not been previously computed. - Record->push_back(D->getODRHash()); + // We only perform ODR checks for decls not in GMF. + if (!isFromExplicitGMF(D)) { + // getODRHash will compute the ODRHash if it has not been previously + // computed. + Record->push_back(D->getODRHash()); + } bool ModulesDebugInfo = Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType(); diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index bb1f51786d28..86d40436a9ba 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -493,7 +493,9 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) { EnumDeclBits.addBit(D->isFixed()); Record.push_back(EnumDeclBits); - Record.push_back(D->getODRHash()); + // We only perform ODR checks for decls not in GMF. + if (!isFromExplicitGMF(D)) + Record.push_back(D->getODRHash()); if (MemberSpecializationInfo *MemberInfo = D->getMemberSpecializationInfo()) { Record.AddDeclRef(MemberInfo->getInstantiatedFrom()); @@ -510,7 +512,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) { !D->isTopLevelDeclInObjCContainer() && !CXXRecordDecl::classofKind(D->getKind()) && !D->getIntegerTypeSourceInfo() && !D->getMemberSpecializationInfo() && - !needsAnonymousDeclarationNumber(D) && + !needsAnonymousDeclarationNumber(D) && !isFromExplicitGMF(D) && D->getDeclName().getNameKind() == DeclarationName::Identifier) AbbrevToUse = Writer.getDeclEnumAbbrev(); @@ -701,7 +703,9 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) { if (D->isExplicitlyDefaulted()) Record.AddSourceLocation(D->getDefaultLoc()); - Record.push_back(D->getODRHash()); + // We only perform ODR checks for decls not in GMF. + if (!isFromExplicitGMF(D)) + Record.push_back(D->getODRHash()); if (D->isDefaulted()) { if (auto *FDI = D->getDefaultedFunctionInfo()) { @@ -1506,7 +1510,8 @@ void ASTDeclWriter::VisitCXXMethodDecl(CXXMethodDecl *D) { D->getFirstDecl() == D->getMostRecentDecl() && !D->isInvalidDecl() && !D->hasAttrs() && !D->isTopLevelDeclInObjCContainer() && D->getDeclName().getNameKind() == DeclarationName::Identifier && - !D->hasExtInfo() && !D->isExplicitlyDefaulted()) { + !isFromExplicitGMF(D) && !D->hasExtInfo() && + !D->isExplicitlyDefaulted()) { if (D->getTemplatedKind() == FunctionDecl::TK_NonTemplate || D->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate || D->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization || diff --git a/clang/test/Modules/concept.cppm b/clang/test/Modules/concept.cppm index 0e85a46411a5..a343c9cd76a1 100644 --- a/clang/test/Modules/concept.cppm +++ b/clang/test/Modules/concept.cppm @@ -70,13 +70,6 @@ module; export module B; import A; -#ifdef DIFFERENT -// expected-error@foo.h:41 {{'__fn::operator()' from module 'A.' is not present in definition of '__fn' provided earlier}} -// expected-note@* 1+{{declaration of 'operator()' does not match}} -#else -// expected-no-diagnostics -#endif - template struct U { auto operator+(U) { return 0; } @@ -94,3 +87,10 @@ void foo() { __fn{}(U(), U()); } + +#ifdef DIFFERENT +// expected-error@B.cppm:* {{call to object of type '__fn' is ambiguous}} +// expected-note@* 1+{{candidate function}} +#else +// expected-no-diagnostics +#endif diff --git a/clang/test/Modules/no-eager-load.cppm b/clang/test/Modules/no-eager-load.cppm index 6632cc60c8eb..8a2c7656bca2 100644 --- a/clang/test/Modules/no-eager-load.cppm +++ b/clang/test/Modules/no-eager-load.cppm @@ -9,19 +9,10 @@ // RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/d.cpp \ // RUN: -fprebuilt-module-path=%t // -// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/e.cppm -o %t/e.pcm -// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/f.cppm -o %t/f.pcm -// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/g.cpp \ -// RUN: -fprebuilt-module-path=%t -// // RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/h.cppm \ // RUN: -fprebuilt-module-path=%t -o %t/h.pcm -// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/i.cppm \ -// RUN: -fprebuilt-module-path=%t -o %t/i.pcm // RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/j.cpp \ // RUN: -fprebuilt-module-path=%t -// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/k.cpp \ -// RUN: -fprebuilt-module-path=%t //--- a.cppm export module a; @@ -53,58 +44,14 @@ void use() { // expected-note@* {{but in 'a' found a different body}} } -//--- foo.h -void foo() { - -} - -//--- bar.h -void bar(); -void foo() { - bar(); -} - -//--- e.cppm -module; -#include "foo.h" -export module e; -export using ::foo; - -//--- f.cppm -module; -#include "bar.h" -export module f; -export using ::foo; - -//--- g.cpp -import e; -import f; -void use() { - foo(); // expected-error@* {{'foo' has different definitions in different modules;}} - // expected-note@* {{but in 'e.' found a different body}} -} - //--- h.cppm export module h; export import a; export import b; -//--- i.cppm -export module i; -export import e; -export import f; - //--- j.cpp import h; void use() { foo(); // expected-error@* {{'foo' has different definitions in different modules;}} // expected-note@* {{but in 'a' found a different body}} } - -//--- k.cpp -import i; -void use() { - foo(); // expected-error@* {{'foo' has different definitions in different modules;}} - // expected-note@* {{but in 'e.' found a different body}} -} - diff --git a/clang/test/Modules/polluted-operator.cppm b/clang/test/Modules/polluted-operator.cppm index b24464aa6ad2..d4b0041b5d34 100644 --- a/clang/test/Modules/polluted-operator.cppm +++ b/clang/test/Modules/polluted-operator.cppm @@ -46,12 +46,10 @@ module; export module a; //--- b.cppm +// This is actually an ODR violation. But given https://github.com/llvm/llvm-project/issues/79240, +// we don't count it as an ODR violation any more. +// expected-no-diagnostics module; #include "bar.h" export module b; import a; - -// expected-error@* {{has different definitions in different modules; first difference is defined here found data member '_S_copy_ctor' with an initializer}} -// expected-note@* {{but in 'a.' found data member '_S_copy_ctor' with a different initializer}} -// expected-error@* {{from module 'a.' is not present in definition of 'variant<_Types...>' provided earlier}} -// expected-note@* {{declaration of 'swap' does not match}} diff --git a/clang/test/Modules/pr76638.cppm b/clang/test/Modules/pr76638.cppm index 8cc807961421..28d03384634a 100644 --- a/clang/test/Modules/pr76638.cppm +++ b/clang/test/Modules/pr76638.cppm @@ -57,6 +57,9 @@ export module mod3; export using std::align_val_t; //--- mod4.cppm +// This is actually an ODR violation. But given https://github.com/llvm/llvm-project/issues/79240, +// we don't count it as an ODR violation now. +// expected-no-diagnostics module; #include "signed_size_t.h" #include "csize_t" @@ -64,6 +67,3 @@ module; export module mod4; import mod3; export using std::align_val_t; - -// expected-error@align.h:* {{'std::align_val_t' has different definitions in different modules; defined here first difference is enum with specified type 'size_t' (aka 'int')}} -// expected-note@align.h:* {{but in 'mod3.' found enum with specified type 'size_t' (aka 'unsigned int')}}