Author: Chuanqi Xu Date: 2024-01-29T11:44:59+08:00 New Revision: a0b6747804e46665ecfd00295b60432bfe1775b6
URL: https://github.com/llvm/llvm-project/commit/a0b6747804e46665ecfd00295b60432bfe1775b6 DIFF: https://github.com/llvm/llvm-project/commit/a0b6747804e46665ecfd00295b60432bfe1775b6.diff LOG: [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. Added: Modified: clang/docs/ReleaseNotes.rst clang/include/clang/Serialization/ASTReader.h clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/ASTWriterDecl.cpp clang/test/Modules/concept.cppm clang/test/Modules/no-eager-load.cppm clang/test/Modules/polluted-operator.cppm clang/test/Modules/pr76638.cppm Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 2aa8fea4ad67c0..9d68be469dac39 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -62,6 +62,11 @@ C++ Language Changes C++20 Feature Support ^^^^^^^^^^^^^^^^^^^^^ +- 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 <https://github.com/llvm/llvm-project/issues/79240>`_). + C++23 Feature Support ^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index dd1451bbf2d2c9..ba06ab0cd45097 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -2452,6 +2452,10 @@ class BitsUnpacker { 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 89b044a61a7b15..2abe5e44e2e988 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9747,6 +9747,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<CXXMethodDecl>(FD)) { PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn); diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 867f4c47eaeceb..29ece2a132bf8c 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 eb2f8e23710997..dcb18ad3389321 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -6027,8 +6027,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 bb1f51786d2813..86d40436a9bac1 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 0e85a46411a544..a343c9cd76a119 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.<global>' is not present in definition of '__fn' provided earlier}} -// expected-note@* 1+{{declaration of 'operator()' does not match}} -#else -// expected-no-diagnostics -#endif - template <class T> struct U { auto operator+(U) { return 0; } @@ -94,3 +87,10 @@ void foo() { __fn{}(U<int>(), U<int>()); } + +#ifdef DIFFERENT +// expected-er...@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 6632cc60c8eb84..8a2c7656bca2b4 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 diff erent 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 diff erent definitions in diff erent modules;}} - // expected-note@* {{but in 'e.<global>' found a diff erent 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 diff erent definitions in diff erent modules;}} // expected-note@* {{but in 'a' found a diff erent body}} } - -//--- k.cpp -import i; -void use() { - foo(); // expected-error@* {{'foo' has diff erent definitions in diff erent modules;}} - // expected-note@* {{but in 'e.<global>' found a diff erent body}} -} - diff --git a/clang/test/Modules/polluted-operator.cppm b/clang/test/Modules/polluted-operator.cppm index b24464aa6ad21e..d4b0041b5d346e 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 diff erent definitions in diff erent modules; first diff erence is defined here found data member '_S_copy_ctor' with an initializer}} -// expected-note@* {{but in 'a.<global>' found data member '_S_copy_ctor' with a diff erent initializer}} -// expected-error@* {{from module 'a.<global>' 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 8cc807961421b7..28d03384634ab2 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 diff erent definitions in diff erent modules; defined here first diff erence is enum with specified type 'size_t' (aka 'int')}} -// expected-note@align.h:* {{but in 'mod3.<global>' found enum with specified type 'size_t' (aka 'unsigned int')}} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
