[Clang] Don't give up on an unsuccessful function instantiation #126723

zyn0217 · 2025-02-11T12:43:41Z

For constexpr function templates, we immediately instantiate them upon reference. However, if the function isn't defined at the time of instantiation, even though it might be defined later, the instantiation would forever fail.

This patch corrects the behavior by popping up failed instantiations through PendingInstantiations, so that we are able to instantiate them again in the future (e.g. at the end of TU.)

Fixes #125747

For constexpr function templates, we immediately instantiate them upon reference. However, if the function isn't defined at the point of instantiation, even though it might be defined later, the instantiation would simply fail. This patch corrects the behavior by popping up failed instantiations through PendingInstantiations, so that we are able to instantiate them again in the future (e.g. at the end of TU.)

zyn0217 · 2025-02-12T05:04:36Z

Compile time tracker: http://llvm-compile-time-tracker.com/compare.php?from=83a8bb363ad578da67a7cb568460a3871ce0ad8b&to=21ca76a13ca62715ce98fb2c8b0361df727769b0

I think the difference is acceptable?

llvmbot · 2025-02-12T05:07:36Z

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

For constexpr function templates, we immediately instantiate them upon reference. However, if the function isn't defined at the time of instantiation, even though it might be defined later, the instantiation would forever fail.

This patch corrects the behavior by popping up failed instantiations through PendingInstantiations, so that we are able to instantiate them again in the future (e.g. at the end of TU.)

Fixes #125747

Full diff: https://github.com/llvm/llvm-project/pull/126723.diff

4 Files Affected:

(modified) clang/include/clang/Sema/Sema.h (+16-7)
(modified) clang/lib/Interpreter/IncrementalParser.cpp (+3-2)
(modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+30-17)
(modified) clang/test/CodeGenCXX/function-template-specialization.cpp (+18-1)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 472a0e25adc97..2083a2c2ca40d 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13588,12 +13588,16 @@ class Sema final : public SemaBase {
 
   class LocalEagerInstantiationScope {
   public:
-    LocalEagerInstantiationScope(Sema &S) : S(S) {
+    LocalEagerInstantiationScope(Sema &S, bool AtEndOfTU)
+        : S(S), AtEndOfTU(AtEndOfTU) {
       SavedPendingLocalImplicitInstantiations.swap(
           S.PendingLocalImplicitInstantiations);
     }
 
-    void perform() { S.PerformPendingInstantiations(/*LocalOnly=*/true); }
+    void perform() {
+      S.PerformPendingInstantiations(/*LocalOnly=*/true,
+                                     /*AtEndOfTU=*/AtEndOfTU);
+    }
 
     ~LocalEagerInstantiationScope() {
       assert(S.PendingLocalImplicitInstantiations.empty() &&
@@ -13604,6 +13608,7 @@ class Sema final : public SemaBase {
 
   private:
     Sema &S;
+    bool AtEndOfTU;
     std::deque<PendingImplicitInstantiation>
         SavedPendingLocalImplicitInstantiations;
   };
@@ -13626,8 +13631,8 @@ class Sema final : public SemaBase {
 
   class GlobalEagerInstantiationScope {
   public:
-    GlobalEagerInstantiationScope(Sema &S, bool Enabled)
-        : S(S), Enabled(Enabled) {
+    GlobalEagerInstantiationScope(Sema &S, bool Enabled, bool AtEndOfTU)
+        : S(S), Enabled(Enabled), AtEndOfTU(AtEndOfTU) {
       if (!Enabled)
         return;
 
@@ -13641,7 +13646,8 @@ class Sema final : public SemaBase {
     void perform() {
       if (Enabled) {
         S.DefineUsedVTables();
-        S.PerformPendingInstantiations();
+        S.PerformPendingInstantiations(/*LocalOnly=*/false,
+                                       /*AtEndOfTU=*/AtEndOfTU);
       }
     }
 
@@ -13656,7 +13662,8 @@ class Sema final : public SemaBase {
       S.SavedVTableUses.pop_back();
 
       // Restore the set of pending implicit instantiations.
-      if (S.TUKind != TU_Prefix || !S.LangOpts.PCHInstantiateTemplates) {
+      if ((S.TUKind != TU_Prefix || !S.LangOpts.PCHInstantiateTemplates) &&
+          AtEndOfTU) {
         assert(S.PendingInstantiations.empty() &&
                "PendingInstantiations should be empty before it is discarded.");
         S.PendingInstantiations.swap(S.SavedPendingInstantiations.back());
@@ -13675,6 +13682,7 @@ class Sema final : public SemaBase {
   private:
     Sema &S;
     bool Enabled;
+    bool AtEndOfTU;
   };
 
   ExplicitSpecifier instantiateExplicitSpecifier(
@@ -13860,7 +13868,8 @@ class Sema final : public SemaBase {
 
   /// Performs template instantiation for all implicit template
   /// instantiations we have seen until this point.
-  void PerformPendingInstantiations(bool LocalOnly = false);
+  void PerformPendingInstantiations(bool LocalOnly = false,
+                                    bool AtEndOfTU = true);
 
   TemplateParameterList *
   SubstTemplateParams(TemplateParameterList *Params, DeclContext *Owner,
diff --git a/clang/lib/Interpreter/IncrementalParser.cpp b/clang/lib/Interpreter/IncrementalParser.cpp
index e43cea1baf43a..41d6304bd5f65 100644
--- a/clang/lib/Interpreter/IncrementalParser.cpp
+++ b/clang/lib/Interpreter/IncrementalParser.cpp
@@ -41,8 +41,9 @@ llvm::Expected<TranslationUnitDecl *>
 IncrementalParser::ParseOrWrapTopLevelDecl() {
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<Sema> CleanupSema(&S);
-  Sema::GlobalEagerInstantiationScope GlobalInstantiations(S, /*Enabled=*/true);
-  Sema::LocalEagerInstantiationScope LocalInstantiations(S);
+  Sema::GlobalEagerInstantiationScope GlobalInstantiations(S, /*Enabled=*/true,
+                                                           /*AtEndOfTU=*/true);
+  Sema::LocalEagerInstantiationScope LocalInstantiations(S, /*AtEndOfTU=*/true);
 
   // Add a new PTU.
   ASTContext &C = S.getASTContext();
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index d530ed0847ae8..13d157e2a48d1 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2139,7 +2139,8 @@ Decl *TemplateDeclInstantiator::VisitCXXRecordDecl(CXXRecordDecl *D) {
   // DR1484 clarifies that the members of a local class are instantiated as part
   // of the instantiation of their enclosing entity.
   if (D->isCompleteDefinition() && D->isLocalClass()) {
-    Sema::LocalEagerInstantiationScope LocalInstantiations(SemaRef);
+    Sema::LocalEagerInstantiationScope LocalInstantiations(SemaRef,
+                                                           /*AtEndOfTU=*/false);
 
     SemaRef.InstantiateClass(D->getLocation(), Record, D, TemplateArgs,
                              TSK_ImplicitInstantiation,
@@ -5121,8 +5122,10 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
   // This has to happen before LateTemplateParser below is called, so that
   // it marks vtables used in late parsed templates as used.
   GlobalEagerInstantiationScope GlobalInstantiations(*this,
-                                                     /*Enabled=*/Recursive);
-  LocalEagerInstantiationScope LocalInstantiations(*this);
+                                                     /*Enabled=*/Recursive,
+                                                     /*AtEndOfTU=*/AtEndOfTU);
+  LocalEagerInstantiationScope LocalInstantiations(*this,
+                                                   /*AtEndOfTU=*/AtEndOfTU);
 
   // Call the LateTemplateParser callback if there is a need to late parse
   // a templated function definition.
@@ -5691,10 +5694,12 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
       // If we're performing recursive template instantiation, create our own
       // queue of pending implicit instantiations that we will instantiate
       // later, while we're still within our own instantiation context.
-      GlobalEagerInstantiationScope GlobalInstantiations(*this,
-                                                         /*Enabled=*/Recursive);
+      GlobalEagerInstantiationScope GlobalInstantiations(
+          *this,
+          /*Enabled=*/Recursive, /*AtEndOfTU=*/AtEndOfTU);
       LocalInstantiationScope Local(*this);
-      LocalEagerInstantiationScope LocalInstantiations(*this);
+      LocalEagerInstantiationScope LocalInstantiations(*this,
+                                                       /*AtEndOfTU=*/AtEndOfTU);
 
       // Enter the scope of this instantiation. We don't use
       // PushDeclContext because we don't have a scope.
@@ -5791,14 +5796,16 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
   // queue of pending implicit instantiations that we will instantiate later,
   // while we're still within our own instantiation context.
   GlobalEagerInstantiationScope GlobalInstantiations(*this,
-                                                     /*Enabled=*/Recursive);
+                                                     /*Enabled=*/Recursive,
+                                                     /*AtEndOfTU=*/AtEndOfTU);
 
   // Enter the scope of this instantiation. We don't use
   // PushDeclContext because we don't have a scope.
   ContextRAII PreviousContext(*this, Var->getDeclContext());
   LocalInstantiationScope Local(*this);
 
-  LocalEagerInstantiationScope LocalInstantiations(*this);
+  LocalEagerInstantiationScope LocalInstantiations(*this,
+                                                   /*AtEndOfTU=*/AtEndOfTU);
 
   VarDecl *OldVar = Var;
   if (Def->isStaticDataMember() && !Def->isOutOfLine()) {
@@ -6548,18 +6555,20 @@ NamedDecl *Sema::FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D,
   return D;
 }
 
-void Sema::PerformPendingInstantiations(bool LocalOnly) {
-  std::deque<PendingImplicitInstantiation> delayedPCHInstantiations;
+void Sema::PerformPendingInstantiations(bool LocalOnly, bool AtEndOfTU) {
+  std::deque<PendingImplicitInstantiation> DelayedImplicitInstantiations;
   while (!PendingLocalImplicitInstantiations.empty() ||
          (!LocalOnly && !PendingInstantiations.empty())) {
     PendingImplicitInstantiation Inst;
 
+    bool LocalInstantiation = false;
     if (PendingLocalImplicitInstantiations.empty()) {
       Inst = PendingInstantiations.front();
       PendingInstantiations.pop_front();
     } else {
       Inst = PendingLocalImplicitInstantiations.front();
       PendingLocalImplicitInstantiations.pop_front();
+      LocalInstantiation = true;
     }
 
     // Instantiate function definitions
@@ -6568,22 +6577,26 @@ void Sema::PerformPendingInstantiations(bool LocalOnly) {
                                 TSK_ExplicitInstantiationDefinition;
       if (Function->isMultiVersion()) {
         getASTContext().forEachMultiversionedFunctionVersion(
-            Function, [this, Inst, DefinitionRequired](FunctionDecl *CurFD) {
+            Function,
+            [this, Inst, DefinitionRequired, AtEndOfTU](FunctionDecl *CurFD) {
               InstantiateFunctionDefinition(/*FIXME:*/ Inst.second, CurFD, true,
-                                            DefinitionRequired, true);
+                                            DefinitionRequired, AtEndOfTU);
               if (CurFD->isDefined())
                 CurFD->setInstantiationIsPending(false);
             });
       } else {
         InstantiateFunctionDefinition(/*FIXME:*/ Inst.second, Function, true,
-                                      DefinitionRequired, true);
+                                      DefinitionRequired, AtEndOfTU);
         if (Function->isDefined())
           Function->setInstantiationIsPending(false);
       }
       // Definition of a PCH-ed template declaration may be available only in the TU.
       if (!LocalOnly && LangOpts.PCHInstantiateTemplates &&
           TUKind == TU_Prefix && Function->instantiationIsPending())
-        delayedPCHInstantiations.push_back(Inst);
+        DelayedImplicitInstantiations.push_back(Inst);
+      else if (!AtEndOfTU && Function->instantiationIsPending() &&
+               !LocalInstantiation)
+        DelayedImplicitInstantiations.push_back(Inst);
       continue;
     }
 
@@ -6627,11 +6640,11 @@ void Sema::PerformPendingInstantiations(bool LocalOnly) {
     // Instantiate static data member definitions or variable template
     // specializations.
     InstantiateVariableDefinition(/*FIXME:*/ Inst.second, Var, true,
-                                  DefinitionRequired, true);
+                                  DefinitionRequired, AtEndOfTU);
   }
 
-  if (!LocalOnly && LangOpts.PCHInstantiateTemplates)
-    PendingInstantiations.swap(delayedPCHInstantiations);
+  if (!DelayedImplicitInstantiations.empty())
+    PendingInstantiations.swap(DelayedImplicitInstantiations);
 }
 
 void Sema::PerformDependentDiagnostics(const DeclContext *Pattern,
diff --git a/clang/test/CodeGenCXX/function-template-specialization.cpp b/clang/test/CodeGenCXX/function-template-specialization.cpp
index 7728f3dc74624..31c78358d014c 100644
--- a/clang/test/CodeGenCXX/function-template-specialization.cpp
+++ b/clang/test/CodeGenCXX/function-template-specialization.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple %s -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -Wundefined-func-template -verify -triple %itanium_abi_triple %s -o - | FileCheck %s
+// expected-no-diagnostics
 
 // CHECK-DAG: _ZZN7PR219047GetDataIiEERKibE1i = internal global i32 4
 // CHECK-DAG: _ZZN7PR219047GetDataIiEERKibE1i_0 = internal global i32 2
@@ -43,3 +44,19 @@ const int &GetData<int>(bool b) {
   return i;
 }
 }
+
+namespace GH125747 {
+
+template<typename F> constexpr int visit(F f) { return f(0); }
+    
+template <class T> int G(T t);
+    
+int main() { return visit([](auto s) -> int { return G(s); }); }
+    
+template <class T> int G(T t) {
+  return 0;
+}
+
+// CHECK: define {{.*}} @_ZN8GH1257471GIiEEiT_
+
+}

zyn0217 · 2025-02-18T14:24:10Z

Ping

cor3ntin

This is missing a changelog entry.

I wonder if AtEndOfTU should be a bool in Sema instead (that would replace all existing uses of AtEndOfTU - it would simplify this patch and a few other functions. I don't think we need to do it in this patch though.

The general direction looks good

mizvekov

Sorry that took a while, I am still coming back from WG21, so I can't look into this throughly yet.

I would look into / want to know about the very specific requirements on hitting the bug, described in the bug report, such as the lambda vs struct thing.

mizvekov · 2025-02-19T10:14:53Z

@zyn0217 did you look into, and if so, would you mind including explanations on this part of the issue report:

"Curiously, this bug seems to require a lambda -- if the lambda is replaced by an equivalent struct, the bug disappears. Also, a direct call to the lambda does not trigger the bug; it must be indirectly called via another constexpr function."

If you didn't look into it, it might be worth as otherwise there could be a simpler fix.

zyn0217 · 2025-02-19T10:28:05Z

I looked into that weeks ago, and the reason I remembered was kind of subtle. I'll explain why the fix is necessary later - I'll need a debugger to help me recall that.

zyn0217 · 2025-02-19T12:06:16Z

@mizvekov
The nuance is right here, where we tackle local lambdas and member functions separately:

llvm-project/clang/lib/Sema/SemaExpr.cpp

Lines 18273 to 18282 in 404f94a

    
           if (isa<CXXRecordDecl>(Func->getDeclContext()) && 
        
               cast<CXXRecordDecl>(Func->getDeclContext())->isLocalClass() && 
        
               CodeSynthesisContexts.size()) 
        
             PendingLocalImplicitInstantiations.push_back( 
        
                 std::make_pair(Func, PointOfInstantiation)); 
        
           else if (Func->isConstexpr()) 
        
             // Do not defer instantiations of constexpr functions, to avoid the 
        
             // expression evaluator needing to call back into Sema if it sees a 
        
             // call to such a function. 
        
             InstantiateFunctionDefinition(PointOfInstantiation, Func);

template<typename F> constexpr int visit(F f) { return f(0); }

template <class T> int G(T t);

int main() { return visit([](auto s) -> int { return G(s); }); }

template <class T> int G(T t) {
  return 0;
}

So basically, we perform the instantiation as follows.

The lambda case:

We instantiate visit once we call it because it's constexpr, with a LocalEagerInstantiationScope (L1) and GlobalEagerInstantiationScope (G1)
G1 is disabled because it's called from MarkFunctionReferenced()
We need to instantiate the lambda expression
As the lambda expression is defined locally, we fall into the first branch so that the lambda expression is pushed into L1.
We try to instantiate anything inside L1 after the instantiation of visit, creating a LocalEagerInstantiationScope (L2) and GlobalEagerInstantiationScope (G2).
G2 is enabled because InstantiateFunctionDefinition called from PerformPendingInstantiations goes with Recursive=true
We meet G(s), and it isn't constexpr, so we defer its instantiation and track it in GlobalEagerInstantiationScope, which is G2.
When we finished the lambda body, we try to instantiate G in G2, which fails because we don't have a definition at the moment.
We drop it forever because we assume we're at end of the TU (which isn't the case)

The member function case

We instantiate visit with a LocalEagerInstantiationScope (L1) and GlobalEagerInstantiationScope (G1), with G1 being disabled.
We need to instantiate the member function operator()
As it is a non-local constexpr function, we instantiate it immediately from the second branch I referenced, which creates LocalEagerInstantiationScope (L2) and GlobalEagerInstantiationScope (G2)
G2 is disabled because Recursive=false which is called from MarkFunctionReferenced()
We return back until we reach the end of TU, where we see the body of G, and we instantiate it happily.

Technically swapping the first and second branches I mentioned above should work, but the last time I tried that it caused a regression and was reverted."

erichkeane

I want @mizvekov to do a run through this as well (as he promised), but from my review, this looks fine.

mizvekov · 2025-02-19T18:30:24Z

Technically swapping the first and second branches I mentioned above should work, but the last time I tried that it caused a regression and was reverted."

Yeah, swapping the branches would have been a much simpler fix.

What was the regression? Do we have test case for it anywhere?

zyn0217 · 2025-02-19T23:49:32Z

Technically swapping the first and second branches I mentioned above should work, but the last time I tried that it caused a regression and was reverted."

Yeah, swapping the branches would have been a much simpler fix.

What was the regression? Do we have test case for it anywhere?

#98991

zyn0217 · 2025-03-04T12:56:32Z

Kindly ping @mizvekov in case you have more thoughts

mizvekov

Yeah, I think this is fine, LGTM.

I would have followed up on the original fix. I think I fixed some of these recursive local lambda bugs in my lambda contextdecl patch, but this might be unrelated otherwise, but still there might be an underlying issue here.

Otherwise, I appreciate this might be too much to follow up for this issue, and the current patch looks good.

…#126723) For constexpr function templates, we immediately instantiate them upon reference. However, if the function isn't defined at the time of instantiation, even though it might be defined later, the instantiation would forever fail. This patch corrects the behavior by popping up failed instantiations through PendingInstantiations, so that we are able to instantiate them again in the future (e.g. at the end of TU.) Fixes llvm#125747

zyn0217 added 2 commits February 11, 2025 20:39

Fix windows CI

242d1d5

zyn0217 requested review from zygoloid, mizvekov, cor3ntin and erichkeane February 12, 2025 05:06

zyn0217 marked this pull request as ready for review February 12, 2025 05:06

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 12, 2025

cor3ntin reviewed Feb 18, 2025

View reviewed changes

mizvekov reviewed Feb 18, 2025

View reviewed changes

erichkeane approved these changes Feb 19, 2025

View reviewed changes

mizvekov approved these changes Mar 4, 2025

View reviewed changes

zyn0217 added 2 commits March 5, 2025 11:42

Merge branch 'main' into GH125747

28675ed

Add a release note

3dd0537

zyn0217 merged commit c1468e9 into llvm:main Mar 5, 2025
7 of 11 checks passed

zyn0217 mentioned this pull request Mar 21, 2025

[clang] Linker failure for std::visit with a templated lambda using a forward declared templated function #115731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Don't give up on an unsuccessful function instantiation #126723

[Clang] Don't give up on an unsuccessful function instantiation #126723

Uh oh!

zyn0217 commented Feb 11, 2025 •

edited

Loading

Uh oh!

zyn0217 commented Feb 12, 2025

Uh oh!

llvmbot commented Feb 12, 2025

Uh oh!

zyn0217 commented Feb 18, 2025

Uh oh!

cor3ntin left a comment

Uh oh!

mizvekov left a comment

Uh oh!

mizvekov commented Feb 19, 2025

Uh oh!

zyn0217 commented Feb 19, 2025

Uh oh!

zyn0217 commented Feb 19, 2025 •

edited

Loading

Uh oh!

erichkeane left a comment

Uh oh!

mizvekov commented Feb 19, 2025

Uh oh!

zyn0217 commented Feb 19, 2025

Uh oh!

zyn0217 commented Mar 4, 2025

Uh oh!

mizvekov left a comment

Uh oh!

Uh oh!

Uh oh!

[Clang] Don't give up on an unsuccessful function instantiation #126723

[Clang] Don't give up on an unsuccessful function instantiation #126723

Uh oh!

Conversation

zyn0217 commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyn0217 commented Feb 12, 2025

Uh oh!

llvmbot commented Feb 12, 2025

Uh oh!

zyn0217 commented Feb 18, 2025

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

mizvekov left a comment

Choose a reason for hiding this comment

Uh oh!

mizvekov commented Feb 19, 2025

Uh oh!

zyn0217 commented Feb 19, 2025

Uh oh!

zyn0217 commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

mizvekov commented Feb 19, 2025

Uh oh!

zyn0217 commented Feb 19, 2025

Uh oh!

zyn0217 commented Mar 4, 2025

Uh oh!

mizvekov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zyn0217 commented Feb 11, 2025 •

edited

Loading

zyn0217 commented Feb 19, 2025 •

edited

Loading