[clang] separate recursive instantiation check from CodeSynthesisContext #162224
+142
−62
Conversation
This file contains hidden or 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 Author: Matheus Izvekov (mizvekov) ChangesThis makes pushing / popping CodeSynthesisContexts much cheaper, as it delegates to another class this functionality which is not actually needed in most cases. It also converts a bunch of these uses into just asserts. This improves compiler performance a little bit: Some diagnostics changes, because we avoid printing a redundant context notes. Full diff: https://github.com/llvm/llvm-project/pull/162224.diff 5 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 265462a86e405..7456285472dd1 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -12995,6 +12995,36 @@ class Sema final : public SemaBase {
/// default arguments of its methods have been parsed.
UnparsedDefaultArgInstantiationsMap UnparsedDefaultArgInstantiations;
+ using InstantiatingSpecializationsKey = llvm::PointerIntPair<Decl *, 2>;
+
+ struct RecursiveInstGuard {
+ enum class Kind {
+ Template,
+ DefaultArgument,
+ ExceptionSpec,
+ };
+
+ RecursiveInstGuard(Sema &S, Decl *D, Kind Kind)
+ : S(S), Key(D->getCanonicalDecl(), unsigned(Kind)) {
+ auto [_, Created] = S.InstantiatingSpecializations.insert(Key);
+ if (!Created)
+ Key = {};
+ }
+
+ ~RecursiveInstGuard() {
+ if (Key.getOpaqueValue()) {
+ [[maybe_unused]] bool Found = S.InstantiatingSpecializations.erase(Key);
+ assert(Found);
+ }
+ }
+
+ operator bool() const { return Key.getOpaqueValue() == nullptr; }
+
+ private:
+ Sema &S;
+ Sema::InstantiatingSpecializationsKey Key;
+ };
+
/// A context in which code is being synthesized (where a source location
/// alone is not sufficient to identify the context). This covers template
/// instantiation and various forms of implicitly-generated functions.
@@ -13356,14 +13386,9 @@ class Sema final : public SemaBase {
/// recursive template instantiations.
bool isInvalid() const { return Invalid; }
- /// Determine whether we are already instantiating this
- /// specialization in some surrounding active instantiation.
- bool isAlreadyInstantiating() const { return AlreadyInstantiating; }
-
private:
Sema &SemaRef;
bool Invalid;
- bool AlreadyInstantiating;
InstantiatingTemplate(Sema &SemaRef,
CodeSynthesisContext::SynthesisKind Kind,
@@ -13486,7 +13511,7 @@ class Sema final : public SemaBase {
SmallVector<CodeSynthesisContext, 16> CodeSynthesisContexts;
/// Specializations whose definitions are currently being instantiated.
- llvm::DenseSet<std::pair<Decl *, unsigned>> InstantiatingSpecializations;
+ llvm::DenseSet<InstantiatingSpecializationsKey> InstantiatingSpecializations;
/// Non-dependent types used in templates that have already been instantiated
/// by some template instantiation.
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 7b05e4cf1385f..69b76378db3bc 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -638,15 +638,8 @@ Sema::InstantiatingTemplate::InstantiatingTemplate(
}
Invalid = SemaRef.pushCodeSynthesisContext(Inst);
- if (!Invalid) {
- AlreadyInstantiating =
- !Inst.Entity
- ? false
- : !SemaRef.InstantiatingSpecializations
- .insert({Inst.Entity->getCanonicalDecl(), Inst.Kind})
- .second;
+ if (!Invalid)
atTemplateBegin(SemaRef.TemplateInstCallbacks, SemaRef, Inst);
- }
}
Sema::InstantiatingTemplate::InstantiatingTemplate(
@@ -901,13 +894,6 @@ void Sema::popCodeSynthesisContext() {
void Sema::InstantiatingTemplate::Clear() {
if (!Invalid) {
- if (!AlreadyInstantiating) {
- auto &Active = SemaRef.CodeSynthesisContexts.back();
- if (Active.Entity)
- SemaRef.InstantiatingSpecializations.erase(
- {Active.Entity->getCanonicalDecl(), Active.Kind});
- }
-
atTemplateEnd(SemaRef.TemplateInstCallbacks, SemaRef,
SemaRef.CodeSynthesisContexts.back());
@@ -3311,17 +3297,21 @@ bool Sema::SubstDefaultArgument(
FunctionDecl *FD = cast<FunctionDecl>(Param->getDeclContext());
Expr *PatternExpr = Param->getUninstantiatedDefaultArg();
+ RecursiveInstGuard AlreadyInstantiating(
+ *this, Param, RecursiveInstGuard::Kind::DefaultArgument);
+ if (AlreadyInstantiating) {
+ Diag(Param->getBeginLoc(), diag::err_recursive_default_argument)
+ << FD << PatternExpr->getSourceRange();
+ Param->setInvalidDecl();
+ return true;
+ }
+
EnterExpressionEvaluationContext EvalContext(
*this, ExpressionEvaluationContext::PotentiallyEvaluated, Param);
InstantiatingTemplate Inst(*this, Loc, Param, TemplateArgs.getInnermost());
if (Inst.isInvalid())
return true;
- if (Inst.isAlreadyInstantiating()) {
- Diag(Param->getBeginLoc(), diag::err_recursive_default_argument) << FD;
- Param->setInvalidDecl();
- return true;
- }
ExprResult Result;
// C++ [dcl.fct.default]p5:
@@ -3559,6 +3549,12 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,
const MultiLevelTemplateArgumentList &TemplateArgs,
TemplateSpecializationKind TSK,
bool Complain) {
+#ifndef NDEBUG
+ RecursiveInstGuard AlreadyInstantiating(*this, Instantiation,
+ RecursiveInstGuard::Kind::Template);
+ assert(!AlreadyInstantiating && "should have been caught by caller");
+#endif
+
CXXRecordDecl *PatternDef
= cast_or_null<CXXRecordDecl>(Pattern->getDefinition());
if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Instantiation,
@@ -3595,7 +3591,6 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,
InstantiatingTemplate Inst(*this, PointOfInstantiation, Instantiation);
if (Inst.isInvalid())
return true;
- assert(!Inst.isAlreadyInstantiating() && "should have been caught by caller");
PrettyDeclStackTraceEntry CrashInfo(Context, Instantiation, SourceLocation(),
"instantiating class definition");
@@ -3807,6 +3802,12 @@ bool Sema::InstantiateEnum(SourceLocation PointOfInstantiation,
EnumDecl *Instantiation, EnumDecl *Pattern,
const MultiLevelTemplateArgumentList &TemplateArgs,
TemplateSpecializationKind TSK) {
+#ifndef NDEBUG
+ RecursiveInstGuard AlreadyInstantiating(*this, Instantiation,
+ RecursiveInstGuard::Kind::Template);
+ assert(!AlreadyInstantiating && "should have been caught by caller");
+#endif
+
EnumDecl *PatternDef = Pattern->getDefinition();
if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Instantiation,
Instantiation->getInstantiatedFromMemberEnum(),
@@ -3824,8 +3825,6 @@ bool Sema::InstantiateEnum(SourceLocation PointOfInstantiation,
InstantiatingTemplate Inst(*this, PointOfInstantiation, Instantiation);
if (Inst.isInvalid())
return true;
- if (Inst.isAlreadyInstantiating())
- return false;
PrettyDeclStackTraceEntry CrashInfo(Context, Instantiation, SourceLocation(),
"instantiating enum definition");
@@ -3864,6 +3863,15 @@ bool Sema::InstantiateInClassInitializer(
Pattern->getInClassInitStyle() &&
"pattern and instantiation disagree about init style");
+ RecursiveInstGuard AlreadyInstantiating(*this, Instantiation,
+ RecursiveInstGuard::Kind::Template);
+ if (AlreadyInstantiating) {
+ // Error out if we hit an instantiation cycle for this initializer.
+ Diag(PointOfInstantiation, diag::err_default_member_initializer_cycle)
+ << Instantiation;
+ return true;
+ }
+
// Error out if we haven't parsed the initializer of the pattern yet because
// we are waiting for the closing brace of the outer class.
Expr *OldInit = Pattern->getInClassInitializer();
@@ -3882,12 +3890,6 @@ bool Sema::InstantiateInClassInitializer(
InstantiatingTemplate Inst(*this, PointOfInstantiation, Instantiation);
if (Inst.isInvalid())
return true;
- if (Inst.isAlreadyInstantiating()) {
- // Error out if we hit an instantiation cycle for this initializer.
- Diag(PointOfInstantiation, diag::err_default_member_initializer_cycle)
- << Instantiation;
- return true;
- }
PrettyDeclStackTraceEntry CrashInfo(Context, Instantiation, SourceLocation(),
"instantiating default member init");
@@ -3968,11 +3970,15 @@ static ActionResult<CXXRecordDecl *> getPatternForClassTemplateSpecialization(
Sema &S, SourceLocation PointOfInstantiation,
ClassTemplateSpecializationDecl *ClassTemplateSpec,
TemplateSpecializationKind TSK, bool PrimaryStrictPackMatch) {
+#ifndef NDEBUG
+ Sema::RecursiveInstGuard AlreadyInstantiating(
+ S, ClassTemplateSpec, Sema::RecursiveInstGuard::Kind::Template);
+ assert(!AlreadyInstantiating && "should have been caught by caller");
+#endif
+
Sema::InstantiatingTemplate Inst(S, PointOfInstantiation, ClassTemplateSpec);
if (Inst.isInvalid())
return {/*Invalid=*/true};
- if (Inst.isAlreadyInstantiating())
- return {/*Invalid=*/false};
llvm::PointerUnion<ClassTemplateDecl *,
ClassTemplatePartialSpecializationDecl *>
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 3819f775811e5..384ef3f3c6f34 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5282,6 +5282,16 @@ void Sema::InstantiateExceptionSpec(SourceLocation PointOfInstantiation,
if (Proto->getExceptionSpecType() != EST_Uninstantiated)
return;
+ RecursiveInstGuard AlreadyInstantiating(
+ *this, Decl, RecursiveInstGuard::Kind::ExceptionSpec);
+ if (AlreadyInstantiating) {
+ // This exception specification indirectly depends on itself. Reject.
+ // FIXME: Corresponding rule in the standard?
+ Diag(PointOfInstantiation, diag::err_exception_spec_cycle) << Decl;
+ UpdateExceptionSpec(Decl, EST_None);
+ return;
+ }
+
InstantiatingTemplate Inst(*this, PointOfInstantiation, Decl,
InstantiatingTemplate::ExceptionSpecification());
if (Inst.isInvalid()) {
@@ -5290,13 +5300,6 @@ void Sema::InstantiateExceptionSpec(SourceLocation PointOfInstantiation,
UpdateExceptionSpec(Decl, EST_None);
return;
}
- if (Inst.isAlreadyInstantiating()) {
- // This exception specification indirectly depends on itself. Reject.
- // FIXME: Corresponding rule in the standard?
- Diag(PointOfInstantiation, diag::err_exception_spec_cycle) << Decl;
- UpdateExceptionSpec(Decl, EST_None);
- return;
- }
// Enter the scope of this instantiation. We don't use
// PushDeclContext because we don't have a scope.
@@ -5356,8 +5359,6 @@ TemplateDeclInstantiator::InitFunctionInstantiation(FunctionDecl *New,
if (ActiveInst.Kind == ActiveInstType::ExplicitTemplateArgumentSubstitution ||
ActiveInst.Kind == ActiveInstType::DeducedTemplateArgumentSubstitution) {
if (isa<FunctionTemplateDecl>(ActiveInst.Entity)) {
- SemaRef.InstantiatingSpecializations.erase(
- {ActiveInst.Entity->getCanonicalDecl(), ActiveInst.Kind});
atTemplateEnd(SemaRef.TemplateInstCallbacks, SemaRef, ActiveInst);
ActiveInst.Kind = ActiveInstType::TemplateInstantiation;
ActiveInst.Entity = New;
@@ -5515,6 +5516,12 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
Function = const_cast<FunctionDecl*>(ExistingDefn);
}
+#ifndef NDEBUG
+ RecursiveInstGuard AlreadyInstantiating(*this, Function,
+ RecursiveInstGuard::Kind::Template);
+ assert(!AlreadyInstantiating && "should have been caught by caller");
+#endif
+
// Find the function body that we'll be substituting.
const FunctionDecl *PatternDecl = Function->getTemplateInstantiationPattern();
assert(PatternDecl && "instantiating a non-template");
@@ -5654,7 +5661,7 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
}
InstantiatingTemplate Inst(*this, PointOfInstantiation, Function);
- if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
+ if (Inst.isInvalid())
return;
PrettyDeclStackTraceEntry CrashInfo(Context, Function, SourceLocation(),
"instantiating function definition");
@@ -6223,6 +6230,11 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
if (TSK == TSK_ExplicitSpecialization)
return;
+ RecursiveInstGuard AlreadyInstantiating(*this, Var,
+ RecursiveInstGuard::Kind::Template);
+ if (AlreadyInstantiating)
+ return;
+
// Find the pattern and the arguments to substitute into it.
VarDecl *PatternDecl = Var->getTemplateInstantiationPattern();
assert(PatternDecl && "no pattern for templated variable");
@@ -6246,7 +6258,7 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
// FIXME: Factor out the duplicated instantiation context setup/tear down
// code here.
InstantiatingTemplate Inst(*this, PointOfInstantiation, Var);
- if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
+ if (Inst.isInvalid())
return;
PrettyDeclStackTraceEntry CrashInfo(Context, Var, SourceLocation(),
"instantiating variable initializer");
@@ -6350,7 +6362,7 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
}
InstantiatingTemplate Inst(*this, PointOfInstantiation, Var);
- if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
+ if (Inst.isInvalid())
return;
PrettyDeclStackTraceEntry CrashInfo(Context, Var, SourceLocation(),
"instantiating variable definition");
diff --git a/clang/test/SemaCXX/libstdcxx_pair_swap_hack.cpp b/clang/test/SemaCXX/libstdcxx_pair_swap_hack.cpp
index 6b8ca4f740914..a4775710e6d8c 100644
--- a/clang/test/SemaCXX/libstdcxx_pair_swap_hack.cpp
+++ b/clang/test/SemaCXX/libstdcxx_pair_swap_hack.cpp
@@ -82,13 +82,14 @@ namespace sad {
template<typename T> void swap(T &, T &);
template<typename A, typename B> struct CLASS {
- void swap(CLASS &other) noexcept(noexcept(swap(*this, other))); // expected-error {{too many arguments}} expected-note {{declared here}}
- // expected-error@-1{{uses itself}} expected-note@-1{{in instantiation of}}
+ void swap(CLASS &other) // expected-note {{declared here}}
+ noexcept(noexcept(swap(*this, other))); // expected-error {{too many arguments}}
+ // expected-error@-1{{uses itself}}
};
CLASS<int, int> pi;
- static_assert(!noexcept(pi.swap(pi)), ""); // expected-note 2{{in instantiation of exception specification for 'swap'}}
+ static_assert(!noexcept(pi.swap(pi)), ""); // expected-note {{in instantiation of exception specification for 'swap'}}
}
#endif
diff --git a/clang/test/SemaTemplate/instantiate-self.cpp b/clang/test/SemaTemplate/instantiate-self.cpp
index 4999a4ad3784d..a99a2ef30b479 100644
--- a/clang/test/SemaTemplate/instantiate-self.cpp
+++ b/clang/test/SemaTemplate/instantiate-self.cpp
@@ -86,7 +86,7 @@ namespace test7 {
namespace test8 {
template<typename T> struct A {
- int n = A{}.n; // expected-error {{default member initializer for 'n' uses itself}} expected-note {{instantiation of default member init}}
+ int n = A{}.n; // expected-error {{default member initializer for 'n' uses itself}}
};
A<int> ai = {}; // expected-note {{instantiation of default member init}}
}
@@ -100,7 +100,7 @@ namespace test9 {
namespace test10 {
template<typename T> struct A {
- void f() noexcept(noexcept(f())); // expected-error {{exception specification of 'f' uses itself}} expected-note {{instantiation of}}
+ void f() noexcept(noexcept(f())); // expected-error {{exception specification of 'f' uses itself}}
};
bool b = noexcept(A<int>().f()); // expected-note {{instantiation of}}
}
@@ -125,7 +125,7 @@ namespace test11 {
}
namespace test12 {
- template<typename T> int f(T t, int = f(T())) {} // expected-error {{recursive evaluation of default argument}} expected-note {{instantiation of}}
+ template<typename T> int f(T t, int = f(T())) {} // expected-error {{recursive evaluation of default argument}}
struct X {};
int q = f(X()); // expected-note {{instantiation of}}
}
|
erichkeane
approved these changes
Oct 7, 2025
This makes pushing / popping CodeSynthesisContexts much cheaper, as it delegates to another class this functionality which is not actually needed in most cases. It also converts a bunch of these uses into just asserts. This improves compiler performance a little bit: Some diagnostics change a little bit, because we avoid printing a redundant context notes.
mizvekov
force-pushed
the
users/mizvekov/refactor-recursive-inst-diag
branch
from
dcf5b6f to
77f9dfd
Compare
h-vetinari
reviewed
Oct 19, 2025
Comment on lines
+3546
to
+3564
| bool Sema::InstantiateClass(SourceLocation PointOfInstantiation, | ||
| CXXRecordDecl *Instantiation, | ||
| CXXRecordDecl *Pattern, | ||
| const MultiLevelTemplateArgumentList &TemplateArgs, | ||
| TemplateSpecializationKind TSK, bool Complain) { | ||
| #ifndef NDEBUG | ||
| RecursiveInstGuard AlreadyInstantiating(*this, Instantiation, | ||
| RecursiveInstGuard::Kind::Template); | ||
| assert(!AlreadyInstantiating && "should have been caught by caller"); | ||
| #endif | ||
|
|
||
| return InstantiateClassImpl(PointOfInstantiation, Instantiation, Pattern, | ||
| TemplateArgs, TSK, Complain); | ||
| } | ||
|
|
||
| bool Sema::InstantiateClassImpl( | ||
| SourceLocation PointOfInstantiation, CXXRecordDecl *Instantiation, | ||
| CXXRecordDecl *Pattern, const MultiLevelTemplateArgumentList &TemplateArgs, | ||
| TemplateSpecializationKind TSK, bool Complain) { |
There was a problem hiding this comment.
If the only difference between Sema::InstantiateClass and Sema::InstantiateClassImpl is a debug check that gets compiled out, is that really a strong enough reason to split the function? Or are there other plans to let these two diverge some more?
There was a problem hiding this comment.
I had other ideas to split more, but I don't have enough time to work on that at this point.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/17725 Here is the relevant piece of the build log for the reference |
mizvekov
added a commit
that referenced
this pull request
Oct 19, 2025
…esisContext" (#164174) Reverts #162224 Broke buildbot here: #162224 (comment)
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Oct 19, 2025
…m CodeSynthesisContext" (#164174) Reverts llvm/llvm-project#162224 Broke buildbot here: llvm/llvm-project#162224 (comment)
mizvekov
added a commit
that referenced
this pull request
Oct 19, 2025
…esisContext This makes pushing / popping CodeSynthesisContexts much cheaper, as it delegates to another class this functionality which is not actually needed in most cases. It also converts a bunch of these uses into just asserts. This improves compiler performance a little bit: Some diagnostics change a little bit, because we avoid printing a redundant context notes. This relands #162224 with no changes, turns out the buildbot failure was unrelated.
mizvekov
added a commit
that referenced
this pull request
Oct 19, 2025
…esisContext (#164177) This makes pushing / popping CodeSynthesisContexts much cheaper, as it delegates to another class this functionality which is not actually needed in most cases. It also converts a bunch of these uses into just asserts. This improves compiler performance a little bit: Some diagnostics change a little bit, because we avoid printing a redundant context notes. This relands #162224 with no changes, turns out the buildbot failure was unrelated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
This makes pushing / popping CodeSynthesisContexts much cheaper, as it delegates to another class this functionality which is not actually needed in most cases.
It also converts a bunch of these uses into just asserts.
This improves compiler performance a little bit:
Some diagnostics changes, because we avoid printing redundant context notes.