[clang] 2cee266 - [Concepts] Correctly handle failure when checking concepts recursively
Erich Keane via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 1 12:19:07 PDT 2022
Author: Erich Keane
Date: 2022-11-01T12:18:58-07:00
New Revision: 2cee2663338ee65ff5786432c30ba22c8dcd711b
URL: https://github.com/llvm/llvm-project/commit/2cee2663338ee65ff5786432c30ba22c8dcd711b
DIFF: https://github.com/llvm/llvm-project/commit/2cee2663338ee65ff5786432c30ba22c8dcd711b.diff
LOG: [Concepts] Correctly handle failure when checking concepts recursively
Based on discussion on the core reflector, it was made clear that a
concept that depends on itself should be a hard error, not a constraint
failure. This patch implements a stack of constraint-checks-in-progress
to make sure we quit, rather than hitting stack-exhaustion.
Note that we DO need to be careful to make sure we still check
constraints properly that are caused by a previous constraint, but not
derived from (such as when a check causes us to check special member
function generation), so we cannot use the existing logic to see if this
is being instantiated.
This fixes https://github.com/llvm/llvm-project/issues/44304 and
https://github.com/llvm/llvm-project/issues/50891.
Differential Revision: https://reviews.llvm.org/D136975
Added:
clang/test/SemaTemplate/concepts-recursive-inst.cpp
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaConcept.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaOverload.cpp
Removed:
clang/test/SemaTemplate/concepts-GH53213.cpp
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 488d4d6efb5d0..3085d4b62522e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -270,6 +270,10 @@ Bug Fixes
`Issue 58679 <https://github.com/llvm/llvm-project/issues/58679>`_
- Fix a crash when a ``btf_type_tag`` attribute is applied to the pointee of
a function pointer.
+- Fix a number of recursively-instantiated constraint issues, which would possibly
+ result in a stack overflow.
+ `Issue 44304 <https://github.com/llvm/llvm-project/issues/44304>`_
+ `Issue 50891 <https://github.com/llvm/llvm-project/issues/50891>`_
Improvements to Clang's diagnostics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d3299f6314570..a3da8abde58a6 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5205,6 +5205,8 @@ def err_function_template_partial_spec : Error<
def err_template_recursion_depth_exceeded : Error<
"recursive template instantiation exceeded maximum depth of %0">,
DefaultFatal, NoSFINAE;
+def err_constraint_depends_on_self : Error<
+ "satisfaction of constraint '%0' depends on itself">, NoSFINAE;
def note_template_recursion_depth : Note<
"use -ftemplate-depth=N to increase recursive template instantiation depth">;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index fb9906e24b5ab..46c139d6c62b0 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7221,7 +7221,43 @@ class Sema final {
FunctionDecl *FD, llvm::Optional<ArrayRef<TemplateArgument>> TemplateArgs,
LocalInstantiationScope &Scope);
+private:
+ // The current stack of constraint satisfactions, so we can exit-early.
+ llvm::SmallVector<llvm::FoldingSetNodeID, 10> SatisfactionStack;
+
public:
+ void PushSatisfactionStackEntry(const llvm::FoldingSetNodeID &ID) {
+ SatisfactionStack.push_back(ID);
+ }
+
+ void PopSatisfactionStackEntry() { SatisfactionStack.pop_back(); }
+
+ bool SatisfactionStackContains(const llvm::FoldingSetNodeID &ID) const {
+ return llvm::find(SatisfactionStack, ID) != SatisfactionStack.end();
+ }
+
+ // Resets the current SatisfactionStack for cases where we are instantiating
+ // constraints as a 'side effect' of normal instantiation in a way that is not
+ // indicative of recursive definition.
+ class SatisfactionStackResetRAII {
+ llvm::SmallVector<llvm::FoldingSetNodeID, 10> BackupSatisfactionStack;
+ Sema &SemaRef;
+
+ public:
+ SatisfactionStackResetRAII(Sema &S) : SemaRef(S) {
+ SemaRef.SwapSatisfactionStack(BackupSatisfactionStack);
+ }
+
+ ~SatisfactionStackResetRAII() {
+ SemaRef.SwapSatisfactionStack(BackupSatisfactionStack);
+ }
+ };
+
+ void
+ SwapSatisfactionStack(llvm::SmallVectorImpl<llvm::FoldingSetNodeID> &NewSS) {
+ SatisfactionStack.swap(NewSS);
+ }
+
const NormalizedConstraint *
getNormalizedAssociatedConstraints(
NamedDecl *ConstrainedDecl, ArrayRef<const Expr *> AssociatedConstraints);
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 63c58d64c472b..b02054bd97aaf 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -146,6 +146,17 @@ bool Sema::CheckConstraintExpression(const Expr *ConstraintExpression,
return true;
}
+namespace {
+struct SatisfactionStackRAII {
+ Sema &SemaRef;
+ SatisfactionStackRAII(Sema &SemaRef, llvm::FoldingSetNodeID FSNID)
+ : SemaRef(SemaRef) {
+ SemaRef.PushSatisfactionStackEntry(FSNID);
+ }
+ ~SatisfactionStackRAII() { SemaRef.PopSatisfactionStackEntry(); }
+};
+} // namespace
+
template <typename AtomicEvaluator>
static ExprResult
calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
@@ -258,6 +269,29 @@ calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
return SubstitutedAtomicExpr;
}
+static bool
+DiagRecursiveConstraintEval(Sema &S, llvm::FoldingSetNodeID &ID, const Expr *E,
+ const MultiLevelTemplateArgumentList &MLTAL) {
+ E->Profile(ID, S.Context, /*Canonical=*/true);
+ for (const auto &List : MLTAL)
+ for (const auto &TemplateArg : List.Args)
+ TemplateArg.Profile(ID, S.Context);
+
+ // Note that we have to do this with our own collection, because there are
+ // times where a constraint-expression check can cause us to need to evaluate
+ // other constriants that are unrelated, such as when evaluating a recovery
+ // expression, or when trying to determine the constexpr-ness of special
+ // members. Otherwise we could just use the
+ // Sema::InstantiatingTemplate::isAlreadyBeingInstantiated function.
+ if (S.SatisfactionStackContains(ID)) {
+ S.Diag(E->getExprLoc(), diag::err_constraint_depends_on_self)
+ << const_cast<Expr *>(E) << E->getSourceRange();
+ return true;
+ }
+
+ return false;
+}
+
static ExprResult calculateConstraintSatisfaction(
Sema &S, const NamedDecl *Template, SourceLocation TemplateNameLoc,
const MultiLevelTemplateArgumentList &MLTAL, const Expr *ConstraintExpr,
@@ -278,6 +312,16 @@ static ExprResult calculateConstraintSatisfaction(
AtomicExpr->getSourceRange());
if (Inst.isInvalid())
return ExprError();
+
+ llvm::FoldingSetNodeID ID;
+ if (DiagRecursiveConstraintEval(S, ID, AtomicExpr, MLTAL)) {
+ Satisfaction.IsSatisfied = false;
+ Satisfaction.ContainsErrors = true;
+ return ExprEmpty();
+ }
+
+ SatisfactionStackRAII StackRAII(S, ID);
+
// We do not want error diagnostics escaping here.
Sema::SFINAETrap Trap(S);
SubstitutedExpression =
@@ -414,6 +458,7 @@ bool Sema::CheckConstraintSatisfaction(
if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
ConvertedConstraints, TemplateArgsLists,
TemplateIDRange, *Satisfaction)) {
+ OutSatisfaction = *Satisfaction;
return true;
}
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 1e5a56a14fba4..80b748cbb3744 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7193,6 +7193,11 @@ specialMemberIsConstexpr(Sema &S, CXXRecordDecl *ClassDecl,
bool ConstRHS,
CXXConstructorDecl *InheritedCtor = nullptr,
Sema::InheritedConstructorInfo *Inherited = nullptr) {
+ // Suppress duplicate constraint checking here, in case a constraint check
+ // caused us to decide to do this. Any truely recursive checks will get
+ // caught during these checks anyway.
+ Sema::SatisfactionStackResetRAII SSRAII{S};
+
// If we're inheriting a constructor, see if we need to call it for this base
// class.
if (InheritedCtor) {
@@ -7289,8 +7294,8 @@ static bool defaultedSpecialMemberIsConstexpr(
// class is a constexpr function, and
for (const auto &B : ClassDecl->bases()) {
const RecordType *BaseType = B.getType()->getAs<RecordType>();
- if (!BaseType) continue;
-
+ if (!BaseType)
+ continue;
CXXRecordDecl *BaseClassDecl = cast<CXXRecordDecl>(BaseType->getDecl());
if (!specialMemberIsConstexpr(S, BaseClassDecl, CSM, 0, ConstArg,
InheritedCtor, Inherited))
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index a65cdbed7fd02..7972f994982b7 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -13147,17 +13147,16 @@ DiagnoseTwoPhaseOperatorLookup(Sema &SemaRef, OverloadedOperatorKind Op,
namespace {
class BuildRecoveryCallExprRAII {
Sema &SemaRef;
+ Sema::SatisfactionStackResetRAII SatStack;
+
public:
- BuildRecoveryCallExprRAII(Sema &S) : SemaRef(S) {
+ BuildRecoveryCallExprRAII(Sema &S) : SemaRef(S), SatStack(S) {
assert(SemaRef.IsBuildingRecoveryCallExpr == false);
SemaRef.IsBuildingRecoveryCallExpr = true;
}
- ~BuildRecoveryCallExprRAII() {
- SemaRef.IsBuildingRecoveryCallExpr = false;
- }
+ ~BuildRecoveryCallExprRAII() { SemaRef.IsBuildingRecoveryCallExpr = false; }
};
-
}
/// Attempts to recover from a call where no functions were found.
diff --git a/clang/test/SemaTemplate/concepts-GH53213.cpp b/clang/test/SemaTemplate/concepts-GH53213.cpp
deleted file mode 100644
index 23bd70b013027..0000000000000
--- a/clang/test/SemaTemplate/concepts-GH53213.cpp
+++ /dev/null
@@ -1,49 +0,0 @@
-// RUN: %clang_cc1 -std=c++20 -verify %s
-namespace GH53213 {
-template<typename T>
-concept c = requires(T t) { f(t); }; // #CDEF
-
-auto f(c auto); // #FDEF
-
-void g() {
- f(0);
- // expected-error at -1{{no matching function for call to 'f'}}
- // expected-note@#FDEF{{constraints not satisfied}}
- // expected-note@#FDEF{{because 'int' does not satisfy 'c'}}
- // expected-note@#CDEF{{because 'f(t)' would be invalid: no matching function for call to 'f'}}
-}
-} // namespace GH53213
-
-namespace GH45736 {
-struct constrained;
-
-template<typename T>
- struct type {
- };
-template<typename T>
- constexpr bool f(type<T>) {
- return true;
- }
-
-template<typename T>
- concept matches = f(type<T>());
-
-
-struct constrained {
- template<typename U> requires matches<U>
- explicit constrained(U value) {
- }
-};
-
-bool f(constrained const &) {
- return true;
-}
-
-struct outer {
- constrained state;
-};
-
-bool f(outer const & x) {
- return f(x.state);
-}
-} // namespace GH45736
diff --git a/clang/test/SemaTemplate/concepts-recursive-inst.cpp b/clang/test/SemaTemplate/concepts-recursive-inst.cpp
new file mode 100644
index 0000000000000..651ce1a490558
--- /dev/null
+++ b/clang/test/SemaTemplate/concepts-recursive-inst.cpp
@@ -0,0 +1,122 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+namespace GH53213 {
+template<typename T>
+concept c = requires(T t) { f(t); }; // #CDEF
+
+auto f(c auto); // #FDEF
+
+void g() {
+ f(0);
+ // expected-error at -1{{no matching function for call to 'f'}}
+ // expected-note@#FDEF{{constraints not satisfied}}
+ // expected-note@#FDEF{{because 'int' does not satisfy 'c'}}
+ // expected-note@#CDEF{{because 'f(t)' would be invalid: no matching function for call to 'f'}}
+}
+} // namespace GH53213
+
+namespace GH45736 {
+struct constrained;
+
+template<typename T>
+ struct type {
+ };
+template<typename T>
+ constexpr bool f(type<T>) {
+ return true;
+ }
+
+template<typename T>
+ concept matches = f(type<T>());
+
+
+struct constrained {
+ template<typename U> requires matches<U>
+ explicit constrained(U value) {
+ }
+};
+
+bool f(constrained const &) {
+ return true;
+}
+
+struct outer {
+ constrained state;
+};
+
+bool f(outer const & x) {
+ return f(x.state);
+}
+} // namespace GH45736
+
+namespace DirectRecursiveCheck {
+template<class T>
+concept NotInf = true;
+template<class T>
+concept Inf = requires(T& v){ // #INF_REQ
+ {begin(v)}; // #INF_BEGIN_EXPR
+};
+
+void begin(NotInf auto& v){ } // #NOTINF_BEGIN
+// This lookup should fail, since it results in a recursive check.
+// However, this is a 'hard failure'(not a SFINAE failure or constraints
+// violation), so it needs to cause the entire lookup to fail.
+void begin(Inf auto& v){ } // #INF_BEGIN
+
+struct my_range{
+} rng;
+
+void baz() {
+auto it = begin(rng); // #BEGIN_CALL
+// expected-error@#INF_BEGIN {{satisfaction of constraint 'Inf<Inf auto>' depends on itself}}
+// expected-note@#INF_BEGIN {{while substituting template arguments into constraint expression here}}
+// expected-note@#INF_BEGIN_EXPR {{while checking constraint satisfaction for template 'begin<DirectRecursiveCheck::my_range>' required here}}
+// expected-note@#INF_BEGIN_EXPR {{while substituting deduced template arguments into function template 'begin'}}
+// expected-note@#INF_BEGIN_EXPR {{in instantiation of requirement here}}
+// expected-note@#INF_REQ {{while substituting template arguments into constraint expression here}}
+// expected-note@#INF_BEGIN {{while checking the satisfaction of concept 'Inf<DirectRecursiveCheck::my_range>' requested here}}
+// expected-note@#INF_BEGIN {{while substituting template arguments into constraint expression here}}
+// expected-note@#BEGIN_CALL {{while checking constraint satisfaction for template 'begin<DirectRecursiveCheck::my_range>' required here}}
+// expected-note@#BEGIN_CALL {{in instantiation of function template specialization}}
+
+// Fallout of the failure is failed lookup, which is necessary to stop odd
+// cascading errors.
+// expected-error@#BEGIN_CALL {{no matching function for call to 'begin'}}
+// expected-note@#NOTINF_BEGIN {{candidate function}}
+// expected-note@#INF_BEGIN{{candidate template ignored: constraints not satisfied}}
+}
+} // namespace DirectRecursiveCheck
+
+namespace GH50891 {
+ template <typename T>
+ concept Numeric = requires(T a) { // #NUMERIC
+ foo(a); // #FOO_CALL
+ };
+
+ struct Deferred {
+ friend void foo(Deferred);
+ template <Numeric TO> operator TO(); // #OP_TO
+ };
+
+ static_assert(Numeric<Deferred>); // #STATIC_ASSERT
+ // expected-error@#OP_TO {{satisfaction of constraint 'Numeric<TO>' depends on itself}}
+ // expected-note@#OP_TO {{while substituting template arguments into constraint expression here}}
+ // FIXME: The following two refer to type-parameter-0-0, it would be nice to
+ // see if we could instead diagnose with the sugared name.
+ // expected-note@#FOO_CALL {{while checking constraint satisfaction for template}}
+ // expected-note@#FOO_CALL {{while substituting deduced template arguments into function template}}
+ // expected-note@#FOO_CALL {{in instantiation of requirement here}}
+ // expected-note@#NUMERIC {{while substituting template arguments into constraint expression here}}
+ // expected-note@#OP_TO {{skipping 2 contexts in backtrace}}
+ // expected-note@#FOO_CALL {{while checking constraint satisfaction for template}}
+ // expected-note@#FOO_CALL {{in instantiation of function template specialization}}
+ // expected-note@#FOO_CALL {{in instantiation of requirement here}}
+ // expected-note@#NUMERIC {{while substituting template arguments into constraint expression here}}
+ // expected-note@#STATIC_ASSERT{{while checking the satisfaction of concept 'Numeric<Deferred>' requested here}}
+
+ // Fallout of that failure is that deferred does not satisfy numeric,
+ // which is unfortunate, but about what we can accomplish here.
+ // expected-error@#STATIC_ASSERT {{static assertion failed}}
+ // expected-note@#STATIC_ASSERT{{because 'Deferred' does not satisfy 'Numeric'}}
+ // expected-note@#FOO_CALL {{because 'foo(a)' would be invalid}}
+} // namespace GH50891
+
More information about the cfe-commits
mailing list