[clang] d26dd58 - [StmtProfile] Don't profile the body of lambda expressions
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 16 00:42:51 PDT 2024
Author: Chuanqi Xu
Date: 2024-04-16T15:41:26+08:00
New Revision: d26dd58ca5b59032eb371b8f51d9134acdd8d3ad
URL: https://github.com/llvm/llvm-project/commit/d26dd58ca5b59032eb371b8f51d9134acdd8d3ad
DIFF: https://github.com/llvm/llvm-project/commit/d26dd58ca5b59032eb371b8f51d9134acdd8d3ad.diff
LOG: [StmtProfile] Don't profile the body of lambda expressions
Close https://github.com/llvm/llvm-project/issues/87609
We tried to profile the body of the lambda expressions in
https://reviews.llvm.org/D153957. But as the original comments show,
it is indeed dangerous. After we tried to skip calculating the ODR
hash values recently, we have fall into this trap twice.
So in this patch, I choose to not profile the body of the lambda
expression. The signature of the lambda is still profiled.
Added:
clang/test/Modules/hashing-decls-in-exprs-from-gmf-2.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/AST/StmtProfile.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 2194d268fa86f0..1079993f496945 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -672,16 +672,6 @@ class alignas(8) Decl {
/// Whether this declaration comes from explicit global module.
bool isFromExplicitGlobalModule() const;
- /// 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 e3fde887f99cb7..43ee06c524b3a0 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2457,6 +2457,12 @@ class BitsUnpacker {
uint32_t Value;
uint32_t CurrentBitsIndex = ~0;
};
+
+inline bool shouldSkipCheckingODR(const Decl *D) {
+ return D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
+ D->isFromExplicitGlobalModule();
+}
+
} // namespace clang
#endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 2b2d5a2663a18b..33b6f8611f2162 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -4534,7 +4534,7 @@ unsigned FunctionDecl::getODRHash() {
}
class ODRHash Hash;
- Hash.AddFunctionDecl(this, /*SkipBody=*/shouldSkipCheckingODR());
+ Hash.AddFunctionDecl(this);
setHasODRHash(true);
ODRHash = Hash.CalculateHash();
return ODRHash;
diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
index 66a727d9dd0c39..434926324c96ca 100644
--- a/clang/lib/AST/DeclBase.cpp
+++ b/clang/lib/AST/DeclBase.cpp
@@ -1106,11 +1106,6 @@ bool Decl::isFromExplicitGlobalModule() const {
return getOwningModule() && getOwningModule()->isExplicitGlobalModule();
}
-bool Decl::shouldSkipCheckingODR() const {
- return getASTContext().getLangOpts().SkipODRCheckInGMF &&
- isFromExplicitGlobalModule();
-}
-
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/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index d2aac1e640380f..789e4634bd293b 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2071,13 +2071,31 @@ StmtProfiler::VisitLambdaExpr(const LambdaExpr *S) {
}
CXXRecordDecl *Lambda = S->getLambdaClass();
- ID.AddInteger(Lambda->getODRHash());
-
for (const auto &Capture : Lambda->captures()) {
ID.AddInteger(Capture.getCaptureKind());
if (Capture.capturesVariable())
VisitDecl(Capture.getCapturedVar());
}
+
+ // Profiling the body of the lambda may be dangerous during deserialization.
+ // So we'd like only to profile the signature here.
+ ODRHash Hasher;
+ // FIXME: We can't get the operator call easily by
+ // `CXXRecordDecl::getLambdaCallOperator()` if we're in deserialization.
+ // So we have to do something raw here.
+ for (auto *SubDecl : Lambda->decls()) {
+ FunctionDecl *Call = nullptr;
+ if (auto *FTD = dyn_cast<FunctionTemplateDecl>(SubDecl))
+ Call = FTD->getTemplatedDecl();
+ else if (auto *FD = dyn_cast<FunctionDecl>(SubDecl))
+ Call = FD;
+
+ if (!Call)
+ continue;
+
+ Hasher.AddFunctionDecl(Call, /*SkipBody=*/true);
+ }
+ ID.AddInteger(Hasher.CalculateHash());
}
void
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index feb60bc54413a5..f47d540ea4b86d 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9785,7 +9785,7 @@ void ASTReader::finishPendingActions() {
!NonConstDefn->isLateTemplateParsed() &&
// We only perform ODR checks for decls not in the explicit
// global module fragment.
- !FD->shouldSkipCheckingODR() &&
+ !shouldSkipCheckingODR(FD) &&
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 e4b6a75c118ba3..74d40f7da34cad 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -826,7 +826,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 (!ED->shouldSkipCheckingODR() &&
+ if (!shouldSkipCheckingODR(ED) &&
OldDef->getODRHash() != ED->getODRHash())
Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
} else {
@@ -868,7 +868,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(!RD->shouldSkipCheckingODR());
+ assert(!shouldSkipCheckingODR(RD));
RD->setODRHash(Record.readInt());
// Maintain the invariant of a redeclaration chain containing only
@@ -2155,7 +2155,7 @@ void ASTDeclReader::MergeDefinitionData(
}
// We don't want to check ODR for decls in the global module fragment.
- if (MergeDD.Definition->shouldSkipCheckingODR())
+ if (shouldSkipCheckingODR(MergeDD.Definition))
return;
if (D->getODRHash() != MergeDD.ODRHash) {
@@ -3530,7 +3530,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() &&
- !D->shouldSkipCheckingODR() && MergedDCIt->second == D->getDeclContext())
+ !shouldSkipCheckingODR(D) && 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 85b7fd5535a1bf..ce6fa1feb1eeb3 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -6188,7 +6188,7 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
BitsPacker DefinitionBits;
- bool ShouldSkipCheckingODR = D->shouldSkipCheckingODR();
+ bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D);
DefinitionBits.addBit(ShouldSkipCheckingODR);
#define FIELD(Name, Width, Merge) \
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 276b6257f1d841..d0d49bcdf991a9 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -526,7 +526,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
BitsPacker EnumDeclBits;
EnumDeclBits.addBits(D->getNumPositiveBits(), /*BitWidth=*/8);
EnumDeclBits.addBits(D->getNumNegativeBits(), /*BitWidth=*/8);
- bool ShouldSkipCheckingODR = D->shouldSkipCheckingODR();
+ bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D);
EnumDeclBits.addBit(ShouldSkipCheckingODR);
EnumDeclBits.addBit(D->isScoped());
EnumDeclBits.addBit(D->isScopedUsingClassTag());
@@ -552,7 +552,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
!D->isTopLevelDeclInObjCContainer() &&
!CXXRecordDecl::classofKind(D->getKind()) &&
!D->getIntegerTypeSourceInfo() && !D->getMemberSpecializationInfo() &&
- !needsAnonymousDeclarationNumber(D) && !D->shouldSkipCheckingODR() &&
+ !needsAnonymousDeclarationNumber(D) && !shouldSkipCheckingODR(D) &&
D->getDeclName().getNameKind() == DeclarationName::Identifier)
AbbrevToUse = Writer.getDeclEnumAbbrev();
@@ -718,7 +718,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 = D->shouldSkipCheckingODR();
+ bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D);
FunctionDeclBits.addBit(ShouldSkipCheckingODR);
FunctionDeclBits.addBit(D->isInlineSpecified());
FunctionDeclBits.addBit(D->isInlined());
@@ -1559,7 +1559,7 @@ void ASTDeclWriter::VisitCXXMethodDecl(CXXMethodDecl *D) {
D->getFirstDecl() == D->getMostRecentDecl() && !D->isInvalidDecl() &&
!D->hasAttrs() && !D->isTopLevelDeclInObjCContainer() &&
D->getDeclName().getNameKind() == DeclarationName::Identifier &&
- !D->shouldSkipCheckingODR() && !D->hasExtInfo() &&
+ !shouldSkipCheckingODR(D) && !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-2.cppm b/clang/test/Modules/hashing-decls-in-exprs-from-gmf-2.cppm
new file mode 100644
index 00000000000000..66143102cb9e40
--- /dev/null
+++ b/clang/test/Modules/hashing-decls-in-exprs-from-gmf-2.cppm
@@ -0,0 +1,44 @@
+// 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/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() {}
+
+//--- test_func.h
+#include "func.h"
+
+inline void test_func() {
+ func<Optional<int>>();
+}
+
+//--- A.cppm
+module;
+#include "header.h"
+#include "test_func.h"
+export module A;
+export using ::test_func;
+
+//--- test.cpp
+// expected-no-diagnostics
+import A;
+#include "test_func.h"
+
+void test() {
+ test_func();
+}
More information about the cfe-commits
mailing list