[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Thanks, landed! I have benefited a lot from your comments!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG36f67434f724: [AST] Stop evaluate constant expression if the 
condition expression which in… (authored by yronglin).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-function-recovery-crash.cpp


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });   
 // expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,12 +5007,13 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
-return ESR_Failed;
-} else {
-  if (!EvaluateInteger(SS->getCond(), Value, Info))
-return ESR_Failed;
+  // We don't know what the value is, and which branch should jump to.
+  EvaluateDependentExpr(SS->getCond(), Info);
+  return ESR_Failed;
 }
+if (!EvaluateInteger(SS->getCond(), Value, Info))
+  return ESR_Failed;
+
 if (!CondScope.destroy())
   return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -570,6 +570,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 `_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });// expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,12 +5007,13 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
-return ESR_Failed;
-} else {
-  if (!EvaluateInteger(SS->getCond(), Value, Info))
-return ESR_Failed;
+  // We don't know what the value is, and which branch should jump to.
+  EvaluateDependentExpr(SS->getCond(), Info);
+  return ESR_Failed;
 }
+if (!EvaluateInteger(SS->getCond(), Value, Info))
+  return ESR_Failed;
+
 if (!CondScope.destroy())
   return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -570,6 +570,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 `_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM. thank you!

In D153296#4480243 , @hokein wrote:

> In D153296#4480141 , @yronglin 
> wrote:
>
>> In D153296#4479718 , @hokein wrote:
>>
>>> Thanks, this looks good.
>>
>> Thanks for your review! I don't know why the reversion status still `Needs 
>> Review`, and the `libcxx ci` often fails to start.
>
> I guess the `Needs Review` is probably caused by the previous "Requested 
> Changes" by Aaron.

Correct.

> The `libcxx ci` failure doesn't seem to be relevant. I think we're all on the 
> same page about the fix, it is fine to land it assuming that the `ninja 
> check-clang` passes.

Agreed, the libcxx failure is unrelated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D153296#4480141 , @yronglin wrote:

> In D153296#4479718 , @hokein wrote:
>
>> Thanks, this looks good.
>
> Thanks for your review! I don't know why the reversion status still `Needs 
> Review`, and the `libcxx ci` often fails to start.

I guess the `Needs Review` is probably caused by the previous "Requested 
Changes" by Aaron.
The `libcxx ci` failure doesn't seem to be relevant. I think we're all on the 
same page about the fix, it is fine to land it assuming that the `ninja 
check-clang` passes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D153296#4479718 , @hokein wrote:

> Thanks, this looks good.

Thanks for your review! I don't know why the reversion status still `Needs 
Review`, and the `libcxx ci` often fails to start.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

Thanks, this looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin marked 2 inline comments as done.
yronglin added a comment.

Thanks for your review!




Comment at: clang/lib/AST/ExprConstant.cpp:4914
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());

aaron.ballman wrote:
> rsmith wrote:
> > I don't think the changes to this function are appropriate, because:
> > 
> > 1) The special-casing of `RecoveryExpr` doesn't seem like it can be 
> > correct. There's no guarantee that we get a `RecoveryExpr` any time we 
> > encounter an expression that contains errors; error-dependence can be 
> > propagated from other places, such as types.
> > 2) For other error-dependent expressions, we also can't necessarily compute 
> > a value.
> > 3) It's not the responsibility of this function to deal with the situation 
> > where a value is needed and can't be produced -- the responsibility to 
> > handle that lies with the caller of this function instead. Eg, look at the 
> > handling of `ReturnStmt` or `DoStmt`.
> > 
> > So I think we should undo all the changes in this function, and only fix 
> > `SwitchStmt` to properly handle a value-dependent condition.
> Thank you for the explanation -- this makes more sense to me now. I agree 
> with the suggestion to just change `EvaluateSwitch()`, sorry for the false 
> start!
Thanks! I have undo this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 537929.
yronglin added a comment.

Format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-function-recovery-crash.cpp


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });   
 // expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,12 +5007,13 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
-return ESR_Failed;
-} else {
-  if (!EvaluateInteger(SS->getCond(), Value, Info))
-return ESR_Failed;
+  // We don't know what the value is, and which branch should jump to.
+  EvaluateDependentExpr(SS->getCond(), Info);
+  return ESR_Failed;
 }
+if (!EvaluateInteger(SS->getCond(), Value, Info))
+  return ESR_Failed;
+
 if (!CondScope.destroy())
   return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -568,6 +568,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 `_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });// expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,12 +5007,13 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
-return ESR_Failed;
-} else {
-  if (!EvaluateInteger(SS->getCond(), Value, Info))
-return ESR_Failed;
+  // We don't know what the value is, and which branch should jump to.
+  EvaluateDependentExpr(SS->getCond(), Info);
+  return ESR_Failed;
 }
+if (!EvaluateInteger(SS->getCond(), Value, Info))
+  return ESR_Failed;
+
 if (!CondScope.destroy())
   return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -568,6 +568,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 `_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^
___
cfe-commits mailing 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 537928.
yronglin marked an inline comment as done.
yronglin added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-function-recovery-crash.cpp


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });   
 // expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,12 +5007,13 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
-return ESR_Failed;
-} else {
-  if (!EvaluateInteger(SS->getCond(), Value, Info))
-return ESR_Failed;
+  // We don't know what the value is, and which branch should jump to.
+  EvaluateDependentExpr(SS->getCond(), Info);
+  return ESR_Failed;
 }
+if (!EvaluateInteger(SS->getCond(), Value, Info))
+return ESR_Failed;
+   
 if (!CondScope.destroy())
   return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -568,6 +568,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 `_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });// expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,12 +5007,13 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
-return ESR_Failed;
-} else {
-  if (!EvaluateInteger(SS->getCond(), Value, Info))
-return ESR_Failed;
+  // We don't know what the value is, and which branch should jump to.
+  EvaluateDependentExpr(SS->getCond(), Info);
+  return ESR_Failed;
 }
+if (!EvaluateInteger(SS->getCond(), Value, Info))
+return ESR_Failed;
+   
 if (!CondScope.destroy())
   return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -568,6 +568,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 `_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4914
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());

rsmith wrote:
> I don't think the changes to this function are appropriate, because:
> 
> 1) The special-casing of `RecoveryExpr` doesn't seem like it can be correct. 
> There's no guarantee that we get a `RecoveryExpr` any time we encounter an 
> expression that contains errors; error-dependence can be propagated from 
> other places, such as types.
> 2) For other error-dependent expressions, we also can't necessarily compute a 
> value.
> 3) It's not the responsibility of this function to deal with the situation 
> where a value is needed and can't be produced -- the responsibility to handle 
> that lies with the caller of this function instead. Eg, look at the handling 
> of `ReturnStmt` or `DoStmt`.
> 
> So I think we should undo all the changes in this function, and only fix 
> `SwitchStmt` to properly handle a value-dependent condition.
Thank you for the explanation -- this makes more sense to me now. I agree with 
the suggestion to just change `EvaluateSwitch()`, sorry for the false start!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Making the `return ESR_Failed;` unconditional looks to be the correct change 
here. We can't continue evaluation past that point because we don't know what 
would be executed next. Unconditionally returning `ESR_Failed` in that 
situation is what the other similar paths through `EvaluateStmt` do.




Comment at: clang/lib/AST/ExprConstant.cpp:4914
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());

I don't think the changes to this function are appropriate, because:

1) The special-casing of `RecoveryExpr` doesn't seem like it can be correct. 
There's no guarantee that we get a `RecoveryExpr` any time we encounter an 
expression that contains errors; error-dependence can be propagated from other 
places, such as types.
2) For other error-dependent expressions, we also can't necessarily compute a 
value.
3) It's not the responsibility of this function to deal with the situation 
where a value is needed and can't be produced -- the responsibility to handle 
that lies with the caller of this function instead. Eg, look at the handling of 
`ReturnStmt` or `DoStmt`.

So I think we should undo all the changes in this function, and only fix 
`SwitchStmt` to properly handle a value-dependent condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin added a comment.

Many thanks for all of your comments, I learned a lot from the discussions, 
your incredible depth of knowledge have helped fundamentally shape Clang into a 
great compiler! 
It seems the common denominator is that constant evaluation should stop when we 
encounter a value dependent expression, and return `ESR_Failed`.




Comment at: clang/lib/AST/ExprConstant.cpp:5019
 if (SS->getCond()->isValueDependent()) {
   if (!EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

shafik wrote:
> Please don't forget to remove this `if` and make the return unconditional as 
> reinforced by @hokein comment above.
Thanks for your tips!, removed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 537714.
yronglin added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-function-recovery-crash.cpp


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });   
 // expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4913,11 +4913,20 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
-  if (Info.noteSideEffect())
-return true;
+  // Note that we have a side effect that matters for constant evaluation.
+  bool SideEffects = Info.noteSideEffect();
+  // If the reason we're here is because of a recovery expression, we don't
+  // want to continue to evaluate further as we will never know what the actual
+  // value is.
+  if (isa(E))
+return false;
+
+  // Otherwise, return whether we want to continue after noting the side
+  // effects, which should only happen if the expression has errors but isn't
+  // a recovery expression on its own.
   assert(E->containsErrors() && "valid value-dependent expression should never 
"
 "reach invalid code path.");
-  return false;
+  return SideEffects;
 }
 
 /// Evaluate a condition (either a variable declaration or an expression).
@@ -5007,12 +5016,13 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
-return ESR_Failed;
-} else {
-  if (!EvaluateInteger(SS->getCond(), Value, Info))
-return ESR_Failed;
+  // We don't know what the value is, and which branch should jump to.
+  EvaluateDependentExpr(SS->getCond(), Info);
+  return ESR_Failed;
 }
+if (!EvaluateInteger(SS->getCond(), Value, Info))
+return ESR_Failed;
+   
 if (!CondScope.destroy())
   return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -568,6 +568,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 `_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});  // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });// expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
 // FIXME: should bail out instead of looping.
 // expected-note@-2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4913,11 +4913,20 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
