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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.
Herald added a project: All.

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.

Can you rebase and verify if there are still assertion failures. I think the 
assertion is fair. Dependent immediate function invocations must be done during 
instantiation. I suspect https://reviews.llvm.org/D132031 might have fixed the 
issue.

> This code ...
>
>   template
>   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 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?..

Can you also give a reproducer for which you see the assertion failure if you 
still see this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

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
  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::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?..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 409035.
Izaron added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,24 @@
 
 } // namespace unevaluated
 
+namespace templated {
+
+consteval int f(int v) {
+  return v;
+}
+
+template 
+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;
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5326,8 +5326,13 @@
 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;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16637,6 +16637,11 @@
 ExpressionEvaluationContextRecord::ExpressionKind ExprContext) {
   ExprEvalContexts.emplace_back(NewContext, ExprCleanupObjects.size(), Cleanup,
 LambdaContextDecl, ExprContext);
+  assert(
+  !(ExprEvalContexts.back().Context ==
+ExpressionEvaluationContext::PotentiallyEvaluated &&
+ExprEvalContexts[ExprEvalContexts.size() - 2].isConstantEvaluated()) &&
+  "Can't have an evaluated context within an unevaluated context!");
 
   // Discarded statements and immediate contexts nested in other
   // discarded statements or immediate context are themselves


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,24 @@
 
 } // namespace unevaluated
 
+namespace templated {
+
+consteval int f(int v) {
+  return v;
+}
+
+template 
+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;
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5326,8 +5326,13 @@
 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;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16637,6 +16637,11 @@
 ExpressionEvaluationContextRecord::ExpressionKind ExprContext) {
   ExprEvalContexts.emplace_back(NewContext, ExprCleanupObjects.size(), Cleanup,
  

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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

> 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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

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 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: cor3ntin.
erichkeane added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 408220.
Izaron added a comment.

(Fast test comment 80 character line fix)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119651/new/

https://reviews.llvm.org/D119651

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,24 @@
 
 } // namespace unevaluated
 
+namespace templated {
+
+consteval int f(int v) {
+  return v;
+}
+
+template 
+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;
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5326,8 +5326,13 @@
 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;


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,24 @@
 
 } // namespace unevaluated
 
+namespace templated {
+
+consteval int f(int v) {
+  return v;
+}
+
+template 
+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;
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5326,8 +5326,13 @@
 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;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron created this revision.
Izaron added reviewers: aaron.ballman, erichkeane, rsmith.
Izaron requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119651

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,23 @@
 
 } // namespace unevaluated
 
+namespace templated {
+
+consteval int f(int v) {
+  return v;
+}
+
+template 
+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;
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5326,8 +5326,13 @@
 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;


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,23 @@
 
 } // namespace unevaluated
 
+namespace templated {
+
+consteval int f(int v) {
+  return v;
+}
+
+template 
+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;
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5326,8 +5326,13 @@
 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;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits