[clang] 3f6bc1a - [C++20] [Moduls] Avoid computing odr hash for functions from comparing constraint expression
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 10 20:50:25 PDT 2024
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();
+}
More information about the cfe-commits
mailing list