[clang] Fix evaluation context type for consteval function calls in instantiations

[Izaron]

Summary

When instantiating a VarDecl initializing sub-AST, the evaluation
context was only PotentiallyEvaluated, even if we are inside a consteval
function. It dictated clang to construct a redundant ConstantExpr node, which
could be not evaluable.

Fixes https://github.com/llvm/llvm-project/issues/51182
and https://github.com/llvm/llvm-project/issues/52648

[Izaron]

(Fast test comment 80 character line fix)

[erichkeane]

I'm pretty sure @cor3ntin just messed with this quite a bit, so I'd like to hear his thoughts on this. But this generally looks right on first blush.

[cor3ntin]

In D119651#3317346, @erichkeane wrote:

I'm pretty sure @cor3ntin just messed with this quite a bit, so I'd like to hear his thoughts on this. But this generally looks right on first blush.

Probably not as much as you haha.
There seems to be quite a number of consteval related issues still - https://reviews.llvm.org/D113859 is very similar - yet completely unrelated -

This patch does look okay to me, in so far as it fixes this issue, in a way that seems sensible to me. I'm just wondering if there are similar issues in other places...should we survey all the evaluation contexts to see see if that can occur in other places?
Should we maybe always treat PotentiallyEvaluated as ConstantEvaluated in constant evaluated contexts? could that work ?

[Izaron]

Should we maybe always treat PotentiallyEvaluated as ConstantEvaluated in constant evaluated contexts? could that work ?

Indeed, within this patch we can prevent similars bugs to appear. I researched other places where a new context is pushed, and haven't find other bugs, but nevertheless.
In my subjective opinion, the architecture of ExprEvalContext is pretty fragile... We could add an assert before this line, ensuring that we don't push a (runtime) evaluated context after an immediate context. Or should we just don't push the new context in this case?... I wonder what is better, can't say right now =(

In D119651#3317458, @cor3ntin wrote:

There seems to be quite a number of consteval related issues still - https://reviews.llvm.org/D113859 is very similar - yet completely unrelated -

This patch does look okay to me, in so far as it fixes this issue, in a way that seems sensible to me. I'm just wondering if there are similar issues in other places...

BTW after looking at consteval-related issues on github, I've made four bite-sized patches. The issues are indeed completely unrelated to each other and do not have common source of errors.

https://reviews.llvm.org/D119095 Extra constructor call - a fix in RemoveNestedImmediateInvocation.
https://reviews.llvm.org/D119375 Trying to evaluate value-dependent ConstantExpr - a fix in CheckForImmediateInvocation (approved)
https://reviews.llvm.org/D119646 Trying to mark declarations as "referenced" inside a ConstantExpr in default arguments - a fix in the custom def. arg. AST visitor.
https://reviews.llvm.org/D119651 (This patch) a PotentiallyEvaluated context is created within a ConstantEvaluated context - remove where we do this.

As far as I see, the consteval bugs rarely have common source... They mostly require ad-hoc solutions.

[erichkeane]

In D119651#3317619, @Izaron wrote:

Should we maybe always treat PotentiallyEvaluated as ConstantEvaluated in constant evaluated contexts? could that work ?

Indeed, within this patch we can prevent similars bugs to appear. I researched other places where a new context is pushed, and haven't find other bugs, but nevertheless.
In my subjective opinion, the architecture of ExprEvalContext is pretty fragile... We could add an assert before this line, ensuring that we don't push a (runtime) evaluated context after an immediate context. Or should we just don't push the new context in this case?... I wonder what is better, can't say right now =(

In D119651#3317458, @cor3ntin wrote:

There seems to be quite a number of consteval related issues still - https://reviews.llvm.org/D113859 is very similar - yet completely unrelated -

This patch does look okay to me, in so far as it fixes this issue, in a way that seems sensible to me. I'm just wondering if there are similar issues in other places...

BTW after looking at consteval-related issues on github, I've made four bite-sized patches. The issues are indeed completely unrelated to each other and do not have common source of errors.

https://reviews.llvm.org/D119095 Extra constructor call - a fix in RemoveNestedImmediateInvocation.
https://reviews.llvm.org/D119375 Trying to evaluate value-dependent ConstantExpr - a fix in CheckForImmediateInvocation (approved)
https://reviews.llvm.org/D119646 Trying to mark declarations as "referenced" inside a ConstantExpr in default arguments - a fix in the custom def. arg. AST visitor.
https://reviews.llvm.org/D119651 (This patch) a PotentiallyEvaluated context is created within a ConstantEvaluated context - remove where we do this.

As far as I see, the consteval bugs rarely have common source... They mostly require ad-hoc solutions.

I like the idea of the assert, can we do it as a part of this? That way when we run into it it becomes more obvious/linked to this discussion.

[cor3ntin]

I like the idea of the assert, can we do it as a part of this? That way when we run into it it becomes more obvious/linked to this discussion.

I think that's a good idea too

[Izaron]

I've added an assert that will prevent similar bugs. Let's look if we're good with this one?
Here is the list of eval contexts:
https://github.com/llvm/llvm-project/blob/87b218b42b14e392aa0363a413d440b77bf04bd4/clang/include/clang/Sema/Sema.h#L1191-L1239
Should we also check for PotentiallyEvaluatedIfUsed or it is too much?
Looks like only two eval contexts from the list may be evaluated in run-time.

[erichkeane]

Actually, based on the build bot pre-merge checks, it looks like something during self-build has hit your assert!