-  if (Info.noteSideEffect())
-return true;
+  // Note that we have a side effect that matters for constant evaluation.
+  bool SideEffects = Info.noteSideEffect();
+  // If the reason we're here is because of a recovery expression, we don't
+  // want to 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

aaron.ballman wrote:
> hokein wrote:
> > hokein wrote:
> > > hokein wrote:
> > > > yronglin wrote:
> > > > > yronglin wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > yronglin wrote:
> > > > > > > > hokein wrote:
> > > > > > > > > The constant evaluator is not aware of the "error" concept, 
> > > > > > > > > it is only aware of value-dependent -- the general idea 
> > > > > > > > > behind is that we treat the dependent-on-error and 
> > > > > > > > > dependent-on-template-parameters cases the same, they are 
> > > > > > > > > potentially constant (if we see an expression contains 
> > > > > > > > > errors, it could be constant depending on how the error is 
> > > > > > > > > resolved), this will give us nice recovery and avoid bogus 
> > > > > > > > > following diagnostics.
> > > > > > > > > 
> > > > > > > > > So, a `RecoveryExpr` should not result in failure when 
> > > > > > > > > checking for a potential constant expression.
> > > > > > > > > 
> > > > > > > > > I think the right fix is to remove the conditional check `if 
> > > > > > > > > (!EvaluateDependentExpr(SS->getCond(), Info))` in 
> > > > > > > > > `EvaluateSwitch`, and return `ESR_Failed` unconditionally (we 
> > > > > > > > > don't know its value, any switch-case anwser will be wrong in 
> > > > > > > > > some cases). We already do this for return-statment, 
> > > > > > > > > do-statement etc.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > Do you mean?
> > > > > > > > ```
> > > > > > > > if (SS->getCond()->isValueDependent()) {
> > > > > > > > EvaluateDependentExpr(SS->getCond(), Info);
> > > > > > > > return ESR_Failed;
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > the general idea behind is that we treat the dependent-on-error 
> > > > > > > > and dependent-on-template-parameters cases the same, they are 
> > > > > > > > potentially constant (if we see an expression contains errors, 
> > > > > > > > it could be constant depending on how the error is resolved), 
> > > > > > > > this will give us nice recovery and avoid bogus following 
> > > > > > > > diagnostics.
> > > > > > > 
> > > > > > > I could use some further education on why this is the correct 
> > > > > > > approach. For a dependent-on-template-parameters case, this makes 
> > > > > > > sense -- either the template will be instantiated (at which point 
> > > > > > > we'll know if it's a constant expression) or it won't be (at 
> > > > > > > which point it's constant expression-ness doesn't matter). But 
> > > > > > > for error recovery, we will *never* get a valid constant 
> > > > > > > expression.
> > > > > > > 
> > > > > > > I worry about the performance overhead of continuing on with 
> > > > > > > constant expression evaluation in the error case. We use these 
> > > > > > > code paths not only to get a value but to say "is this a constant 
> > > > > > > expression at all?".
> > > > > > > 
> > > > > > > I don't see why the fix should be localized to just the switch 
> > > > > > > statement condition; it seems like *any* attempt to get a 
> > > > > > > dependent value from an error recovery expression is a point at 
> > > > > > > which we can definitively say "this is not a constant expression" 
> > > > > > > and move on.
> > > > > > I understand that continuing to perform constant evaluation when an 
> > > > > > error occurs can bring more additional diagnostic information (such 
> > > > > > as jumping to the default branch to continue calculation when the 
> > > > > > condition expression evaluation of switch-statement fails), but the 
> > > > > > additional diagnostic message that is emitted is in some cases 
> > > > > > doesn't usually useful, and as Aaron said may affect performance of 
> > > > > > clang. I don't have enough experience to make a tradeoff between 
> > > > > > the two. BTW 
> > > > > > https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909
> > > > > >  I don't quite understand why a RecoveryExpr is not created here, 
> > > > > > which caused to the whole do statement disappears on the 
> > > > > > AST(https://godbolt.org/z/PsPb31YKP), should we fix this? 
> > > > > Thanks a lot for your comments! @aaron.ballman 
> > > > > But for error recovery, we will *never* get a valid constant 
> > > > > expression.
> > > > 
> > > > Yeah, this is true, and we don't evaluate these error-dependent 
> > > > expressions.
> > > > 
> > > > I think the question here is that when we encounter an error-dependent 
> > > > expression during a constant expression evaluation, do we want to 
> > > > bailout the whole evaluation immediately, or just ignore the evaluation 
> > > > of this error-dependent expression and continue to proceed when 
> > > > possible?
> > > > 
> > > > We choose 2) -- this was based on the discussion 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5019
 if (SS->getCond()->isValueDependent()) {
   if (!EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

Please don't forget to remove this `if` and make the return unconditional as 
reinforced by @hokein comment above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

hokein wrote:
> hokein wrote:
> > hokein wrote:
> > > yronglin wrote:
> > > > yronglin wrote:
> > > > > aaron.ballman wrote:
> > > > > > yronglin wrote:
> > > > > > > hokein wrote:
> > > > > > > > The constant evaluator is not aware of the "error" concept, it 
> > > > > > > > is only aware of value-dependent -- the general idea behind is 
> > > > > > > > that we treat the dependent-on-error and 
> > > > > > > > dependent-on-template-parameters cases the same, they are 
> > > > > > > > potentially constant (if we see an expression contains errors, 
> > > > > > > > it could be constant depending on how the error is resolved), 
> > > > > > > > this will give us nice recovery and avoid bogus following 
> > > > > > > > diagnostics.
> > > > > > > > 
> > > > > > > > So, a `RecoveryExpr` should not result in failure when checking 
> > > > > > > > for a potential constant expression.
> > > > > > > > 
> > > > > > > > I think the right fix is to remove the conditional check `if 
> > > > > > > > (!EvaluateDependentExpr(SS->getCond(), Info))` in 
> > > > > > > > `EvaluateSwitch`, and return `ESR_Failed` unconditionally (we 
> > > > > > > > don't know its value, any switch-case anwser will be wrong in 
> > > > > > > > some cases). We already do this for return-statment, 
> > > > > > > > do-statement etc.
> > > > > > > > 
> > > > > > > > 
> > > > > > > Do you mean?
> > > > > > > ```
> > > > > > > if (SS->getCond()->isValueDependent()) {
> > > > > > > EvaluateDependentExpr(SS->getCond(), Info);
> > > > > > > return ESR_Failed;
> > > > > > > }
> > > > > > > ```
> > > > > > > the general idea behind is that we treat the dependent-on-error 
> > > > > > > and dependent-on-template-parameters cases the same, they are 
> > > > > > > potentially constant (if we see an expression contains errors, it 
> > > > > > > could be constant depending on how the error is resolved), this 
> > > > > > > will give us nice recovery and avoid bogus following diagnostics.
> > > > > > 
> > > > > > I could use some further education on why this is the correct 
> > > > > > approach. For a dependent-on-template-parameters case, this makes 
> > > > > > sense -- either the template will be instantiated (at which point 
> > > > > > we'll know if it's a constant expression) or it won't be (at which 
> > > > > > point it's constant expression-ness doesn't matter). But for error 
> > > > > > recovery, we will *never* get a valid constant expression.
> > > > > > 
> > > > > > I worry about the performance overhead of continuing on with 
> > > > > > constant expression evaluation in the error case. We use these code 
> > > > > > paths not only to get a value but to say "is this a constant 
> > > > > > expression at all?".
> > > > > > 
> > > > > > I don't see why the fix should be localized to just the switch 
> > > > > > statement condition; it seems like *any* attempt to get a dependent 
> > > > > > value from an error recovery expression is a point at which we can 
> > > > > > definitively say "this is not a constant expression" and move on.
> > > > > I understand that continuing to perform constant evaluation when an 
> > > > > error occurs can bring more additional diagnostic information (such 
> > > > > as jumping to the default branch to continue calculation when the 
> > > > > condition expression evaluation of switch-statement fails), but the 
> > > > > additional diagnostic message that is emitted is in some cases 
> > > > > doesn't usually useful, and as Aaron said may affect performance of 
> > > > > clang. I don't have enough experience to make a tradeoff between the 
> > > > > two. BTW 
> > > > > https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909
> > > > >  I don't quite understand why a RecoveryExpr is not created here, 
> > > > > which caused to the whole do statement disappears on the 
> > > > > AST(https://godbolt.org/z/PsPb31YKP), should we fix this? 
> > > > Thanks a lot for your comments! @aaron.ballman 
> > > > But for error recovery, we will *never* get a valid constant expression.
> > > 
> > > Yeah, this is true, and we don't evaluate these error-dependent 
> > > expressions.
> > > 
> > > I think the question here is that when we encounter an error-dependent 
> > > expression during a constant expression evaluation, do we want to bailout 
> > > the whole evaluation immediately, or just ignore the evaluation of this 
> > > error-dependent expression and continue to proceed when possible?
> > > 
> > > We choose 2) -- this was based on the discussion with @rsmith. This will 
> > > likely give us decent error-recovery and useful followup diagnostics.
> > > 
> > > Some concrete examples,
> > > 
> > > ```
> > > int abc();
> > > constexpr int f() { 
> > >   error(); 
> > >   // We emit a 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

yronglin wrote:
> yronglin wrote:
> > aaron.ballman wrote:
> > > yronglin wrote:
> > > > hokein wrote:
> > > > > The constant evaluator is not aware of the "error" concept, it is 
> > > > > only aware of value-dependent -- the general idea behind is that we 
> > > > > treat the dependent-on-error and dependent-on-template-parameters 
> > > > > cases the same, they are potentially constant (if we see an 
> > > > > expression contains errors, it could be constant depending on how the 
> > > > > error is resolved), this will give us nice recovery and avoid bogus 
> > > > > following diagnostics.
> > > > > 
> > > > > So, a `RecoveryExpr` should not result in failure when checking for a 
> > > > > potential constant expression.
> > > > > 
> > > > > I think the right fix is to remove the conditional check `if 
> > > > > (!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, 
> > > > > and return `ESR_Failed` unconditionally (we don't know its value, any 
> > > > > switch-case anwser will be wrong in some cases). We already do this 
> > > > > for return-statment, do-statement etc.
> > > > > 
> > > > > 
> > > > Do you mean?
> > > > ```
> > > > if (SS->getCond()->isValueDependent()) {
> > > > EvaluateDependentExpr(SS->getCond(), Info);
> > > > return ESR_Failed;
> > > > }
> > > > ```
> > > > the general idea behind is that we treat the dependent-on-error and 
> > > > dependent-on-template-parameters cases the same, they are potentially 
> > > > constant (if we see an expression contains errors, it could be constant 
> > > > depending on how the error is resolved), this will give us nice 
> > > > recovery and avoid bogus following diagnostics.
> > > 
> > > I could use some further education on why this is the correct approach. 
> > > For a dependent-on-template-parameters case, this makes sense -- either 
> > > the template will be instantiated (at which point we'll know if it's a 
> > > constant expression) or it won't be (at which point it's constant 
> > > expression-ness doesn't matter). But for error recovery, we will *never* 
> > > get a valid constant expression.
> > > 
> > > I worry about the performance overhead of continuing on with constant 
> > > expression evaluation in the error case. We use these code paths not only 
> > > to get a value but to say "is this a constant expression at all?".
> > > 
> > > I don't see why the fix should be localized to just the switch statement 
> > > condition; it seems like *any* attempt to get a dependent value from an 
> > > error recovery expression is a point at which we can definitively say 
> > > "this is not a constant expression" and move on.
> > I understand that continuing to perform constant evaluation when an error 
> > occurs can bring more additional diagnostic information (such as jumping to 
> > the default branch to continue calculation when the condition expression 
> > evaluation of switch-statement fails), but the additional diagnostic 
> > message that is emitted is in some cases doesn't usually useful, and as 
> > Aaron said may affect performance of clang. I don't have enough experience 
> > to make a tradeoff between the two. BTW 
> > https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909
> >  I don't quite understand why a RecoveryExpr is not created here, which 
> > caused to the whole do statement disappears on the 
> > AST(https://godbolt.org/z/PsPb31YKP), should we fix this? 
> Thanks a lot for your comments! @aaron.ballman 
> But for error recovery, we will *never* get a valid constant expression.

Yeah, this is true, and we don't evaluate these error-dependent expressions.

I think the question here is that when we encounter an error-dependent 
expression during a constant expression evaluation, do we want to bailout the 
whole evaluation immediately, or just ignore the evaluation of this 
error-dependent expression and continue to proceed when possible?

We choose 2) -- this was based on the discussion with @rsmith. This will likely 
give us decent error-recovery and useful followup diagnostics.

Some concrete examples,

```
int abc();
constexpr int f() { 
  error(); 
  // We emit a diagnostic "Constexpr function never produces a constant 
expression, because abc() cannot be used in a constant expression"
  return abc(); 
}
```

```
// We prefer to treat the function f as a constexpr function even though it has 
an error expression. We will preserve more information in the AST, e.g. 
clangd's hover on the function call `f()` can give you the return value 1.

constexpr int f() {
   error();
   return 1;
}
```

> I worry about the performance overhead of continuing on with constant 
> expression evaluation in the error case. We use these code paths not only to 
> get a value but to say "is 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

yronglin wrote:
> aaron.ballman wrote:
> > yronglin wrote:
> > > hokein wrote:
> > > > The constant evaluator is not aware of the "error" concept, it is only 
> > > > aware of value-dependent -- the general idea behind is that we treat 
> > > > the dependent-on-error and dependent-on-template-parameters cases the 
> > > > same, they are potentially constant (if we see an expression contains 
> > > > errors, it could be constant depending on how the error is resolved), 
> > > > this will give us nice recovery and avoid bogus following diagnostics.
> > > > 
> > > > So, a `RecoveryExpr` should not result in failure when checking for a 
> > > > potential constant expression.
> > > > 
> > > > I think the right fix is to remove the conditional check `if 
> > > > (!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, and 
> > > > return `ESR_Failed` unconditionally (we don't know its value, any 
> > > > switch-case anwser will be wrong in some cases). We already do this for 
> > > > return-statment, do-statement etc.
> > > > 
> > > > 
> > > Do you mean?
> > > ```
> > > if (SS->getCond()->isValueDependent()) {
> > > EvaluateDependentExpr(SS->getCond(), Info);
> > > return ESR_Failed;
> > > }
> > > ```
> > > the general idea behind is that we treat the dependent-on-error and 
> > > dependent-on-template-parameters cases the same, they are potentially 
> > > constant (if we see an expression contains errors, it could be constant 
> > > depending on how the error is resolved), this will give us nice recovery 
> > > and avoid bogus following diagnostics.
> > 
> > I could use some further education on why this is the correct approach. For 
> > a dependent-on-template-parameters case, this makes sense -- either the 
> > template will be instantiated (at which point we'll know if it's a constant 
> > expression) or it won't be (at which point it's constant expression-ness 
> > doesn't matter). But for error recovery, we will *never* get a valid 
> > constant expression.
> > 
> > I worry about the performance overhead of continuing on with constant 
> > expression evaluation in the error case. We use these code paths not only 
> > to get a value but to say "is this a constant expression at all?".
> > 
> > I don't see why the fix should be localized to just the switch statement 
> > condition; it seems like *any* attempt to get a dependent value from an 
> > error recovery expression is a point at which we can definitively say "this 
> > is not a constant expression" and move on.
> I understand that continuing to perform constant evaluation when an error 
> occurs can bring more additional diagnostic information (such as jumping to 
> the default branch to continue calculation when the condition expression 
> evaluation of switch-statement fails), but the additional diagnostic message 
> that is emitted is in some cases doesn't usually useful, and as Aaron said 
> may affect performance of clang. I don't have enough experience to make a 
> tradeoff between the two. BTW 
> https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909
>  I don't quite understand why a RecoveryExpr is not created here, which 
> caused to the whole do statement disappears on the 
> AST(https://godbolt.org/z/PsPb31YKP), should we fix this? 
Thanks a lot for your comments! @aaron.ballman 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin marked 3 inline comments as done.
yronglin added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

aaron.ballman wrote:
> yronglin wrote:
> > hokein wrote:
> > > The constant evaluator is not aware of the "error" concept, it is only 
> > > aware of value-dependent -- the general idea behind is that we treat the 
> > > dependent-on-error and dependent-on-template-parameters cases the same, 
> > > they are potentially constant (if we see an expression contains errors, 
> > > it could be constant depending on how the error is resolved), this will 
> > > give us nice recovery and avoid bogus following diagnostics.
> > > 
> > > So, a `RecoveryExpr` should not result in failure when checking for a 
> > > potential constant expression.
> > > 
> > > I think the right fix is to remove the conditional check `if 
> > > (!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, and 
> > > return `ESR_Failed` unconditionally (we don't know its value, any 
> > > switch-case anwser will be wrong in some cases). We already do this for 
> > > return-statment, do-statement etc.
> > > 
> > > 
> > Do you mean?
> > ```
> > if (SS->getCond()->isValueDependent()) {
> > EvaluateDependentExpr(SS->getCond(), Info);
> > return ESR_Failed;
> > }
> > ```
> > the general idea behind is that we treat the dependent-on-error and 
> > dependent-on-template-parameters cases the same, they are potentially 
> > constant (if we see an expression contains errors, it could be constant 
> > depending on how the error is resolved), this will give us nice recovery 
> > and avoid bogus following diagnostics.
> 
> I could use some further education on why this is the correct approach. For a 
> dependent-on-template-parameters case, this makes sense -- either the 
> template will be instantiated (at which point we'll know if it's a constant 
> expression) or it won't be (at which point it's constant expression-ness 
> doesn't matter). But for error recovery, we will *never* get a valid constant 
> expression.
> 
> I worry about the performance overhead of continuing on with constant 
> expression evaluation in the error case. We use these code paths not only to 
> get a value but to say "is this a constant expression at all?".
> 
> I don't see why the fix should be localized to just the switch statement 
> condition; it seems like *any* attempt to get a dependent value from an error 
> recovery expression is a point at which we can definitively say "this is not 
> a constant expression" and move on.
I understand that continuing to perform constant evaluation when an error 
occurs can bring more additional diagnostic information (such as jumping to the 
default branch to continue calculation when the condition expression evaluation 
of switch-statement fails), but the additional diagnostic message that is 
emitted is in some cases doesn't usually useful, and as Aaron said may affect 
performance of clang. I don't have enough experience to make a tradeoff between 
the two. BTW 
https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909
 I don't quite understand why a RecoveryExpr is not created here, which caused 
to the whole do statement disappears on the 
AST(https://godbolt.org/z/PsPb31YKP), should we fix this? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

yronglin wrote:
> hokein wrote:
> > The constant evaluator is not aware of the "error" concept, it is only 
> > aware of value-dependent -- the general idea behind is that we treat the 
> > dependent-on-error and dependent-on-template-parameters cases the same, 
> > they are potentially constant (if we see an expression contains errors, it 
> > could be constant depending on how the error is resolved), this will give 
> > us nice recovery and avoid bogus following diagnostics.
> > 
> > So, a `RecoveryExpr` should not result in failure when checking for a 
> > potential constant expression.
> > 
> > I think the right fix is to remove the conditional check `if 
> > (!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, and 
> > return `ESR_Failed` unconditionally (we don't know its value, any 
> > switch-case anwser will be wrong in some cases). We already do this for 
> > return-statment, do-statement etc.
> > 
> > 
> Do you mean?
> ```
> if (SS->getCond()->isValueDependent()) {
> EvaluateDependentExpr(SS->getCond(), Info);
> return ESR_Failed;
> }
> ```
> the general idea behind is that we treat the dependent-on-error and 
> dependent-on-template-parameters cases the same, they are potentially 
> constant (if we see an expression contains errors, it could be constant 
> depending on how the error is resolved), this will give us nice recovery and 
> avoid bogus following diagnostics.

I could use some further education on why this is the correct approach. For a 
dependent-on-template-parameters case, this makes sense -- either the template 
will be instantiated (at which point we'll know if it's a constant expression) 
or it won't be (at which point it's constant expression-ness doesn't matter). 
But for error recovery, we will *never* get a valid constant expression.

I worry about the performance overhead of continuing on with constant 
expression evaluation in the error case. We use these code paths not only to 
get a value but to say "is this a constant expression at all?".

I don't see why the fix should be localized to just the switch statement 
condition; it seems like *any* attempt to get a dependent value from an error 
recovery expression is a point at which we can definitively say "this is not a 
constant expression" and move on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin marked 3 inline comments as done.
yronglin added a comment.

Thanks a lot for your comments! @hokein




Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

hokein wrote:
> The constant evaluator is not aware of the "error" concept, it is only aware 
> of value-dependent -- the general idea behind is that we treat the 
> dependent-on-error and dependent-on-template-parameters cases the same, they 
> are potentially constant (if we see an expression contains errors, it could 
> be constant depending on how the error is resolved), this will give us nice 
> recovery and avoid bogus following diagnostics.
> 
> So, a `RecoveryExpr` should not result in failure when checking for a 
> potential constant expression.
> 
> I think the right fix is to remove the conditional check `if 
> (!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, and 
> return `ESR_Failed` unconditionally (we don't know its value, any switch-case 
> anwser will be wrong in some cases). We already do this for return-statment, 
> do-statement etc.
> 
> 
Do you mean?
```
if (SS->getCond()->isValueDependent()) {
EvaluateDependentExpr(SS->getCond(), Info);
return ESR_Failed;
}
```



Comment at: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp:91
+
+static_assert(test13(), "should not crash"); // expected-error {{static 
assertion expression is not an integral constant expression}}
+

hokein wrote:
> nit: we can simplify it with the `TEST_EVALUATE` macro:
> 
> ```
> TEST_EVALUATE(SwitchErrorCond, switch(undef) { case 0: return 7; default: 
> break;})
> ```
Thanks, I will use `TEST_EVALUATE ` to simplify.



Comment at: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp:93
+
+constexpr int test14() {
+int sum = 0;

hokein wrote:
> Is this a new crash (and the tests below)?
> 
> They don't look like new crashes, I think the current constant evaluator 
> should be able to handle them well. IIUC the only crash we have is the case 
> where we have a error-dependent condition in `switch`?
Thanks you for catch this, it's my mistake, I have ever copied these tests from 
the code above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the fix. Sorry for being late (I was looking through the emails and 
found this patch).




Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa(E))
+return false;

The constant evaluator is not aware of the "error" concept, it is only aware of 
value-dependent -- the general idea behind is that we treat the 
dependent-on-error and dependent-on-template-parameters cases the same, they 
are potentially constant (if we see an expression contains errors, it could be 
constant depending on how the error is resolved), this will give us nice 
recovery and avoid bogus following diagnostics.

So, a `RecoveryExpr` should not result in failure when checking for a potential 
constant expression.

I think the right fix is to remove the conditional check `if 
(!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, and return 
`ESR_Failed` unconditionally (we don't know its value, any switch-case anwser 
will be wrong in some cases). We already do this for return-statment, 
do-statement etc.





Comment at: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp:91
+
+static_assert(test13(), "should not crash"); // expected-error {{static 
assertion expression is not an integral constant expression}}
+

nit: we can simplify it with the `TEST_EVALUATE` macro:

```
TEST_EVALUATE(SwitchErrorCond, switch(undef) { case 0: return 7; default: 
break;})
```



Comment at: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp:93
+
+constexpr int test14() {
+int sum = 0;

Is this a new crash (and the tests below)?

They don't look like new crashes, I think the current constant evaluator should 
be able to handle them well. IIUC the only crash we have is the case where we 
have a error-dependent condition in `switch`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 536802.
yronglin added a comment.

Address comment.

- Change `EvaluateDependentExpr`.
- Add more test for do/while/for/return/ctor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-function-recovery-crash.cpp

Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -78,6 +78,132 @@
 constexpr int test12() { return "wrong"; } // expected-error {{cannot initialize return object of type 'int'}}
 constexpr int force12 = test12();  // expected-error {{must be initialized by a constant}}
 
+constexpr int test13() {
+switch (invalid_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(test13(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test14() {
+int sum = 0;
+for (int i = 0; i < invalid_value; ++i) // expected-error {{use of undeclared identifier}}
+sum = sum + i;
+return sum;
+}
+
+static_assert(test14(), "should not crash"); // expected-error {{static assertion failed due to requirement}}
+
+constexpr int test15() {
+int sum = 0;
+for (int i = 0; i < 10; i += invalid_value) // expected-error {{use of undeclared identifier}}
+sum = sum + i;
+return sum;
+}
+
+static_assert(test15(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test16() {
+int sum = 0;
+for (int i = invalid_value; i < 10; ++i) // expected-error {{use of undeclared identifier}}
+sum = sum + i;
+return sum;
+}
+
+static_assert(test16(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test17() {
+int sum = 0, i = 0;
+do
+  sum += i;
+while (invalid_value); // expected-error {{use of undeclared identifier}}
+return sum;
+}
+
+static_assert(test17(), "should not crash"); // expected-error {{static assertion failed due to requirement}}
+
+constexpr int test18() {
+  int sum = 0, i = 0;
+  while (invalid_value) // expected-error {{use of undeclared identifier}}
+sum += i;
+  return sum;
+}
+
+static_assert(test18(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test19() {
+struct Test19 {
+int a[2] = {0, 1};
+constexpr const int *begin() const {
+return invalid_value;  // expected-error {{use of undeclared identifier}}
+}
+constexpr const int *end() const {
+return a + 2;
+}
+};
+
+int sum = 0;
+Test19 t;
+for (const auto v : t) {
+sum += v;
+}
+return sum;
+}
+
+static_assert(test19(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test20() {
+struct Test20 {
+int a[2] = {0, 1};
+constexpr const int *begin() const {
+return a;
+}
+constexpr const int *end() const {
+return invalid_value;  // expected-error {{use of undeclared identifier}}
+}
+};
+
+int sum = 0;
+Test20 t;
+for (const auto v : t) {
+sum += v;
+}
+return sum;
+}
+
+static_assert(test20(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test21() {
+  return invalid_value; // expected-error {{use of undeclared identifier}}
+}
+
+static_assert(test21(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test22() {
+  struct Test22 {
+int value = invalid_value; // expected-error {{use of undeclared identifier}}
+  };
+  return Test22().value;
+}
+
+static_assert(test22(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
+constexpr int test23() {
+  struct Test23 {
+int value;
+constexpr Test23(int v) : value(v) {}
+constexpr Test23(int a, int b) : Test23(a * b * invalid_value) {} // expected-error {{use of undeclared identifier}}
+  };
+  return Test23(1, 2).value;
+}
+
+static_assert(test23(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
 #define TEST_EVALUATE(Name, X) \
   constexpr int testEvaluate##Name() { \
 X return 0;\
Index: 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D153296#4468373 , @aaron.ballman 
wrote:

> In D153296#4459769 , @yronglin 
> wrote:
>
>> Please help me, I have no better idea on this issue, do you have any better 
>> ideas? @erichkeane @shafik
>
> I think what's being suggested is to change `EvaluateDependentExpr()` 
> somewhat along these lines:
>
>   static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
> assert(E->isValueDependent());
>   
> // Note that we have a side effect that matters for constant evaluation.
> bool SideEffects = Info.noteSideEffect();
> // If the reason we're here is because of a recovery expression, we don't
> // want to continue to evaluate further as we will never know what the 
> actual
> // value is.
> if (isa(E))
>   return false;
>   
> // Otherwise, return whether we want to continue after noting the side
> // effects, which should only happen if the expression has errors but 
> isn't
> // a recovery expression on its own.
> assert(E->containsErrors() && "valid value-dependent expression should 
> never "
>   "reach invalid code path.");
> return SideEffects;
>   }
>
> This way, code paths that get down to a `RecoveryExpr` will not continue to 
> evaluate further (as there's really no point -- there's no way to get a 
> reasonable value from from the recovery expression anyway), but the fix isn't 
> specific to just switch statements. After making these changes, you should 
> look for places where `EvaluateDependentExpr()` is being called to try to 
> come up with a test case where that expression is a recovery expression so 
> that we can fill out test coverage beyond just the situation with `switch` 
> from the original report. Does that make sense?
>
> (Marking as requesting changes so it's clear this review isn't yet accepted.)

Thanks a lot! @aaron.ballman , I try to address comments and add more test, 
this case (https://godbolt.org/z/ExPoEKcrf) looks strange, why the do-statement 
missing in the printed AST?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Aaron Ballman via Phabricator via cfe-commits]

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

In D153296#4459769 , @yronglin wrote:

> Please help me, I have no better idea on this issue, do you have any better 
> ideas? @erichkeane @shafik

I think what's being suggested is to change `EvaluateDependentExpr()` somewhat 
along these lines:

  static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
assert(E->isValueDependent());
  
// Note that we have a side effect that matters for constant evaluation.
bool SideEffects = Info.noteSideEffect();
// If the reason we're here is because of a recovery expression, we don't
// want to continue to evaluate further as we will never know what the 
actual
// value is.
if (isa(E))
  return false;
  
// Otherwise, return whether we want to continue after noting the side
// effects, which should only happen if the expression has errors but isn't
// a recovery expression on its own.
assert(E->containsErrors() && "valid value-dependent expression should 
never "
  "reach invalid code path.");
return SideEffects;
  }

This way, code paths that get down to a `RecoveryExpr` will not continue to 
evaluate further (as there's really no point -- there's no way to get a 
reasonable value from from the recovery expression anyway), but the fix isn't 
specific to just switch statements. After making these changes, you should look 
for places where `EvaluateDependentExpr()` is being called to try to come up 
with a test case where that expression is a recovery expression so that we can 
fill out test coverage beyond just the situation with `switch` from the 
original report. Does that make sense?

(Marking as requesting changes so it's clear this review isn't yet accepted.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

friendly ping~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Please help me, I have no better idea on this issue, do you have any better 
idea? @erichkeane @shafik


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Seems the diagnostic message `:5:9: note: constexpr evaluation hit 
maximum step limit; possible infinite loop?` was redundant, also gcc dose not 
emit this message.

https://godbolt.org/z/v55P88cdT


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 534606.
yronglin added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/switch.cpp


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is 
not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,7 +5007,9 @@
 APSInt LHS = CS->getLHS()->EvaluateKnownConstInt(Info.Ctx);
 APSInt RHS = CS->getRHS() ? CS->getRHS()->EvaluateKnownConstInt(Info.Ctx)
   : LHS;
-if (LHS <= Value && Value <= RHS) {
+if (Value.getBitWidth() == LHS.getBitWidth() &&
+Value.getBitWidth() == RHS.getBitWidth() && LHS <= Value &&
+Value <= RHS) {
   Found = SC;
   break;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -531,6 +531,9 @@
 - Fixed a failing assertion when applying an attribute to an anonymous union.
   The assertion was benign outside of asserts builds and would only fire in 
C++.
   (`#48512 _`).
+- Stop evaluating a constant expression if the condition expression which in 
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -5007,7 +5007,9 @@
 APSInt LHS = CS->getLHS()->EvaluateKnownConstInt(Info.Ctx);
 APSInt RHS = CS->getRHS() ? CS->getRHS()->EvaluateKnownConstInt(Info.Ctx)
   : LHS;
-if (LHS <= Value && Value <= RHS) {
+if (Value.getBitWidth() == LHS.getBitWidth() &&
+Value.getBitWidth() == RHS.getBitWidth() && LHS <= Value &&
+Value <= RHS) {
   Found = SC;
   break;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -531,6 +531,9 @@
 - Fixed a failing assertion when applying an attribute to an anonymous union.
   The assertion was benign outside of asserts builds and would only fire in C++.
   (`#48512 _`).
+- Stop evaluating a constant expression if the condition expression which in 
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D153296#4449089 , @yronglin wrote:

> Can we both check `SS->getCond()->containsErrors()` ? Maybe it can avoid 
> bitint's effect. WDYT? @erichkeane @shafik

I'm a TOUCH uncomfortable in that this only fixes the 'current' issue, and is a 
little fragile perhaps?  The problem we have is that the 'Value' is the 
incorrect size, which I presume we can get to a couple of ways?  I'd like to 
make sure we fix as many of those problems as possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Can we both check `SS->getCond()->containsErrors()` ? Maybe it can avoid 
bitint's effect. WDYT? @erichkeane @shafik


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D153296#4446016 , @shafik wrote:

> In D153296#612 , @erichkeane 
> wrote:
>
>> So I think I'm pretty confident that the only time we would call 
>> `EvaluateDependentExpr` is when we are in an error condition, so I'm 
>> convinced the fix 'as is' is incorrect.  The check for noteSideEffect 
>> records that we HAVE a side effect, then returns if we are OK ignoring them 
>> right now.
>>
>> So since we are in a state where ignoring this error-case is acceptable, I 
>> think returning early there is incorrect as well, at least from a 'code 
>> correctness' (even if there isn't a reproducer that would matter?).  I think 
>> we're in a case where we want to continue in order to ensure we go through 
>> the entire flow, so I THINK we should treat this as 'we have a value we 
>> don't know, so its just not found', and should fall into the check on 5019 
>> (unless of course, there is a 'default' option!).
>>
>> So I think that we should be checking if `Value` is valid right after the 
>> default check, which lets us fall into the 'default' branch and get 
>> additional diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?
>
> I do see what you are saying but I think that we have similar cases where 
> without a value the next step is impossible to do for example in 
> `EvaluateStmt` the `case Stmt::ReturnStmtClass:` case:
>
>   case Stmt::ReturnStmtClass: {
>  const Expr *RetExpr = cast(S)->getRetValue();
>  FullExpressionRAII Scope(Info);
>  if (RetExpr && RetExpr->isValueDependent()) {
>EvaluateDependentExpr(RetExpr, Info);
>// We know we returned, but we don't know what the value is.
>return ESR_Failed;
>  }
>
> We can't return a value we can't calculate right? and here as well for the 
> `Stmt::DoStmtClass` case
>
>   if (DS->getCond()->isValueDependent()) {
>  EvaluateDependentExpr(DS->getCond(), Info);
>  // Bailout as we don't know whether to keep going or terminate the loop.
>  return ESR_Failed;
>}
>
> This case feels the same as the two above, we can't calculate the jump for 
> the switch if we can't calculate the value.
>
> CC @rsmith

In the return-case I think it makes sense to skip out right away, I'm less 
convinced in the 'doStmt' case.  Either way, I think we can get a somewhat 
'better' diagnostic by continuing here, which is my point.  We COULD start 
essentially compling with error-limit=1, but there is value to continuing when 
we can.  And in this case, I think it is logical to continue.

In D153296#4446662 , @yronglin wrote:

> In D153296#612 , @erichkeane 
> wrote:
>
>> So I think I'm pretty confident that the only time we would call 
>> `EvaluateDependentExpr` is when we are in an error condition, so I'm 
>> convinced the fix 'as is' is incorrect.  The check for noteSideEffect 
>> records that we HAVE a side effect, then returns if we are OK ignoring them 
>> right now.
>>
>> So since we are in a state where ignoring this error-case is acceptable, I 
>> think returning early there is incorrect as well, at least from a 'code 
>> correctness' (even if there isn't a reproducer that would matter?).  I think 
>> we're in a case where we want to continue in order to ensure we go through 
>> the entire flow, so I THINK we should treat this as 'we have a value we 
>> don't know, so its just not found', and should fall into the check on 5019 
>> (unless of course, there is a 'default' option!).
>>
>> So I think that we should be checking if `Value` is valid right after the 
>> default check, which lets us fall into the 'default' branch and get 
>> additional diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?
>
> Erich, do you mean we do a modification like this？If I'm not misunderstand, I 
> think this looks good to me, we can get more diagnostics.
>
>   diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
>   index f1f89122d4cc..967695c61df5 100644
>   --- a/clang/lib/AST/ExprConstant.cpp
>   +++ b/clang/lib/AST/ExprConstant.cpp
>   @@ -4984,8 +4984,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult 
> , EvalInfo ,
>  return ESR_Failed;
>if (SS->getCond()->isValueDependent()) {
>  // Stop evaluate if condition expression contains errors.
>   -  if (SS->getCond()->containsErrors() ||
>   -  !EvaluateDependentExpr(SS->getCond(), Info))
>   +  if (!EvaluateDependentExpr(SS->getCond(), Info))
>return ESR_Failed;
>} else {
>  if (!EvaluateInteger(SS->getCond(), Value, Info))
>   @@ -4995,6 +4994,8 @@ static EvalStmtResult EvaluateSwitch(StmtResult 
> , EvalInfo ,
>  return ESR_Failed;
>  }
>
>   +  bool CondHasSideEffects = SS->getCond()->HasSideEffects(Info.getCtx());
>   +
>  // Find the switch case 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D153296#612 , @erichkeane 
wrote:

> So I think I'm pretty confident that the only time we would call 
> `EvaluateDependentExpr` is when we are in an error condition, so I'm 
> convinced the fix 'as is' is incorrect.  The check for noteSideEffect records 
> that we HAVE a side effect, then returns if we are OK ignoring them right now.
>
> So since we are in a state where ignoring this error-case is acceptable, I 
> think returning early there is incorrect as well, at least from a 'code 
> correctness' (even if there isn't a reproducer that would matter?).  I think 
> we're in a case where we want to continue in order to ensure we go through 
> the entire flow, so I THINK we should treat this as 'we have a value we don't 
> know, so its just not found', and should fall into the check on 5019 (unless 
> of course, there is a 'default' option!).
>
> So I think that we should be checking if `Value` is valid right after the 
> default check, which lets us fall into the 'default' branch and get 
> additional diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?

Erich, do you mean we do a modification like this？If I'm not misunderstand, I 
think this looks good to me, we can get more diagnostics.

  diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
  index f1f89122d4cc..967695c61df5 100644
  --- a/clang/lib/AST/ExprConstant.cpp
  +++ b/clang/lib/AST/ExprConstant.cpp
  @@ -4984,8 +4984,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult 
, EvalInfo ,
 return ESR_Failed;
   if (SS->getCond()->isValueDependent()) {
 // Stop evaluate if condition expression contains errors.
  -  if (SS->getCond()->containsErrors() ||
  -  !EvaluateDependentExpr(SS->getCond(), Info))
  +  if (!EvaluateDependentExpr(SS->getCond(), Info))
   return ESR_Failed;
   } else {
 if (!EvaluateInteger(SS->getCond(), Value, Info))
  @@ -4995,6 +4994,8 @@ static EvalStmtResult EvaluateSwitch(StmtResult 
, EvalInfo ,
 return ESR_Failed;
 }
   
  +  bool CondHasSideEffects = SS->getCond()->HasSideEffects(Info.getCtx());
  +
 // Find the switch case corresponding to the value of the condition.
 // FIXME: Cache this lookup.
 const SwitchCase *Found = nullptr;
  @@ -5009,7 +5010,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult 
, EvalInfo ,
   APSInt LHS = CS->getLHS()->EvaluateKnownConstInt(Info.Ctx);
   APSInt RHS = CS->getRHS() ? CS->getRHS()->EvaluateKnownConstInt(Info.Ctx)
 : LHS;
  -if (LHS <= Value && Value <= RHS) {
  +if (!CondHasSideEffects && LHS <= Value && Value <= RHS) {
 Found = SC;
 break;
   }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D153296#612 , @erichkeane 
wrote:

> So I think I'm pretty confident that the only time we would call 
> `EvaluateDependentExpr` is when we are in an error condition, so I'm 
> convinced the fix 'as is' is incorrect.  The check for noteSideEffect records 
> that we HAVE a side effect, then returns if we are OK ignoring them right now.
>
> So since we are in a state where ignoring this error-case is acceptable, I 
> think returning early there is incorrect as well, at least from a 'code 
> correctness' (even if there isn't a reproducer that would matter?).  I think 
> we're in a case where we want to continue in order to ensure we go through 
> the entire flow, so I THINK we should treat this as 'we have a value we don't 
> know, so its just not found', and should fall into the check on 5019 (unless 
> of course, there is a 'default' option!).
>
> So I think that we should be checking if `Value` is valid right after the 
> default check, which lets us fall into the 'default' branch and get 
> additional diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?

I do see what you are saying but I think that we have similar cases where 
without a value the next step is impossible to do for example in `EvaluateStmt` 
the `case Stmt::ReturnStmtClass:` case:

  case Stmt::ReturnStmtClass: {
 const Expr *RetExpr = cast(S)->getRetValue();
 FullExpressionRAII Scope(Info);
 if (RetExpr && RetExpr->isValueDependent()) {
   EvaluateDependentExpr(RetExpr, Info);
   // We know we returned, but we don't know what the value is.
   return ESR_Failed;
 }

We can't return a value we can't calculate right? and here as well for the 
`Stmt::DoStmtClass` case

  if (DS->getCond()->isValueDependent()) {
 EvaluateDependentExpr(DS->getCond(), Info);
 // Bailout as we don't know whether to keep going or terminate the loop.
 return ESR_Failed;
   }

This case feels the same as the two above, we can't calculate the jump for the 
switch if we can't calculate the value.

CC @rsmith


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So I think I'm pretty confident that the only time we would call 
`EvaluateDependentExpr` is when we are in an error condition, so I'm convinced 
the fix 'as is' is incorrect.  The check for noteSideEffect records that we 
HAVE a side effect, then returns if we are OK ignoring them right now.

So since we are in a state where ignoring this error-case is acceptable, I 
think returning early there is incorrect as well, at least from a 'code 
correctness' (even if there isn't a reproducer that would matter?).  I think 
we're in a case where we want to continue in order to ensure we go through the 
entire flow, so I THINK we should treat this as 'we have a value we don't know, 
so its just not found', and should fall into the check on 5019 (unless of 
course, there is a 'default' option!).

So I think that we should be checking if `Value` is valid right after the 
default check, which lets us fall into the 'default' branch and get additional 
diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4989
 if (SS->getCond()->isValueDependent()) {
   if (!EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

shafik wrote:
> As far as I can tell `Value` will still not be set if we don't return here 
> and we will still crash when we attempt to compare `Value` below:
> 
> ```
> LHS <= Value && Value <= RHS
> ```
I don't understand the comment?   We're returning 'failed', aren't we?  Except 
now `EvaluateDependentExpr`is where the failure is coming from?



Comment at: clang/lib/AST/ExprConstant.cpp:4893
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;

yronglin wrote:
> yronglin wrote:
> > erichkeane wrote:
> > > I'd probably suggest `E->containsErrors()` instead, to cover cases where 
> > > we're not the 'root' of a recovery expr?  So something like:
> > > 
> > > `switch(func_call(unknown_value))`
> > > 
> > > should create a dependent call expr, but would still contain errors.
> > Thanks! Use `E->containsErrors()` and added into release note.
> Seems check error inside `EvaluateDependentExpr` will missing diagnostic 
> messages.
> 
> This case was introduced in D84637
> ```
> constexpr int test5() { // expected-error {{constexpr function never produce}}
>   for (;; a++); // expected-error {{use of undeclared identifier}}  \
>expected-note {{constexpr evaluation hit maximum step 
> limit; possible infinite loop?}}
>   return 1;
> }
> ```
> ```
> ./main.cpp:2:11: error: use of undeclared identifier 'a'
> 2 |   for (;; a++); // expected-error {{use of undeclared identifier}}  \
>   |   ^
> 1 error generated.
> ```
> But I think the `infinite loop` diagnostic is unnecessary, should we update 
> the test case? WDYT?
Huh... thats interesting.  The 'Info.noteSideEffect' is sufficient to do that?  
Looking closer, I wonder if this is the wrong fix and his original idea was 
better.  It seems that this already has a 'containsError' check at the end, and 
should if it doesn't have side effects.  

I was hoping to have the problem generalized, but I think I was wrong, and we 
should go back to just fixing the 'switch' statements.



Comment at: clang/lib/AST/ExprConstant.cpp:4988
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

shafik wrote:
> yronglin wrote:
> > erichkeane wrote:
> > > It seems to me that the 'better' solution is to make 
> > > EvaluateDependentExpr (or one of its children) be RecoveryExpr aware, and 
> > > result in a failed value instead.  That way we get this 'fix' for more 
> > > than just switch statements.
> > Thanks for your review!
> Erich so there are places in `ExprConstant.cpp` where if we  
> `isValueDependent()` we bail out like in the `Stmt::ReturnStmtClass` case 
> inside `EvaluateStmt1()`. The gist I get from the comment there is 
> 
> ```cpp
> // We know we returned, but we don't know what the value is.
> ```
> 
> Is that not correct or does it depend on each specific case?
TBH, i don't have a good idea of that.  I spent a little time digging here, but 
the constant evaluator isn't my forte.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4989
 if (SS->getCond()->isValueDependent()) {
   if (!EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

As far as I can tell `Value` will still not be set if we don't return here and 
we will still crash when we attempt to compare `Value` below:

```
LHS <= Value && Value <= RHS
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4988
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

yronglin wrote:
> erichkeane wrote:
> > It seems to me that the 'better' solution is to make EvaluateDependentExpr 
> > (or one of its children) be RecoveryExpr aware, and result in a failed 
> > value instead.  That way we get this 'fix' for more than just switch 
> > statements.
> Thanks for your review!
Erich so there are places in `ExprConstant.cpp` where if we  
`isValueDependent()` we bail out like in the `Stmt::ReturnStmtClass` case 
inside `EvaluateStmt1()`. The gist I get from the comment there is 

```cpp
// We know we returned, but we don't know what the value is.
```

Is that not correct or does it depend on each specific case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4893
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;

yronglin wrote:
> erichkeane wrote:
> > I'd probably suggest `E->containsErrors()` instead, to cover cases where 
> > we're not the 'root' of a recovery expr?  So something like:
> > 
> > `switch(func_call(unknown_value))`
> > 
> > should create a dependent call expr, but would still contain errors.
> Thanks! Use `E->containsErrors()` and added into release note.
Seems check error inside `EvaluateDependentExpr` will missing diagnostic 
messages.

This case was introduced in D84637
```
constexpr int test5() { // expected-error {{constexpr function never produce}}
  for (;; a++); // expected-error {{use of undeclared identifier}}  \
   expected-note {{constexpr evaluation hit maximum step limit; 
possible infinite loop?}}
  return 1;
}
```
```
./main.cpp:2:11: error: use of undeclared identifier 'a'
2 |   for (;; a++); // expected-error {{use of undeclared identifier}}  \
  |   ^
1 error generated.
```
But I think the `infinite loop` diagnostic is unnecessary, should we update the 
test case? WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D153296#4442363 , @erichkeane 
wrote:

> Just a rewording of the message, else LGTM.

Thanks a lot for you're review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 533738.
yronglin marked an inline comment as done.
yronglin added a comment.

Update wording.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/switch.cpp


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is 
not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4889,6 +4889,9 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
+  // Stop evaluating if expression contains errors.
+  if (E->containsErrors())
+return false;
   if (Info.noteSideEffect())
 return true;
   assert(E->containsErrors() && "valid value-dependent expression should never 
"
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -520,6 +520,9 @@
   type by the default argument promotions, and thus this is UB. Clang's
   behavior now matches GCC's behavior in C++.
   (`#38717 _`).
+- Stop evaluating a constant expression if the condition expression which in 
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4889,6 +4889,9 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
+  // Stop evaluating if expression contains errors.
+  if (E->containsErrors())
+return false;
   if (Info.noteSideEffect())
 return true;
   assert(E->containsErrors() && "valid value-dependent expression should never "
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -520,6 +520,9 @@
   type by the default argument promotions, and thus this is UB. Clang's
   behavior now matches GCC's behavior in C++.
   (`#38717 _`).
+- Stop evaluating a constant expression if the condition expression which in 
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4893
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;

erichkeane wrote:
> I'd probably suggest `E->containsErrors()` instead, to cover cases where 
> we're not the 'root' of a recovery expr?  So something like:
> 
> `switch(func_call(unknown_value))`
> 
> should create a dependent call expr, but would still contain errors.
Thanks! Use `E->containsErrors()` and added into release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Just a rewording of the message, else LGTM.




Comment at: clang/docs/ReleaseNotes.rst:523
   (`#38717 _`).
+- Stop evaluate constant expression if the condition expression which in 
+  switch statement contains errors.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 533735.
yronglin marked an inline comment as done.
yronglin added a comment.

Add ReleaseNotes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/switch.cpp


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is 
not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4889,6 +4889,9 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
+  // Stop evaluate if expression contains errors.
+  if (E->containsErrors())
+return false;
   if (Info.noteSideEffect())
 return true;
   assert(E->containsErrors() && "valid value-dependent expression should never 
"
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -520,6 +520,9 @@
   type by the default argument promotions, and thus this is UB. Clang's
   behavior now matches GCC's behavior in C++.
   (`#38717 _`).
+- Stop evaluate constant expression if the condition expression which in 
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4889,6 +4889,9 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
+  // Stop evaluate if expression contains errors.
+  if (E->containsErrors())
+return false;
   if (Info.noteSideEffect())
 return true;
   assert(E->containsErrors() && "valid value-dependent expression should never "
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -520,6 +520,9 @@
   type by the default argument promotions, and thus this is UB. Clang's
   behavior now matches GCC's behavior in C++.
   (`#38717 _`).
+- Stop evaluate constant expression if the condition expression which in 
+  switch statement contains errors.
+  (`#63453 _`)
 
 Bug Fixes to Compiler Builtins
 ^^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Also needs a release note.




Comment at: clang/lib/AST/ExprConstant.cpp:4893
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;

I'd probably suggest `E->containsErrors()` instead, to cover cases where we're 
not the 'root' of a recovery expr?  So something like:

`switch(func_call(unknown_value))`

should create a dependent call expr, but would still contain errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4988
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

erichkeane wrote:
> It seems to me that the 'better' solution is to make EvaluateDependentExpr 
> (or one of its children) be RecoveryExpr aware, and result in a failed value 
> instead.  That way we get this 'fix' for more than just switch statements.
Thanks for your review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 533714.
yronglin added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/switch.cpp


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is 
not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4889,6 +4889,9 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;
   if (Info.noteSideEffect())
 return true;
   assert(E->containsErrors() && "valid value-dependent expression should never 
"


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4889,6 +4889,9 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) {
   assert(E->isValueDependent());
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;
   if (Info.noteSideEffect())
 return true;
   assert(E->containsErrors() && "valid value-dependent expression should never "
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4988
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

It seems to me that the 'better' solution is to make EvaluateDependentExpr (or 
one of its children) be RecoveryExpr aware, and result in a failed value 
instead.  That way we get this 'fix' for more than just switch statements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 532853.
yronglin added a comment.

Fix test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/switch.cpp


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is 
not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4983,7 +4983,9 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
+  // Stop evaluate if condition expression contains errors.
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;
 } else {
   if (!EvaluateInteger(SS->getCond(), Value, Info))


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
@@ -146,3 +147,17 @@
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is not an integral constant expression}}
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4983,7 +4983,9 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
+  // Stop evaluate if condition expression contains errors.
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;
 } else {
   if (!EvaluateInteger(SS->getCond(), Value, Info))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Oops, compiler explorer website seems crashed, Once it's recovery, I'll append 
a link.

  :2:13: error: use of undeclared identifier 'f'
  2 | switch (f) {
| ^
  clang++: /root/llvm-project/llvm/lib/Support/APInt.cpp:282: int 
llvm::APInt::compareSigned(const llvm::APInt&) const: Assertion `BitWidth == 
RHS.BitWidth && "Bit widths must be same for comparison"' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.Program arguments: 
/opt/compiler-explorer/clang-assertions-trunk/bin/clang++ -gdwarf-4 -g -o 
/app/output.s -mllvm --x86-asm-syntax=intel -S 
--gcc-toolchain=/opt/compiler-explorer/gcc-snapshot -fcolor-diagnostics 
-fno-crash-diagnostics -std=c++20 
  1.:11:1: current parser token 'static_assert'
  2.:1:36: parsing function body 'foo'
   #0 0x55cfeaf267ff llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x3c3a7ff)
   #1 0x55cfeaf2456c llvm::sys::CleanupOnSignal(unsigned long) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x3c3856c)
   #2 0x55cfeae6e528 CrashRecoverySignalHandler(int) 
CrashRecoveryContext.cpp:0:0
   #3 0x7f6bd7059420 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
   #4 0x7f6bd6b2600b raise (/lib/x86_64-linux-gnu/libc.so.6+0x4300b)
   #5 0x7f6bd6b05859 abort (/lib/x86_64-linux-gnu/libc.so.6+0x22859)
   #6 0x7f6bd6b05729 (/lib/x86_64-linux-gnu/libc.so.6+0x22729)
   #7 0x7f6bd6b16fd6 (/lib/x86_64-linux-gnu/libc.so.6+0x33fd6)
   #8 0x55cfeae461cd llvm::APInt::compareSigned(llvm::APInt const&) const 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x3b5a1cd)
   #9 0x55cfee84a705 EvaluateStmt((anonymous namespace)::StmtResult&, 
(anonymous namespace)::EvalInfo&, clang::Stmt const*, clang::SwitchCase const*) 
(.part.0) ExprConstant.cpp:0:0
  #10 0x55cfee84a25f EvaluateStmt((anonymous namespace)::StmtResult&, 
(anonymous namespace)::EvalInfo&, clang::Stmt const*, clang::SwitchCase const*) 
(.part.0) ExprConstant.cpp:0:0
  #11 0x55cfee84e961 HandleFunctionCall(clang::SourceLocation, 
clang::FunctionDecl const*, (anonymous namespace)::LValue const*, 
llvm::ArrayRef, (anonymous namespace)::CallRef, clang::Stmt 
const*, (anonymous namespace)::EvalInfo&, clang::APValue&, (anonymous 
namespace)::LValue const*) ExprConstant.cpp:0:0
  #12 0x55cfee8918d8 
clang::Expr::isPotentialConstantExpr(clang::FunctionDecl const*, 
llvm::SmallVectorImpl>&) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x75a58d8)
  #13 0x55cfedaae4de 
clang::Sema::CheckConstexprFunctionDefinition(clang::FunctionDecl const*, 
clang::Sema::CheckConstexprKind) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x67c24de)
  #14 0x55cfeda05350 clang::Sema::ActOnFinishFunctionBody(clang::Decl*, 
clang::Stmt*, bool) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x6719350)
  #15 0x55cfed77b2dd 
clang::Parser::ParseFunctionStatementBody(clang::Decl*, 
clang::Parser::ParseScope&) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x648f2dd)
  #16 0x55cfed6a5dc1 
clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, 
clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b9dc1)
  #17 0x55cfed6cccd0 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, 
clang::DeclaratorContext, clang::ParsedAttributes&, clang::SourceLocation*, 
clang::Parser::ForRangeInit*) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63e0cd0)
  #18 0x55cfed699661 
clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, 
clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63ad661)
  #19 0x55cfed699f1f 
clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, 
clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) 
(.part.0) Parser.cpp:0:0
  #20 0x55cfed6a08c1 
clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, 
clang::ParsedAttributes&, clang::ParsingDeclSpec*) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b48c1)
  #21 0x55cfed6a1236 
clang::Parser::ParseTopLevelDecl(clang::OpaquePtr&, 
clang::Sema::ModuleImportState&) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b5236)
  #22 0x55cfed6a16d4 
clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr&, 
clang::Sema::ModuleImportState&) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b56d4)
  #23 0x55cfed694fca clang::ParseAST(clang::Sema&, bool, bool) 
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63a8fca)
  #24 0x55cfec1997d8 

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin created this revision.
Herald added a project: All.
yronglin requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Signed-off-by: yronglin 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153296

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/switch.cpp


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -146,3 +146,15 @@
 }
 
 } // namespace EmptyEnum
+
+constexpr int foo(unsigned char c) {
+switch (f) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is 
not an integral constant expression}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4983,7 +4983,9 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
+  // Stop evaluate if condition expression contains errors.
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;
 } else {
   if (!EvaluateInteger(SS->getCond(), Value, Info))


Index: clang/test/SemaCXX/switch.cpp
===
--- clang/test/SemaCXX/switch.cpp
+++ clang/test/SemaCXX/switch.cpp
@@ -146,3 +146,15 @@
 }
 
 } // namespace EmptyEnum
+
+constexpr int foo(unsigned char c) {
+switch (f) { // expected-error {{use of undeclared identifier}}
+case 0:
+return 7;
+default:
+break;
+}
+return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is not an integral constant expression}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4983,7 +4983,9 @@
 !EvaluateDecl(Info, SS->getConditionVariable()))
   return ESR_Failed;
 if (SS->getCond()->isValueDependent()) {
-  if (!EvaluateDependentExpr(SS->getCond(), Info))
+  // Stop evaluate if condition expression contains errors.
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;
 } else {
   if (!EvaluateInteger(SS->getCond(), Value, Info))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits