Page MenuHomePhabricator

[clang] fix consteval call in default arguments
Needs ReviewPublic

Authored by Tyker on Feb 6 2020, 7:49 AM.

Details

Reviewers
rsmith
Summary

I spotted some issues with consteval call in some contexts.

consteval int f1() { return 0; }
consteval auto g() { return f1; }

consteval int h(int (*p)() = g()) { return p(); } // currently generate an error
constexpr auto e = g(); // currently generate 2 errors instead of 1.
auto l2 = [](int (*p)() = g()) consteval { return p(); }; // currently generate an error

this patch fixes these issues

Diff Detail

Event Timeline

Tyker updated this revision to Diff 243435.Feb 9 2020, 12:02 AM
Tyker updated this revision to Diff 298344.Thu, Oct 15, 3:53 AM
Tyker retitled this revision from [clang] fix consteval call in default arguements to [clang] fix consteval call in default arguements.

rebased and add more fixes

Herald added a project: Restricted Project. · View Herald TranscriptThu, Oct 15, 3:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
keryell added inline comments.
clang/lib/Sema/SemaExpr.cpp
16336
keryell retitled this revision from [clang] fix consteval call in default arguements to [clang] fix consteval call in default arguments.Thu, Oct 15, 2:54 PM
rsmith added inline comments.Thu, Oct 15, 4:11 PM
clang/include/clang/Parse/Parser.h
2928
clang/include/clang/Sema/Sema.h
1145
1147
4683

Can you model this as an ExpressionEvaluationContextRecord::ExpressionKind instead of a new parameter?

clang/lib/Sema/Sema.cpp
1026

What do we need this for? If I'm understanding the patch correctly, I think the only way we should propagate immediate invocations upwards should be from the ExpressionEvaluationContext of a lambda default argument into the ExpressionEvaluationContext of the lambda itself, so it should never reach the top level.

clang/lib/Sema/SemaDeclCXX.cpp
17045–17048 ↗(On Diff #298344)

This needs a comment explaining what's going on: why are constexpr / constinit variables special here, but (for example) a variable of type const int is not?

Actually, I think this change might not be correct. Consider the following:

consteval void check(bool b) { if (!b) throw; }
constinit int x = true ? 1 : (check(false), 2);

This code is ill-formed: the immediate invocation of check(false) is not a constant expression. But I think with this code change, we won't require check(false) to be a constant expression, instead deferring to checking whether the initializer of x as a whole is a constant expression (which it is!). So we'll accept invalid code.

17062 ↗(On Diff #298344)

Do we need to restore the old value here, rather than resetting to false? In a case like:

consteval int g() { return 0; }
constexpr int a = ({ constexpr int b = 0; g(); });

it looks like we'd lose the 'override' flag at the end of the definition of b.

Generally I find the idea of using a global override flag like this to be dubious: there are a lot of ways in which we switch context during Sema, and we'd need to make sure we switch out this context at those times too. If we can avoid using a global state flag for this (for example by carrying this on the ExpressionEvaluationContextRecord, that'd be a lot less worrying.

clang/lib/Sema/SemaExpr.cpp
16107–16109

I'm a little uncomfortable about assuming that the innermost ExpressionEvaluationContext surrounding a lambda parameter is always a context for the lambda expression itself -- that's making a lot of assumptions about what contexts might be added in the future.

Instead of propagating this information from the lambda parameter context to the (presumed) lambda context when we pop an expression evaluation context for a lambda parameter, could we pull out the LambdaScopeInfo for the current lambda (getCurLambda()) and store the information there, to be either diagnosed or dropped when we reach ActOnStartOfLambdaDefinition?

clang/lib/Sema/SemaLambda.cpp
895–896

We've already parsed the default arguments at this point; it would seem simpler to directly clear out the immediate invocation state from the ExpressionEvaluationContextRecord rather than asking it to do that at the end of the scope. This would also remove the need for the IIESA_Drop mode entirely; if you also change the IsLambdaParamScope flag into an ExpressionKind on the context record, that would remove the need for the IIEndScopeAction field, given the information you need at the end of a scope is whether the ExpressionKind is that of a lambda parameter or not.

Tyker updated this revision to Diff 298818.Sat, Oct 17, 2:48 AM
Tyker marked 9 inline comments as done.

addressed comments.

clang/lib/Sema/Sema.cpp
1026

This is fixing a separate bug that consteval call in to top-level ExprEvalContext are not handled.
like this:

struct S {
  consteval S(bool b) {if (b) throw;}
};
S s2(true);

this emits no error.
because the immediate invocation are added to global scope and the global scope is never poped via PopExpressionEvaluationContext.

clang/lib/Sema/SemaDeclCXX.cpp
17045–17048 ↗(On Diff #298344)

this fixes an issue where we would generate 2 error when we couldn't evaluate a consteval call inside the initialization of a constexpr/constinit
variable.

consteval void check(bool b) { if (!b) throw; }
constinit int x = (check(false), 2);

one of those error is "call to consteval function 'check' is not a constant expression"
the second is "variable does not have a constant initializer"

i fixed it by marking the expression as failed when the consteval call fails.

17062 ↗(On Diff #298344)

i changed approach and this isn't needed anymore.

rsmith added inline comments.Sat, Oct 17, 4:16 PM
clang/lib/Sema/Sema.cpp
1026

In this case, what's supposed to happen is that we create an ExpressionEvaluationContext for the initializer of s2. See Sema::ActOnCXXEnterDeclInitializer / Sema::ActOnCXXExitDeclInitializer. I guess that's not working somehow?

Ah, the problem is that we leave the context too early in Parser::ParseDeclarationAfterDeclaratorAndAttributes. We call InitScope.pop() before we add the initializer to the declaration, which is where we actually finish building the initialization full-expression -- that seems wrong in general. (We're also missing an InitializerScopeRAII object around the call to ActOnUninitializedDecl in the case where there is no initializer -- we need an ExpressionEvaluationContext for the default-initialization in that case too.)

clang/lib/Sema/SemaExpr.cpp
16232

Changing the dependence flags on an expression isn't safe to do this late: the Error dependence bit should be propagated to all transitive enclosing AST nodes when they're created, which has already happened at this point. If we need a way to represent an expression that's supposed to be a constant expression but isn't constant, perhaps adding a separate flag to ConstantExpr would be reasonable.

clang/lib/Sema/SemaLambda.cpp
897–901

Thanks! I think this can be cleaned up a little bit further:

  • Move the handling of lambda parameter scopes from HandleImmediateInvocations to PopExpressionEvaluationContext
  • Change HandleImmediateInvocations to take the ReferenceToConsteval and ImmediateInvocationCandidates data instead of an ExpressionEvaluationContextRecord
  • Maybe consider wrapping ReferenceToConsteval and ImmediateInvocationCandidates in a struct to make it a bit easier to pass them around as a set.