Page MenuHomePhabricator

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

Authored by Izaron on Feb 12 2022, 3:07 PM.

Details

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

Diff Detail

Event Timeline

Izaron requested review of this revision.Feb 12 2022, 3:07 PM
Izaron created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2022, 3:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Izaron updated this revision to Diff 408220.Feb 12 2022, 3:10 PM

(Fast test comment 80 character line fix)

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.

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 ?

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 =(

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.

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 =(

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.

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 updated this revision to Diff 409035.Feb 15 2022, 1:19 PM

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 requested changes to this revision.Feb 16 2022, 5:54 AM

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.

This revision now requires changes to proceed.Feb 16 2022, 5:54 AM

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 added a subscriber: usaxena95.EditedAug 18 2022, 2:24 AM

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

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.

Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2022, 2:24 AM

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

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

Izaron abandoned this revision.Wed, Sep 14, 12:43 PM