https://buildkite.com/llvm-project/premerge-checks/builds/79589#154a5e12-3ef1-4c5b-b644-0f0d5e3e4383

Please see if you can figure out how to deal with whatever situation is happening there.

FAILED: tools/clang/lib/Tooling/ASTNodeAPI.json /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/lib/Tooling/ASTNodeAPI.json
cd /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/lib/Tooling && /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang-ast-dump --skip-processing=0 -I /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -I /var/lib/buildkite-agent/builds/llvm-project/clang/include -I /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/include -I /var/lib/buildkite-agent/builds/llvm-project/build/include -I /var/lib/buildkite-agent/builds/llvm-project/llvm/include -I /usr/include/c++/11 -I /usr/include/x86_64-linux-gnu/c++/11 -I /usr/include/c++/11/backward -I /usr/lib/llvm-13/lib/clang/13.0.1/include -I /usr/local/include -I /usr/include/x86_64-linux-gnu -I /usr/include --json-output-path /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/lib/Tooling/ASTNodeAPI.json
clang-ast-dump: /var/lib/buildkite-agent/builds/llvm-project/clang/lib/Sema/SemaExpr.cpp:16644: void clang::Sema::PushExpressionEvaluationContext(clang::Sema::ExpressionEvaluationContext, clang::Decl *, ExpressionEvaluationContextRecord::ExpressionKind): Assertion `!(ExprEvalContexts.back().Context == ExpressionEvaluationContext::PotentiallyEvaluated && ExprEvalContexts[ExprEvalContexts.size() - 2].isConstantEvaluated()) && "Can't have an evaluated context within an unevaluated context!"' failed.

[Izaron]

After an investigation in GDB I can say that the assert seems to be wrong. Since Clang instantiates classes and functions "on the fly" where appropriate, we indeed can get a run-time evaluation context after a compile-time evaluation context. I was sure that evaluation contexts were made to represent a clean hierarchy of context, because they're interrelated, but the case with instantiations confuses me.

This code ...

template<int N>
struct good_struct {
    // we are in run-time eval context!
    static consteval int evalconst() {
        // we are in compile-time eval context!
        return N * N;
    }

    void foo();
    void bar();
};

//int good_struct_100 = good_struct<100>::evalconst();
//int good_struct_200 = good_struct<200>::evalconst();

consteval int consteval_foo() {
    // we are in compile-time eval context!
    return good_struct<100>::evalconst();
}

template</* we are in compile-time eval context! */ int N = good_struct<200>::evalconst()>
constexpr int templated_foo() {
    return N;
}

... hits the assert two times, unless we uncomment the lines with good_struct_100 and good_struct_200. That's because Clang instantiates the classes when it "sees" them, straight from consteval/template contexts. I couldn't come up with a correct code that breaks though.

I am now less sure if the patch (without the assert) is acceptable, what if the concept of "evaluation contexts" needs a revision?..

[usaxena95]

Hi, I have been trying to get consteval in a better shape.

In D119651#3327212, @Izaron wrote:

After an investigation in GDB I can say that the assert seems to be wrong. Since Clang instantiates classes and functions "on the fly" where appropriate, we indeed can get a run-time evaluation context after a compile-time evaluation context. I was sure that evaluation contexts were made to represent a clean hierarchy of context, because they're interrelated, but the case with instantiations confuses me.

I can see the assertion still fails at the head for the mentioned case. Happy to lend a hand if this is not on your radar anymore.

[usaxena95]

FYI: Alternative fix for this issue: https://reviews.llvm.org/D132659

[Izaron]

@usaxena95, thanks! It was a really tough issue =)

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

void Sema::InstantiateVariableInitializer(
   // would otherwise imply that the variable is a definition for a
   // non-static data member.
   if (OldVar->isInlineSpecified())
     Var->setInlineSpecified();
   else if (OldVar->isInline())
     Var->setImplicitlyInline();
 
   if (OldVar->getInit()) {
-    EnterExpressionEvaluationContext Evaluated(
-        *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, Var);
+    ExpressionEvaluationContext Context =
+        ExpressionEvaluationContext::PotentiallyEvaluated;
+    // If current context is constant evaluated, the variable initializer
+    // context is also constant evaluated
+    if (isConstantEvaluated())
+      Context = ExpressionEvaluationContext::ConstantEvaluated;
+    EnterExpressionEvaluationContext Evaluated(*this, Context, Var);
 
     // Instantiate the initializer.
     ExprResult Init;
 
     {
       ContextRAII SwitchContext(*this, Var->getDeclContext());
       Init = SubstInitializer(OldVar->getInit(), TemplateArgs,
                               OldVar->getInitStyle() == VarDecl::CallInit);

clang/test/SemaCXX/cxx2a-consteval.cpp

 auto e = g(f()); // expected-error {{is not a constant expression}}
                  // expected-note@-1 {{non-constexpr function 'f' cannot be used in a constant expression}}
 
 using T = decltype(g(f()));
 static_assert(is_same<long, T>::value);
 
 } // namespace unevaluated
 
+namespace templated {
+
+consteval int f(int v) {
+  return v;
+}
+
+template <typename T>
+consteval int g(T a) {
+  // Previously this call was rejected due to incorrect evaluation context
+  // type in instantiations. Now we show that this call is OK.
+  int n = f(a);
+  return n;
+}
+
+static_assert(g(100) == 100);
+
+} // namespace templated
+
 namespace PR50779 {
 struct derp {
   int b = 0;
 };
 
 constexpr derp d;
 
 struct test {