Page MenuHomePhabricator

[clang] fix consteval call in default arguments
AbandonedPublic

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.Oct 15 2020, 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 TranscriptOct 15 2020, 3:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
keryell added inline comments.
clang/lib/Sema/SemaExpr.cpp
16795
keryell retitled this revision from [clang] fix consteval call in default arguements to [clang] fix consteval call in default arguments.Oct 15 2020, 2:54 PM
rsmith added inline comments.Oct 15 2020, 4:11 PM
clang/include/clang/Parse/Parser.h
3029
clang/include/clang/Sema/Sema.h
1250
1252
5010

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

clang/lib/Sema/Sema.cpp
1026 ↗(On Diff #298344)

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
16572–16574

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 ↗(On Diff #298344)

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.Oct 17 2020, 2:48 AM
Tyker marked 9 inline comments as done.

addressed comments.

clang/lib/Sema/Sema.cpp
1026 ↗(On Diff #298344)

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.Oct 17 2020, 4:16 PM
clang/lib/Sema/Sema.cpp
1026 ↗(On Diff #298344)

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
16700

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 ↗(On Diff #298818)

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.

@Tyker -- are you planning to pick this review back up again sometime in the near future? If not, do you care if the review gets commandeered?

Tyker updated this revision to Diff 381416.Oct 21 2021, 4:13 PM

@Tyker -- are you planning to pick this review back up again sometime in the near future? If not, do you care if the review gets commandeered?

here is an update that i think is close to committable. if it is not i am fine if it gets commandeered.

Changes

  • rebased
  • fix the default parameter situation for functions and lambdas.
  • fix the top level variables not being properly checked.

the double errors are not fixed on code like:

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

constexpr auto e = g();
constexpr auto e1 = f1;

@Tyker -- are you planning to pick this review back up again sometime in the near future? If not, do you care if the review gets commandeered?

here is an update that i think is close to committable. if it is not i am fine if it gets commandeered.

Changes

  • rebased
  • fix the default parameter situation for functions and lambdas.
  • fix the top level variables not being properly checked.

the double errors are not fixed on code like:

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

constexpr auto e = g();
constexpr auto e1 = f1;

Thank you for the update and letting me know you're fine if this is commandeered.

FWIW, I am not seeing double errors on that code. Here's the output I get with this patch applied locally:

F:\source\llvm-project>cat "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
consteval int f1() { return 0; }
consteval auto g() { return f1; }

constexpr auto e = g();
constexpr auto e1 = f1;

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only -std=c++2b "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:20: error: call to consteval function 'g' is not a
      constant expression
constexpr auto e = g();
                   ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:20: note: pointer to a consteval declaration is not a
      constant expression
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:16: error: constexpr variable 'e' must be initialized
      by a constant expression
constexpr auto e = g();
               ^   ~~~
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:16: note: pointer to a consteval declaration is not a
      constant expression
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:21: error: cannot take address of consteval function
      'f1' outside of an immediate invocation
constexpr auto e1 = f1;
                    ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:16: error: constexpr variable 'e1' must be initialized
      by a constant expression
constexpr auto e1 = f1;
               ^    ~~
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:16: note: pointer to a consteval declaration is not a
      constant expression
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
4 errors generated.

These look like valid errors to me, so am I misunderstanding something? Is the concern that we're emitting error: constexpr variable '<whatever>' must be initialized by a constant expression after we already issued a diagnostic on the same line?

From my testing, there's more work to be done, but I think it can be done in a follow-up. Consider:

template <typename Ty>
struct S {
  consteval S() = default;
  Ty Val;
};

struct foo {
  foo();
};

S<int> s1; // Correctly rejected
S<foo> s2; // Incorrectly accepted

With this patch applied, s1 is correctly diagnosed, but s2 is not. The reason it's not is because Sema::CheckForImmediateInvocation() tests !Decl->isConsteval() to early return, and for the instantiation of S<foo>, the constructor is marked as "unspecified" rather than consteval. The reason for that is because we set DefaultedDefaultConstructorIsConstexpr to false at https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/DeclCXX.cpp#L1309 when instantiating the class. I think we may have some confusion where we mean "*IsConstexpr" to mean "is a constexpr function" or "is usable as a constexpr function", because my reading of https://eel.is/c++draft/dcl.constexpr#7 is that the instantiated template constructor IS a constexpr function but it is not usable in a constant expression. However, that might be orthogonal to the fixes here and should be done separately.

clang/lib/Parse/ParseDecl.cpp
2393

Is there a reason we're not moving this one to below AddInititializerToDecl() as we did elsewhere?

clang/lib/Sema/SemaDecl.cpp
14510
Tyker updated this revision to Diff 382702.EditedOct 27 2021, 10:01 AM

FWIW, I am not seeing double errors on that code. Here's the output I get with this patch applied locally:

F:\source\llvm-project>cat "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
consteval int f1() { return 0; }
consteval auto g() { return f1; }

constexpr auto e = g();
constexpr auto e1 = f1;

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only -std=c++2b "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:20: error: call to consteval function 'g' is not a
      constant expression
constexpr auto e = g();
                   ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:20: note: pointer to a consteval declaration is not a
      constant expression
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:16: error: constexpr variable 'e' must be initialized
      by a constant expression
constexpr auto e = g();
               ^   ~~~
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:16: note: pointer to a consteval declaration is not a
      constant expression
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:21: error: cannot take address of consteval function
      'f1' outside of an immediate invocation
constexpr auto e1 = f1;
                    ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:16: error: constexpr variable 'e1' must be initialized
      by a constant expression
constexpr auto e1 = f1;
               ^    ~~
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:16: note: pointer to a consteval declaration is not a
      constant expression
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
consteval int f1() { return 0; }
              ^
4 errors generated.

These look like valid errors to me, so am I misunderstanding something? Is the concern that we're emitting error: constexpr variable '<whatever>' must be initialized by a constant expression after we already issued a diagnostic on the same line?

Yes, i think that
error: constexpr variable '<whatever>' must be initialized by a constant expression + call to consteval function '<whatever>' is not a constant expression or
error: constexpr variable '<whatever>' must be initialized by a constant expression + cannot take address of consteval function <whatever>' outside of an immediate invocation
is emitting 2 errors for the same root cause. the correct solution in my mind would be to swap too constant context when processing the initializer of a constexpr (or constinit) variable. this would disable consteval checking.

From my testing, there's more work to be done, but I think it can be done in a follow-up. Consider:

template <typename Ty>
struct S {
  consteval S() = default;
  Ty Val;
};

struct foo {
  foo();
};

S<int> s1; // Correctly rejected
S<foo> s2; // Incorrectly accepted

With this patch applied, s1 is correctly diagnosed, but s2 is not. The reason it's not is because Sema::CheckForImmediateInvocation() tests !Decl->isConsteval() to early return, and for the instantiation of S<foo>, the constructor is marked as "unspecified" rather than consteval. The reason for that is because we set DefaultedDefaultConstructorIsConstexpr to false at https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/DeclCXX.cpp#L1309 when instantiating the class. I think we may have some confusion where we mean "*IsConstexpr" to mean "is a constexpr function" or "is usable as a constexpr function", because my reading of https://eel.is/c++draft/dcl.constexpr#7 is that the instantiated template constructor IS a constexpr function but it is not usable in a constant expression. However, that might be orthogonal to the fixes here and should be done separately.

having a function that is explicitly specified consteval not returning true for isConsteval seems wrong.

IsConstexpr means the function has a either a consteval or constexpr(maybe implicit) specifier.
so i guess that "is a constexpr function" would be isConstexprSpecified
and "is usable as a constexpr function" would be isConstexpr

Tyker added inline comments.Oct 27 2021, 10:04 AM
clang/lib/Parse/ParseDecl.cpp
2393

yes, It was causing a few test failures, i didn't investigate much further.

Tyker abandoned this revision.Aug 21 2022, 10:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2022, 10:39 AM