From 0bf7ff1028fb7cc81324e9c38c585e6533b754e4 Mon Sep 17 00:00:00 2001 From: Chuanqi Xu Date: Mon, 11 Mar 2024 11:14:40 +0800 Subject: [PATCH] [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. --- clang/include/clang/AST/DeclBase.h | 10 +++ clang/include/clang/Serialization/ASTReader.h | 7 -- clang/lib/AST/Decl.cpp | 2 +- clang/lib/AST/DeclBase.cpp | 5 ++ clang/lib/Serialization/ASTReader.cpp | 2 +- clang/lib/Serialization/ASTReaderDecl.cpp | 8 +-- clang/lib/Serialization/ASTWriter.cpp | 2 +- clang/lib/Serialization/ASTWriterDecl.cpp | 8 +-- .../hashing-decls-in-exprs-from-gmf.cppm | 67 +++++++++++++++++++ 9 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 clang/test/Modules/hashing-decls-in-exprs-from-gmf.cppm diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index 9a4736019d1b1..eb7a1a3206007 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 cd28226c295b3..62c25f5b7a0df 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -2451,13 +2451,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 26fdfa040796e..1ee33fd7576d7 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -4476,7 +4476,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 8163f9bdaf8d9..6b3c13ff206d2 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 028610deb3001..490b8cb10a484 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9745,7 +9745,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(FD)) { PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn); diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 321c11e55c14e..110f55f8c0f49 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 73018c1170d8f..378a1f86bd534 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -6010,7 +6010,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 e73800100e3cc..42583c09f009e 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -488,7 +488,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()); @@ -514,7 +514,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(); @@ -680,7 +680,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()); @@ -1514,7 +1514,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 0000000000000..8db53c0ace879 --- /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 Optional {}; + +template +concept C = requires(const _Tp& __t) { + [](const Optional<_Up>&) {}(__t); +}; + +//--- func.h +#include "header.h" +template +void func() {} + +//--- duplicated_func.h +#include "header.h" +template +void duplicated_func() {} + +//--- test_func.h +#include "func.h" + +void test_func() { + func>(); +} + +//--- test_duplicated_func.h +#include "duplicated_func.h" + +void test_duplicated_func() { + duplicated_func>(); +} + +//--- 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(); +}