Skip to content

[Clang] Instantiate local constexpr functions eagerly #95660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 16, 2024

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jun 15, 2024

We had a code path in Sema::MarkFunctionReferenced() that deferred local lambda instantiation even for constexprs. This resulted in any calls to them referring to function decls that lack bodies and hence failures at constant evaluation.

The issue doesn't occur when the lambda has no explicit return type because the return type deduction requires instantiation.

Fixes #35052
Fixes #94849

@zyn0217 zyn0217 requested a review from cor3ntin June 15, 2024 12:55
@zyn0217 zyn0217 marked this pull request as ready for review June 15, 2024 13:28
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 15, 2024
@zyn0217 zyn0217 requested a review from erichkeane June 15, 2024 13:28
@llvmbot
Copy link
Member

llvmbot commented Jun 15, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

We had a code path in Sema::MarkFunctionReferenced() that deferred local lambda instantiation even for constexprs. This resulted in any calls to them referring to function decls that lack bodies and hence failures at constant evaluation.

The issue doesn't occur when the lambda has no explicit return type because the return type deduction requires instantiation.

Fixes #35052
Fixes #94849


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+2-1)
  • (modified) clang/test/SemaTemplate/instantiate-local-class.cpp (+24)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8c2f737836a9d..a424554ed821d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -847,6 +847,7 @@ Bug Fixes to C++ Support
 - Fixed several bugs in capturing variables within unevaluated contexts. (#GH63845), (#GH67260), (#GH69307),
   (#GH88081), (#GH89496), (#GH90669) and (#GH91633).
 - Fixed handling of brace ellison when building deduction guides. (#GH64625), (#GH83368).
+- Clang now instantiates local constexpr functions eagerly for constant evaluators. (#GH35052), (#GH94849)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 99a8704298314..88e9b2b00f84d 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -18112,7 +18112,8 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func,
 
         if (FirstInstantiation || TSK != TSK_ImplicitInstantiation ||
             Func->isConstexpr()) {
-          if (isa<CXXRecordDecl>(Func->getDeclContext()) &&
+          if (!Func->isConstexpr() &&
+              isa<CXXRecordDecl>(Func->getDeclContext()) &&
               cast<CXXRecordDecl>(Func->getDeclContext())->isLocalClass() &&
               CodeSynthesisContexts.size())
             PendingLocalImplicitInstantiations.push_back(
diff --git a/clang/test/SemaTemplate/instantiate-local-class.cpp b/clang/test/SemaTemplate/instantiate-local-class.cpp
index 47591045fd26e..7eee131e28d60 100644
--- a/clang/test/SemaTemplate/instantiate-local-class.cpp
+++ b/clang/test/SemaTemplate/instantiate-local-class.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -verify -std=c++11 %s
 // RUN: %clang_cc1 -verify -std=c++11 -fdelayed-template-parsing %s
+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
 
 template<typename T>
 void f0() {
@@ -509,3 +510,26 @@ namespace LambdaInDefaultMemberInitializer {
   }
   template void f<int>();
 }
+
+#if __cplusplus >= 201703L
+namespace GH35052 {
+
+template <typename F> constexpr int func(F f) {
+  if constexpr (f(1UL)) {
+    return 1;
+  }
+  return 0;
+}
+
+int main() {
+  auto predicate = [](auto v) /*implicit constexpr*/ -> bool {
+    return v == 1;
+  };
+
+  static_assert(predicate(1));
+  return func(predicate);
+}
+
+} // namespace GH35052
+
+#endif

@EricWF
Copy link
Member

EricWF commented Jun 15, 2024

This looks good to me, but I'm not comfortable officially LGTMing it.

Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zyn0217
Copy link
Contributor Author

zyn0217 commented Jun 16, 2024

Thanks for the review! I will commit when CI turns green.

@zyn0217
Copy link
Contributor Author

zyn0217 commented Jun 16, 2024

Linux CI failed with an unrelated error.

TIMEOUT: lldb-api :: functionalities/fork/concurrent_vfork/TestConcurrentVFork.py (87750 of 87750)
  | ******************** TEST 'lldb-api :: functionalities/fork/concurrent_vfork/TestConcurrentVFork.py' FAILED ********************

@zyn0217 zyn0217 merged commit 5548ea3 into llvm:main Jun 16, 2024
6 of 8 checks passed
@zyn0217 zyn0217 deleted the gh35052 branch June 16, 2024 13:54
zyn0217 added a commit to zyn0217/llvm-project that referenced this pull request Jul 16, 2024
…5660)"

Unfortunately, this has caused a regression in DeduceReturnType(), which
causes a problem in that some of the local recursive lambdas are incorrectly
rejected.

This reverts commit 5548ea3. Also,
this adds an offending case to the test.
zyn0217 added a commit that referenced this pull request Jul 16, 2024
…" (#98991)

Unfortunately, #95660 has caused a regression in DeduceReturnType(),
where some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this
adds an offending case to the test.

Closes #98526
ahatanaka pushed a commit to swiftlang/llvm-project that referenced this pull request Jul 18, 2024
…5660)" (llvm#98991)

Unfortunately, llvm#95660 has caused a regression in DeduceReturnType(),
where some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this
adds an offending case to the test.

Closes llvm#98526

(cherry picked from commit 862715e)
ahatanaka pushed a commit to swiftlang/llvm-project that referenced this pull request Jul 18, 2024
…5660)" (llvm#98991)

Unfortunately, llvm#95660 has caused a regression in DeduceReturnType(),
where some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this
adds an offending case to the test.

Closes llvm#98526

(cherry picked from commit 862715e)
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…" (#98991)

Summary:
Unfortunately, #95660 has caused a regression in DeduceReturnType(),
where some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this
adds an offending case to the test.

Closes #98526

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251570
@cor3ntin
Copy link
Contributor

cor3ntin commented Apr 2, 2025

@zyn0217 What is the status of that?

@zyn0217
Copy link
Contributor Author

zyn0217 commented Apr 2, 2025

It's still broken, and I haven’t figured out an approach to fix the regression.

I do recall that we were planning to wire up Sema and the constant evaluator, so this patch might end up being completely superseded if that gets implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
5 participants