[llvm-branch-commits] [clang] release/18.x: backport [C++20] [Moduls] Avoid computing odr hash for functions from comparing constraint expression (PR #84723)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Sun Mar 10 22:43:06 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-modules
Author: Chuanqi Xu (ChuanqiXu9)
<details>
<summary>Changes</summary>
Backport 3f6bc1adf805681293c2ef0b93b708ff52244c00
This fixes a surprising regression introduced in https://github.com/llvm/llvm-project/issues/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:
- (modified) clang/include/clang/AST/DeclBase.h (+10)
- (modified) clang/include/clang/Serialization/ASTReader.h (-7)
- (modified) clang/lib/AST/Decl.cpp (+1-1)
- (modified) clang/lib/AST/DeclBase.cpp (+5)
- (modified) clang/lib/Serialization/ASTReader.cpp (+1-1)
- (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+4-4)
- (modified) clang/lib/Serialization/ASTWriter.cpp (+1-1)
- (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+4-4)
- (added) clang/test/Modules/hashing-decls-in-exprs-from-gmf.cppm (+67)
``````````diff
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();
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/84723
More information about the llvm-branch-commits
mailing list