release/18.x: backport [C++20] [Moduls] Avoid computing odr hash for functions from comparing constraint expression #84723
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesBackport 3f6bc1a This fixes a surprising regression introduced in #79240. Given we've backported that to 18.x. We need to backport this fix to 18.x too. Full diff: https://github.com/llvm/llvm-project/pull/84723.diff 9 Files Affected:
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 9a4736019d1b1b..eb7a1a32060077 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 cd28226c295b32..62c25f5b7a0df8 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 26fdfa040796ed..1ee33fd7576d7d 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 8163f9bdaf8d97..6b3c13ff206d23 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 028610deb3001e..490b8cb10a4841 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<CXXMethodDecl>(FD)) {
PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn);
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 321c11e55c14e3..110f55f8c0f49a 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 73018c1170d8f2..378a1f86bd5342 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 e73800100e3ccf..42583c09f009e0 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 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();
+}
|
|
@tstellar Could we backport this to 18.x? |
…g 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 3f6bc1a
This fixes a surprising regression introduced in #79240. Given we've backported that to 18.x. We need to backport this fix to 18.x too.