[clang] Reset track of immediate function context when entering new function

[Fznamznon]

Summary

Due to not resetting that, clang still thinks that it is in immediate
function context even if it already entered non-consteval function.
This caused consteval functions reaching codegen in some cases.

Fixes https://github.com/llvm/llvm-project/issues/61142

[cor3ntin]

Thanks, for working on this
Congrats on finding the underlying issue, it must not have been easy!

clang/lib/Sema/SemaDecl.cpp

15182

I think it might be worth adding a comment there to explain why this is necessary.
I think i convinced myself this make sense and is almost certainly the correct fix but it would, I think, benefit from some more explanation for future reference.

clang/test/CodeGenCXX/cxx20-consteval-crash.cpp

138

Just to make sure, did you check that this fixes the other examples in the issue? I was afraid i reduced too much

[Fznamznon]

Add a comment, rebase, fix format

clang/lib/Sema/SemaDecl.cpp

15182

Sure, done. Hope it turned out helpful.

clang/test/CodeGenCXX/cxx20-consteval-crash.cpp

138

Yes, it helps to fix them. I checked even the original big one.

[cor3ntin]

I reworked the comment a bit otherwise LGTM.

clang/lib/Sema/SemaDecl.cpp

15183–15190

[aaron.ballman]

clang/docs/ReleaseNotes.rst

275–284

clang/test/CodeGenCXX/cxx20-consteval-crash.cpp

120

Would it make sense to add some CHECK lines to check the actual codegen is sensible?

[shafik]

clang/lib/Sema/SemaDecl.cpp

15191

I am a bit concerned that we are updating InImmediateFunctionContext outside of PushExpressionEvaluationContext. So we are now spreading the logic outside of where it was originally contained.

I am wondering if instead inside of PushExpressionEvaluationContext we could set InImmediateFunctionContext like so:

ExprEvalContexts.back().InImmediateFunctionContext =
      ExprEvalContexts[ExprEvalContexts.size() - 2]
          .isImmediateFunctionContext() || NewContext == ImmediateFunctionContext;

or something along those lines.

[cor3ntin]

clang/lib/Sema/SemaDecl.cpp

15191

Here isImmediateFunctionContext() will be true, we want it to be false

If we wanted to do somelink like what you are suggesting, we'd need something like a new EnteringFunctionDefinitionContext boolean parameter to PushExpressionEvaluationContext. so that
InImmediateFunctionContext is only set to the outer context value when EnteringFunctionDefinitionContextwould be false

Given that we should only get in that situation from ActOnStartOfFunctionDef, I'm not sure that would be a better design

[Fznamznon]

Fix grammar, add checks to the test

[Fznamznon]

clang/lib/Sema/SemaDecl.cpp

15191

Thanks, @cor3ntin .
This is the exact reason why I implemented the fix this way.

clang/test/CodeGenCXX/cxx20-consteval-crash.cpp

120

Well, the assertion was due to consteval function reaching codegen, so I added checks that it doesn't appear in the IR in any way.

[aaron.ballman]

LGTM, thank you for the fix!

clang/docs/ReleaseNotes.rst

- Correct restriction of trailing requirements clauses on a templated function.
   to limit non-defined/non-member functions as well. Additionally, we now diagnose
   requires on lambdas when not allowed, which we previously missed.
   (`#61748 <https://github.com/llvm/llvm-project/issues/61748>`_)
 - Fix confusing diagnostic for incorrect use of qualified concepts names.
 - Fix handling of comments in function like macros so they are ignored in -CC
   mode.
   (`#60887 <https://github.com/llvm/llvm-project/issues/60887>`_)
 - Fix incorrect merging of lambdas across modules.
   (`#60985 <https://github.com/llvm/llvm-project/issues/60985>`_)
 - Fix crash when handling nested immediate invocations in initializers of global
   variables.
   (`#58207 <https://github.com/llvm/llvm-project/issues/58207>`_)
 - Fix crash when generating code coverage information for `PseudoObjectExpr` in 
   Clang AST.
   (`#45481 <https://github.com/llvm/llvm-project/issues/45481>`_)
+- Fix the assertion hit when a template consteval function appears in a nested
+  consteval/constexpr call chain.
+  (`#61142 <https://github.com/llvm/llvm-project/issues/61142>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Sema/SemaDecl.cpp

Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
   if (!isLambdaCallOperator(FD))
     // [expr.const]/p14.1
     // An expression or conversion is in an immediate function context if it is
     // potentially evaluated and either: its innermost enclosing non-block scope
     // is a function parameter scope of an immediate function.
     PushExpressionEvaluationContext(
         FD->isConsteval() ? ExpressionEvaluationContext::ImmediateFunctionContext
                           : ExprEvalContexts.back().Context);
 
+  // Each ExpressionEvaluationContextRecord also keeps track of whether the
+  // context is nested in an immediate function context, so smaller contexts
+  // that appear inside immediate functions (like variable initializers) are
+  // considered to be inside an immediate function context even though by
+  // themselves they are not immediate function contexts. But when a new
+  // function is entered, we need to reset this tracking, since the entered
+  // function might be not an immediate function.
+  ExprEvalContexts.back().InImmediateFunctionContext = FD->isConsteval();
+
   // Check for defining attributes before the check for redefinition.
   if (const auto *Attr = FD->getAttr<AliasAttr>()) {
     Diag(Attr->getLocation(), diag::err_alias_is_definition) << FD << 0;
     FD->dropAttr<AliasAttr>();
     FD->setInvalidDecl();
   }
   if (const auto *Attr = FD->getAttr<IFuncAttr>()) {
     Diag(Attr->getLocation(), diag::err_alias_is_definition) << FD << 1;

clang/test/CodeGenCXX/cxx20-consteval-crash.cpp

void method() {
   // CHECK: %1 = getelementptr inbounds { ptr, ptr, ptr }, ptr %agg.tmp.ensured, i32 0, i32 1
   // CHECK: store ptr null, ptr %1, align 8
   // CHECK: %2 = getelementptr inbounds { ptr, ptr, ptr }, ptr %agg.tmp.ensured, i32 0, i32 2
   // CHECK: store ptr null, ptr %2, align 8
    (void)Derived();
 }
 
 } // namespace GH60166
+
+namespace GH61142 {
+
+template <typename T>
+struct Test {
+  constexpr static void bar() {
+    foo();
+  }
+  consteval static void foo() {};
+};
+
+consteval void a() {
+  Test<int>::bar();
+}
+
+void b() {
+  Test<int>::bar();
+}
+
+// Make sure consteval function is not emitted.
+// CHECK-NOT: call {{.*}}foo{{.*}}()
+// CHECK-NOT: define {{.*}}foo{{.*}}()
+
+} // namespace GH61142