Author: Chuanqi Xu Date: 2024-03-11T11:39:21+08:00 New Revision: 3f6bc1adf805681293c2ef0b93b708ff52244c00
URL: https://github.com/llvm/llvm-project/commit/3f6bc1adf805681293c2ef0b93b708ff52244c00 DIFF: https://github.com/llvm/llvm-project/commit/3f6bc1adf805681293c2ef0b93b708ff52244c00.diff LOG: [C++20] [Moduls] Avoid computing odr hash for functions from comparing constraint expression Previously we disabled to compute ODR hash for declarations from the global module fragment. However, we missed the case that the functions lives in the concept requiments (see the attached the test files for example). And the mismatch causes the potential crashment. Due to we will set the function body as lazy after we deserialize it and we will only take its body when needed. However, we don't allow to take the body during deserializing. So it is actually potentially problematic if we set the body as lazy first and computing the hash value of the function, which requires to deserialize its body. So we will meet a crash here. This patch tries to solve the issue by not taking the body of the function from GMF. Note that we can't skip comparing the constraint expression from the GMF directly since it is an key part of the function selecting and it may be the reason why we can't return 0 directly for `FunctionDecl::getODRHash()` from the GMF. Added: clang/test/Modules/hashing-decls-in-exprs-from-gmf.cppm Modified: clang/include/clang/AST/DeclBase.h clang/include/clang/Serialization/ASTReader.h clang/lib/AST/Decl.cpp clang/lib/AST/DeclBase.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/ASTWriterDecl.cpp Removed: ################################################################################ diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index 76810a86a78a46..47ed6d0d1db0df 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -673,6 +673,16 @@ class alignas(8) Decl { /// fragment. See [module.global.frag]p3,4 for details. bool isDiscardedInGlobalModuleFragment() const { return false; } + /// Check if we should skip checking ODRHash for declaration \param D. + /// + /// The existing ODRHash mechanism seems to be not stable enough and + /// the false positive ODR violation reports are annoying and we rarely see + /// true ODR violation reports. Also we learned that MSVC disabled ODR checks + /// for declarations in GMF. So we try to disable ODR checks in the GMF to + /// get better user experiences before we make the ODR violation checks stable + /// enough. + bool shouldSkipCheckingODR() const; + /// Return true if this declaration has an attribute which acts as /// definition of the entity, such as 'alias' or 'ifunc'. bool hasDefiningAttr() const; diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 2002bf23c9595f..370d8037a4da17 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -2456,13 +2456,6 @@ class BitsUnpacker { uint32_t Value; uint32_t CurrentBitsIndex = ~0; }; - -inline bool shouldSkipCheckingODR(const Decl *D) { - return D->getOwningModule() && - D->getASTContext().getLangOpts().SkipODRCheckInGMF && - D->getOwningModule()->isExplicitGlobalModule(); -} - } // namespace clang #endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index d681791d3920c3..8626f04012f7d4 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -4496,7 +4496,7 @@ unsigned FunctionDecl::getODRHash() { } class ODRHash Hash; - Hash.AddFunctionDecl(this); + Hash.AddFunctionDecl(this, /*SkipBody=*/shouldSkipCheckingODR()); setHasODRHash(true); ODRHash = Hash.CalculateHash(); return ODRHash; diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp index fcedb3cfd176a0..04bbc49ab2f319 100644 --- a/clang/lib/AST/DeclBase.cpp +++ b/clang/lib/AST/DeclBase.cpp @@ -1102,6 +1102,11 @@ bool Decl::isInAnotherModuleUnit() const { return M != getASTContext().getCurrentNamedModule(); } +bool Decl::shouldSkipCheckingODR() const { + return getASTContext().getLangOpts().SkipODRCheckInGMF && getOwningModule() && + getOwningModule()->isExplicitGlobalModule(); +} + static Decl::Kind getKind(const Decl *D) { return D->getKind(); } static Decl::Kind getKind(const DeclContext *DC) { return DC->getDeclKind(); } diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 683a076e6bc399..ede9f6e93469b7 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9762,7 +9762,7 @@ void ASTReader::finishPendingActions() { !NonConstDefn->isLateTemplateParsed() && // We only perform ODR checks for decls not in the explicit // global module fragment. - !shouldSkipCheckingODR(FD) && + !FD->shouldSkipCheckingODR() && 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 d5309e3fc31f70..a22f760408c634 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -832,7 +832,7 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) { Reader.mergeDefinitionVisibility(OldDef, ED); // We don't want to check the ODR hash value for declarations from global // module fragment. - if (!shouldSkipCheckingODR(ED) && + if (!ED->shouldSkipCheckingODR() && OldDef->getODRHash() != ED->getODRHash()) Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED); } else { @@ -874,7 +874,7 @@ 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(!shouldSkipCheckingODR(RD)); + assert(!RD->shouldSkipCheckingODR()); RD->setODRHash(Record.readInt()); // Maintain the invariant of a redeclaration chain containing only @@ -2152,7 +2152,7 @@ void ASTDeclReader::MergeDefinitionData( } // We don't want to check ODR for decls in the global module fragment. - if (shouldSkipCheckingODR(MergeDD.Definition)) + if (MergeDD.Definition->shouldSkipCheckingODR()) return; if (D->getODRHash() != MergeDD.ODRHash) { @@ -3526,7 +3526,7 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) { // same template specialization into the same CXXRecordDecl. auto MergedDCIt = Reader.MergedDeclContexts.find(D->getLexicalDeclContext()); if (MergedDCIt != Reader.MergedDeclContexts.end() && - !shouldSkipCheckingODR(D) && MergedDCIt->second == D->getDeclContext()) + !D->shouldSkipCheckingODR() && MergedDCIt->second == D->getDeclContext()) Reader.PendingOdrMergeChecks.push_back(D); return FindExistingResult(Reader, D, /*Existing=*/nullptr, diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 6904c924c2fd3d..3653d94c6e0739 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -6060,7 +6060,7 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) { BitsPacker DefinitionBits; - bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D); + bool ShouldSkipCheckingODR = D->shouldSkipCheckingODR(); DefinitionBits.addBit(ShouldSkipCheckingODR); #define FIELD(Name, Width, Merge) \ diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index e1862de4a35b8f..d04e1c781b4e28 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -519,7 +519,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) { BitsPacker EnumDeclBits; EnumDeclBits.addBits(D->getNumPositiveBits(), /*BitWidth=*/8); EnumDeclBits.addBits(D->getNumNegativeBits(), /*BitWidth=*/8); - bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D); + bool ShouldSkipCheckingODR = D->shouldSkipCheckingODR(); EnumDeclBits.addBit(ShouldSkipCheckingODR); EnumDeclBits.addBit(D->isScoped()); EnumDeclBits.addBit(D->isScopedUsingClassTag()); @@ -545,7 +545,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) { !D->isTopLevelDeclInObjCContainer() && !CXXRecordDecl::classofKind(D->getKind()) && !D->getIntegerTypeSourceInfo() && !D->getMemberSpecializationInfo() && - !needsAnonymousDeclarationNumber(D) && !shouldSkipCheckingODR(D) && + !needsAnonymousDeclarationNumber(D) && !D->shouldSkipCheckingODR() && D->getDeclName().getNameKind() == DeclarationName::Identifier) AbbrevToUse = Writer.getDeclEnumAbbrev(); @@ -711,7 +711,7 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) { // FIXME: stable encoding FunctionDeclBits.addBits(llvm::to_underlying(D->getLinkageInternal()), 3); FunctionDeclBits.addBits((uint32_t)D->getStorageClass(), /*BitWidth=*/3); - bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D); + bool ShouldSkipCheckingODR = D->shouldSkipCheckingODR(); FunctionDeclBits.addBit(ShouldSkipCheckingODR); FunctionDeclBits.addBit(D->isInlineSpecified()); FunctionDeclBits.addBit(D->isInlined()); @@ -1545,7 +1545,7 @@ void ASTDeclWriter::VisitCXXMethodDecl(CXXMethodDecl *D) { D->getFirstDecl() == D->getMostRecentDecl() && !D->isInvalidDecl() && !D->hasAttrs() && !D->isTopLevelDeclInObjCContainer() && D->getDeclName().getNameKind() == DeclarationName::Identifier && - !shouldSkipCheckingODR(D) && !D->hasExtInfo() && + !D->shouldSkipCheckingODR() && !D->hasExtInfo() && !D->isExplicitlyDefaulted()) { if (D->getTemplatedKind() == FunctionDecl::TK_NonTemplate || D->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate || diff --git a/clang/test/Modules/hashing-decls-in-exprs-from-gmf.cppm b/clang/test/Modules/hashing-decls-in-exprs-from-gmf.cppm new file mode 100644 index 00000000000000..8db53c0ace8796 --- /dev/null +++ b/clang/test/Modules/hashing-decls-in-exprs-from-gmf.cppm @@ -0,0 +1,67 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: split-file %s %t +// +// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/A.cppm -emit-module-interface -o %t/A.pcm +// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/B.cppm -emit-module-interface -o %t/B.pcm +// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/test.cpp -fprebuilt-module-path=%t -fsyntax-only -verify + +//--- header.h +#pragma once +template <class _Tp> +class Optional {}; + +template <class _Tp> +concept C = requires(const _Tp& __t) { + []<class _Up>(const Optional<_Up>&) {}(__t); +}; + +//--- func.h +#include "header.h" +template <C T> +void func() {} + +//--- duplicated_func.h +#include "header.h" +template <C T> +void duplicated_func() {} + +//--- test_func.h +#include "func.h" + +void test_func() { + func<Optional<int>>(); +} + +//--- test_duplicated_func.h +#include "duplicated_func.h" + +void test_duplicated_func() { + duplicated_func<Optional<int>>(); +} + +//--- A.cppm +module; +#include "header.h" +#include "test_duplicated_func.h" +export module A; +export using ::test_duplicated_func; + +//--- B.cppm +module; +#include "header.h" +#include "test_func.h" +#include "test_duplicated_func.h" +export module B; +export using ::test_func; +export using ::test_duplicated_func; + +//--- test.cpp +// expected-no-diagnostics +import A; +import B; + +void test() { + test_func(); + test_duplicated_func(); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits