[clang] Enforce instantiation of constexpr template functions during non-constexpr evaluation

[serge-sans-paille]

Summary

Otherwise these functions are not instantiated and we end up with an undefined
symbol.

Fix #55560

[serge-sans-paille]

I"m not 100% sure of the fix but it fixes bug #55560 and does not introduce regression :-/

[serge-sans-paille]

(rebased on main branch)

[serge-sans-paille]

This also seems to fix https://github.com/llvm/llvm-project/issues/37522 and https://github.com/llvm/llvm-project/issues/55315

[serge-sans-paille]

gentle ping o/

[aaron.ballman]

It's a bummer that patch application failed on precommit CI for reasons unrelated to your patch (as best I can tell, anyway)... Also, please update the patch summary to have some more details about what your changing and why.

clang/test/SemaCXX/constexpr-late-instantiation.cpp

2

3

This test should be in CodeGenCXX, not SemaCXX. But there is a SemaCXX test I'd appreciate:

template <typename T>
constexpr T foo(T a);

int main() {
  int k = foo<int>(5); // Ok
  constexpr int j = foo<int>(5); // Not okay, a definition is needed for this
}

template <typename T>
constexpr T foo(T a) {
  return a;
}

[erichkeane]

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

4859

This condition seems a little aggressive, and will end up instantiating way more than just constexprs here, right? I guess my real question is what it is about the constexpr that makes us decide to skip the instantiation earlier.

[shafik]

During the parsing of the assignment in main for the constexpr case we end up in Sema::MarkFunctionReferenced in particular this code:

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);

And so I experimented with modifying Sema::InstantiateFunctionDefinition in particular:

else if (TSK == TSK_ExplicitInstantiationDefinition) {

And modified in this way:

else if (TSK == TSK_ExplicitInstantiationDefinition || (Function->isConstexpr() && !Recursive)) {

This seems more targeted, reflects what I think is the intent and passes check-clang as well.

I also looked at the non-constexpr case:

template <typename T>
T foo(T a);

int main() {
  int k = foo<int>(5);
}

template <typename T>
T foo(T a) {
  return a;
}

And in this case we hit Sema::InstantiateFunctionDefinition from Sema::PerformPendingInstantiations which is called by Sema::ActOnEndOfTranslationUnitFragment and so by this time we have seen the definition already.

[aaron.ballman]

Pinging about updates to this, because if this really does fix https://github.com/llvm/llvm-project/issues/55560, I think we want to get that fix into Clang 15. The release branch point is coming up pretty soon (https://discourse.llvm.org/t/llvm-15-0-0-release-schedule/63495/10).

[serge-sans-paille]

@shafik: I've implemented your patch which indeed looks closer to the original intent
@aaron.ballman : test case and code updated.

[aaron.ballman]

Can you also add a release note for the fix?

clang/test/SemaCXX/constexpr-late-instantiation.cpp

3

10

[serge-sans-paille]

+ release note
+ take review into account

[aaron.ballman]

LGTM aside from some test changes, assuming that precommit CI comes back green.

clang/docs/ReleaseNotes.rst

184

clang/test/SemaCXX/constexpr-late-instantiation.cpp

2

You can remove this comment since this is no longer testing the IR.

3

Oops, I missed that in my earlier suggestion!

[serge-sans-paille]

++review

clang/docs/ReleaseNotes.rst

184

Thanks you for removing this french touch ;-)

[shafik]

LGTM, thank you!

clang/docs/ReleaseNotes.rst

- Clang will allow calling a ``consteval`` function in a default argument. This
   fixes `Issue 48230 <https://github.com/llvm/llvm-project/issues/48230>`_.
 - Fixed memory leak due to ``VarTemplateSpecializationDecl`` using
   ``TemplateArgumentListInfo`` instead of ``ASTTemplateArgumentListInfo``.
 - An initializer for a static variable declaration, which is nested
   inside a statement expression in an aggregate initializer, is now
   emitted as a dynamic initializer. Previously the variable would
   incorrectly be zero-initialized. In contexts where a dynamic
   initializer is not allowed this is now diagnosed as an error.
+- Clang now correctly emits symbols for implicitly instantiated constexpr
+  template function. Fixes `Issue 55560 <https://github.com/llvm/llvm-project/issues/55560>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
   // FIXME: We need to track the instantiation stack in order to know which
   // definitions should be visible within this instantiation.
   if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Function,
                                 Function->getInstantiatedFromMemberFunction(),
                                      PatternDecl, PatternDef, TSK,
                                      /*Complain*/DefinitionRequired)) {
     if (DefinitionRequired)
       Function->setInvalidDecl();
-    else if (TSK == TSK_ExplicitInstantiationDefinition) {
+    else if (TSK == TSK_ExplicitInstantiationDefinition ||
+             (Function->isConstexpr() && !Recursive)) {
       // Try again at the end of the translation unit (at which point a
       // definition will be required).
       assert(!Recursive);
       Function->setInstantiationIsPending(true);
       PendingInstantiations.push_back(
         std::make_pair(Function, PointOfInstantiation));
     } else if (TSK == TSK_ImplicitInstantiation) {
       if (AtEndOfTU && !getDiagnostics().hasErrorOccurred() &&
           !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc())) {
         Diag(PointOfInstantiation, diag::warn_func_template_missing)
           << Function;
         Diag(PatternDecl->getLocation(), diag::note_forward_template_decl);
         if (getLangOpts().CPlusPlus11)
           Diag(PointOfInstantiation, diag::note_inst_declaration_hint)
               << Function;
       }
     }
 
     return;
   }
 
   // Postpone late parsed template instantiations.
   if (PatternDecl->isLateTemplateParsed() &&

clang/test/CodeGenCXX/constexpr-late-instantiation.cpp

+// Make sure foo is instantiated and we don't get a link error
+// RUN: %clang_cc1 -S -emit-llvm -triple %itanium_abi_triple %s -o- | FileCheck %s
+
+template <typename T>
+constexpr T foo(T a);
+
+// CHECK-LABEL: define {{.*}} @main
+int main() {
+  // CHECK: call {{.*}} @_Z3fooIiET_S0_
+  int k = foo<int>(5);
+}
+// CHECK: }
+
+template <typename T>
+constexpr T foo(T a) {
+  return a;
+}

clang/test/SemaCXX/constexpr-late-instantiation.cpp

+// RUN: %clang_cc1 %s -fsyntax-only -verify
+
+template <typename T>
+constexpr T foo(T a);   // expected-note {{declared here}}
+
+int main() {
+  int k = foo<int>(5);  // Ok
+  constexpr int j =     // expected-error {{constexpr variable 'j' must be initialized by a constant expression}}
+          foo<int>(5);  // expected-note {{undefined function 'foo<int>' cannot be used in a constant expression}}
+}
+
+template <typename T>
+constexpr T foo(T a) {
+  return a;
+}