[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL361050: Added an assertion to constant evaluation enty 
points that prohibits dependent… (authored by gribozavr, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61522?vs=198603=200062#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61522

Files:
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/lib/Sema/SemaOpenMP.cpp
  cfe/trunk/lib/Sema/SemaOverload.cpp

Index: cfe/trunk/lib/AST/Expr.cpp
===
--- cfe/trunk/lib/AST/Expr.cpp
+++ cfe/trunk/lib/AST/Expr.cpp
@@ -2974,6 +2974,9 @@
 
 bool Expr::isConstantInitializer(ASTContext , bool IsForRef,
  const Expr **Culprit) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
+
   // This function is attempting whether an expression is an initializer
   // which can be evaluated at compile-time. It very closely parallels
   // ConstExprEmitter in CGExprConstant.cpp; if they don't match, it
Index: cfe/trunk/lib/AST/ExprConstant.cpp
===
--- cfe/trunk/lib/AST/ExprConstant.cpp
+++ cfe/trunk/lib/AST/ExprConstant.cpp
@@ -11642,6 +11642,8 @@
 /// will be applied to the result.
 bool Expr::EvaluateAsRValue(EvalResult , const ASTContext ,
 bool InConstantContext) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   Info.InConstantContext = InConstantContext;
   return ::EvaluateAsRValue(this, Result, Ctx, Info);
@@ -11649,6 +11651,8 @@
 
 bool Expr::EvaluateAsBooleanCondition(bool ,
   const ASTContext ) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalResult Scratch;
   return EvaluateAsRValue(Scratch, Ctx) &&
  HandleConversionToBool(Scratch.Val, Result);
@@ -11656,18 +11660,25 @@
 
 bool Expr::EvaluateAsInt(EvalResult , const ASTContext ,
  SideEffectsKind AllowSideEffects) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   return ::EvaluateAsInt(this, Result, Ctx, AllowSideEffects, Info);
 }
 
 bool Expr::EvaluateAsFixedPoint(EvalResult , const ASTContext ,
 SideEffectsKind AllowSideEffects) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   return ::EvaluateAsFixedPoint(this, Result, Ctx, AllowSideEffects, Info);
 }
 
 bool Expr::EvaluateAsFloat(APFloat , const ASTContext ,
SideEffectsKind AllowSideEffects) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
+
   if (!getType()->isRealFloatingType())
 return false;
 
@@ -11681,6 +11692,9 @@
 }
 
 bool Expr::EvaluateAsLValue(EvalResult , const ASTContext ) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
+
   EvalInfo Info(Ctx, Result, EvalInfo::EM_ConstantFold);
 
   LValue LV;
@@ -11696,6 +11710,9 @@
 
 bool Expr::EvaluateAsConstantExpr(EvalResult , ConstExprUsage Usage,
   const ASTContext ) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
+
   EvalInfo::EvaluationMode EM = EvalInfo::EM_ConstantExpression;
   EvalInfo Info(Ctx, Result, EM);
   Info.InConstantContext = true;
@@ -11710,6 +11727,9 @@
 bool Expr::EvaluateAsInitializer(APValue , const ASTContext ,
  const VarDecl *VD,
 SmallVectorImpl ) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
+
   // FIXME: Evaluating initializers for large array and record types can cause
   // performance problems. Only do so in C++11 for now.
   if (isRValue() && (getType()->isArrayType() || getType()->isRecordType()) &&
@@ -11752,6 +11772,9 @@
 /// isEvaluatable - Call EvaluateAsRValue to see if this expression can be
 /// constant folded, but discard the result.
 bool Expr::isEvaluatable(const ASTContext , SideEffectsKind SEK) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
+
   EvalResult Result;
   return 

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Alexey Bataev via Phabricator via cfe-commits]

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

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

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

rsmith accepted this revision.
rsmith added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:6369
 // very difficult. Ideally, we should handle them more gracefully.
-if (!EIA->getCond()->EvaluateWithSubstitution(
+if (EIA->getCond()->isValueDependent() ||
+!EIA->getCond()->EvaluateWithSubstitution(

gribozavr wrote:
> rsmith wrote:
> > This is treating value-dependent `enable_if` conditions as having failed. 
> > Is that really appropriate? (When do we call this with value-depnedent 
> > `enable_if` attributes? I'd expect it to only be called after substitution)
> This test case in `llvm-project/clang/test/SemaCXX/enable_if.cpp` passes a 
> dependent condition:
> 
> ```
> void h(int);
> template  void outer() {
>   void local_function() __attribute__((enable_if(::h(T()), "")));
>   local_function();
> };
> ```
> 
> According to https://reviews.llvm.org/D20130, it seems like it was decided to 
> document implementation details as specification, and say that `enable_if` is 
> evaluated during overload resolution, whenever that happens to happen.
Hm. Yeah, I suppose we disable `enable_if` functions if the condition can't be 
evaluated, and this is a "can't be evaluated" case in some sense. I don't like 
it (the right thing would be to treat this as a dependent overload resolution), 
but as a short-term fix for this patch I suppose it's the right local choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

@ABataev @rsmith Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaOverload.cpp:6369
 // very difficult. Ideally, we should handle them more gracefully.
-if (!EIA->getCond()->EvaluateWithSubstitution(
+if (EIA->getCond()->isValueDependent() ||
+!EIA->getCond()->EvaluateWithSubstitution(

rsmith wrote:
> This is treating value-dependent `enable_if` conditions as having failed. Is 
> that really appropriate? (When do we call this with value-depnedent 
> `enable_if` attributes? I'd expect it to only be called after substitution)
This test case in `llvm-project/clang/test/SemaCXX/enable_if.cpp` passes a 
dependent condition:

```
void h(int);
template  void outer() {
  void local_function() __attribute__((enable_if(::h(T()), "")));
  local_function();
};
```

According to https://reviews.llvm.org/D20130, it seems like it was decided to 
document implementation details as specification, and say that `enable_if` is 
evaluated during overload resolution, whenever that happens to happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

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

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:6369
 // very difficult. Ideally, we should handle them more gracefully.
-if (!EIA->getCond()->EvaluateWithSubstitution(
+if (EIA->getCond()->isValueDependent() ||
+!EIA->getCond()->EvaluateWithSubstitution(

This is treating value-dependent `enable_if` conditions as having failed. Is 
that really appropriate? (When do we call this with value-depnedent `enable_if` 
attributes? I'd expect it to only be called after substitution)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

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

In D61522#1494155 , @rsmith wrote:

> The right thing to check in all of these cases should be only 
> `isValueDependent()`. Every type-dependent expression should generally also 
> be value-dependent (because the type is part of the value), but 
> value-dependent exactly means "dependent in a way that prevents constant 
> evaluation", so that's all that we should be checking.


Done.  PTAL.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:5784
 Expr::EvalResult Result;
-if (CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext()))
+if (!CollapseLoopCountExpr->isValueDependent() &&
+!CollapseLoopCountExpr->isTypeDependent() &&

ABataev wrote:
> gribozavr wrote:
> > ABataev wrote:
> > > I would suggest to modify the code of this function if we cannot get the 
> > > value of the loops.
> > > ```
> > > if (CollapseLoopCountExpr->isValueDependent() || 
> > > CollapseLoopCountExpr->isTypeDependent() || 
> > > OrderedLoopCountExpr->isValueDependent() || 
> > > OrderedLoopCountExpr->isTypeDependent()) {
> > >   Built.clear(/* size */0);
> > >   return 1;
> > > }
> > > ```
> > > at the beginning of the function.
> > I tried doing that, and a lot of tests started crashing with:
> > 
> > llvm-project/clang/lib/Sema/SemaOpenMP.cpp:9024: clang::StmtResult 
> > clang::Sema::ActOnOpenMPTargetTeamsDistributeSimdDirective(ArrayRef >  *>, clang::Stmt *, clang::SourceLoc
> > ation, clang::SourceLocation, clang::Sema::VarsWithInheritedDSAType &): 
> > Assertion `(CurContext->isDependentContext() || B.builtAll()) && "omp 
> > target teams distribute simd loop exprs were not built"' failed.
> > 
> > Also, I wanted to note that if I were to make this change, then if 
> > `EvaluateAsInt` fails, we should apply the same recovery (`Built.clear(); 
> > return`).
> The just try to use `Built.clear(/* size */1);`, I assume it must fix the 
> problems. 
Yes, this works!  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 198603.
gribozavr added a comment.

Addressed review comments:

- Simplified error handling in SemaOpenMP,

- Only check isValueDependent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522

Files:
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp

Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6366,7 +6366,8 @@
 APValue Result;
 // FIXME: This doesn't consider value-dependent cases, because doing so is
 // very difficult. Ideally, we should handle them more gracefully.
-if (!EIA->getCond()->EvaluateWithSubstitution(
+if (EIA->getCond()->isValueDependent() ||
+!EIA->getCond()->EvaluateWithSubstitution(
 Result, Context, Function, llvm::makeArrayRef(ConvertedArgs)))
   return EIA;
 
@@ -9547,7 +9548,8 @@
 const FunctionDecl *FD) {
   for (auto *EnableIf : FD->specific_attrs()) {
 bool AlwaysTrue;
-if (!EnableIf->getCond()->EvaluateAsBooleanCondition(AlwaysTrue, Ctx))
+if (EnableIf->getCond()->isValueDependent() ||
+!EnableIf->getCond()->EvaluateAsBooleanCondition(AlwaysTrue, Ctx))
   return false;
 if (!AlwaysTrue)
   return false;
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5880,14 +5880,21 @@
   if (CollapseLoopCountExpr) {
 // Found 'collapse' clause - calculate collapse number.
 Expr::EvalResult Result;
-if (CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext()))
+if (!CollapseLoopCountExpr->isValueDependent() &&
+CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext())) {
   NestedLoopCount = Result.Val.getInt().getLimitedValue();
+} else {
+  Built.clear(/*size=*/1);
+  return 1;
+}
   }
   unsigned OrderedLoopCount = 1;
   if (OrderedLoopCountExpr) {
 // Found 'ordered' clause - calculate collapse number.
 Expr::EvalResult EVResult;
-if (OrderedLoopCountExpr->EvaluateAsInt(EVResult, SemaRef.getASTContext())) {
+if (!OrderedLoopCountExpr->isValueDependent() &&
+OrderedLoopCountExpr->EvaluateAsInt(EVResult,
+SemaRef.getASTContext())) {
   llvm::APSInt Result = EVResult.Val.getInt();
   if (Result.getLimitedValue() < NestedLoopCount) {
 SemaRef.Diag(OrderedLoopCountExpr->getExprLoc(),
@@ -5898,6 +5905,9 @@
 << CollapseLoopCountExpr->getSourceRange();
   }
   OrderedLoopCount = Result.getLimitedValue();
+} else {
+  Built.clear(/*size=*/1);
+  return 1;
 }
   }
   // This is helper routine for loop directives (e.g., 'for', 'simd',
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11100,6 +11100,8 @@
 /// will be applied to the result.
 bool Expr::EvaluateAsRValue(EvalResult , const ASTContext ,
 bool InConstantContext) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   Info.InConstantContext = InConstantContext;
   return ::EvaluateAsRValue(this, Result, Ctx, Info);
@@ -11107,6 +11109,8 @@
 
 bool Expr::EvaluateAsBooleanCondition(bool ,
   const ASTContext ) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalResult Scratch;
   return EvaluateAsRValue(Scratch, Ctx) &&
  HandleConversionToBool(Scratch.Val, Result);
@@ -4,18 +8,25 @@
 
 bool Expr::EvaluateAsInt(EvalResult , const ASTContext ,
  SideEffectsKind AllowSideEffects) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   return ::EvaluateAsInt(this, Result, Ctx, AllowSideEffects, Info);
 }
 
 bool Expr::EvaluateAsFixedPoint(EvalResult , const ASTContext ,
 SideEffectsKind AllowSideEffects) const {
+  assert(!isValueDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   return ::EvaluateAsFixedPoint(this, Result, Ctx, AllowSideEffects, Info);
 }
 
 bool Expr::EvaluateAsFloat(APFloat , const ASTContext ,
SideEffectsKind AllowSideEffects) const {
+  

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

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

rsmith added a comment.

The right thing to check in all of these cases should be only 
`isValueDependent()`. Every type-dependent expression should generally also be 
value-dependent (because the type is part of the value), but value-dependent 
exactly means "dependent in a way that prevents constant evaluation", so that's 
all that we should be checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5784
 Expr::EvalResult Result;
-if (CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext()))
+if (!CollapseLoopCountExpr->isValueDependent() &&
+!CollapseLoopCountExpr->isTypeDependent() &&

ABataev wrote:
> I would suggest to modify the code of this function if we cannot get the 
> value of the loops.
> ```
> if (CollapseLoopCountExpr->isValueDependent() || 
> CollapseLoopCountExpr->isTypeDependent() || 
> OrderedLoopCountExpr->isValueDependent() || 
> OrderedLoopCountExpr->isTypeDependent()) {
>   Built.clear(/* size */0);
>   return 1;
> }
> ```
> at the beginning of the function.
I tried doing that, and a lot of tests started crashing with:

llvm-project/clang/lib/Sema/SemaOpenMP.cpp:9024: clang::StmtResult 
clang::Sema::ActOnOpenMPTargetTeamsDistributeSimdDirective(ArrayRef, clang::Stmt *, clang::SourceLoc
ation, clang::SourceLocation, clang::Sema::VarsWithInheritedDSAType &): 
Assertion `(CurContext->isDependentContext() || B.builtAll()) && "omp target 
teams distribute simd loop exprs were not built"' failed.

Also, I wanted to note that if I were to make this change, then if 
`EvaluateAsInt` fails, we should apply the same recovery (`Built.clear(); 
return`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5784
 Expr::EvalResult Result;
-if (CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext()))
+if (!CollapseLoopCountExpr->isValueDependent() &&
+!CollapseLoopCountExpr->isTypeDependent() &&

gribozavr wrote:
> ABataev wrote:
> > I would suggest to modify the code of this function if we cannot get the 
> > value of the loops.
> > ```
> > if (CollapseLoopCountExpr->isValueDependent() || 
> > CollapseLoopCountExpr->isTypeDependent() || 
> > OrderedLoopCountExpr->isValueDependent() || 
> > OrderedLoopCountExpr->isTypeDependent()) {
> >   Built.clear(/* size */0);
> >   return 1;
> > }
> > ```
> > at the beginning of the function.
> I tried doing that, and a lot of tests started crashing with:
> 
> llvm-project/clang/lib/Sema/SemaOpenMP.cpp:9024: clang::StmtResult 
> clang::Sema::ActOnOpenMPTargetTeamsDistributeSimdDirective(ArrayRef  *>, clang::Stmt *, clang::SourceLoc
> ation, clang::SourceLocation, clang::Sema::VarsWithInheritedDSAType &): 
> Assertion `(CurContext->isDependentContext() || B.builtAll()) && "omp target 
> teams distribute simd loop exprs were not built"' failed.
> 
> Also, I wanted to note that if I were to make this change, then if 
> `EvaluateAsInt` fails, we should apply the same recovery (`Built.clear(); 
> return`).
The just try to use `Built.clear(/* size */1);`, I assume it must fix the 
problems. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> Can you add tests for the bugs you fixed? thanks

The bugs were detected by existing tests (those tests triggered the newly added 
assertions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5784
 Expr::EvalResult Result;
-if (CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext()))
+if (!CollapseLoopCountExpr->isValueDependent() &&
+!CollapseLoopCountExpr->isTypeDependent() &&

I would suggest to modify the code of this function if we cannot get the value 
of the loops.
```
if (CollapseLoopCountExpr->isValueDependent() || 
CollapseLoopCountExpr->isTypeDependent() || 
OrderedLoopCountExpr->isValueDependent() || 
OrderedLoopCountExpr->isTypeDependent()) {
  Built.clear(/* size */0);
  return 1;
}
```
at the beginning of the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added a comment.

Can you add tests for the bugs you fixed? thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61522



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

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Constant evaluator does not work on value-dependent or type-dependent
expressions.

Also fixed bugs uncovered by these assertions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61522

Files:
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp

Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6366,7 +6366,9 @@
 APValue Result;
 // FIXME: This doesn't consider value-dependent cases, because doing so is
 // very difficult. Ideally, we should handle them more gracefully.
-if (!EIA->getCond()->EvaluateWithSubstitution(
+if (EIA->getCond()->isValueDependent() ||
+EIA->getCond()->isTypeDependent() ||
+!EIA->getCond()->EvaluateWithSubstitution(
 Result, Context, Function, llvm::makeArrayRef(ConvertedArgs)))
   return EIA;
 
@@ -9547,7 +9549,9 @@
 const FunctionDecl *FD) {
   for (auto *EnableIf : FD->specific_attrs()) {
 bool AlwaysTrue;
-if (!EnableIf->getCond()->EvaluateAsBooleanCondition(AlwaysTrue, Ctx))
+if (EnableIf->getCond()->isValueDependent() ||
+EnableIf->getCond()->isTypeDependent() ||
+!EnableIf->getCond()->EvaluateAsBooleanCondition(AlwaysTrue, Ctx))
   return false;
 if (!AlwaysTrue)
   return false;
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5781,14 +5781,19 @@
   if (CollapseLoopCountExpr) {
 // Found 'collapse' clause - calculate collapse number.
 Expr::EvalResult Result;
-if (CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext()))
+if (!CollapseLoopCountExpr->isValueDependent() &&
+!CollapseLoopCountExpr->isTypeDependent() &&
+CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext()))
   NestedLoopCount = Result.Val.getInt().getLimitedValue();
   }
   unsigned OrderedLoopCount = 1;
   if (OrderedLoopCountExpr) {
 // Found 'ordered' clause - calculate collapse number.
 Expr::EvalResult EVResult;
-if (OrderedLoopCountExpr->EvaluateAsInt(EVResult, SemaRef.getASTContext())) {
+if (!OrderedLoopCountExpr->isValueDependent() &&
+!OrderedLoopCountExpr->isTypeDependent() &&
+OrderedLoopCountExpr->EvaluateAsInt(EVResult,
+SemaRef.getASTContext())) {
   llvm::APSInt Result = EVResult.Val.getInt();
   if (Result.getLimitedValue() < NestedLoopCount) {
 SemaRef.Diag(OrderedLoopCountExpr->getExprLoc(),
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -12452,7 +12452,7 @@
   CheckImplicitConversions(E, CheckLoc);
   if (!E->isInstantiationDependent())
 CheckUnsequencedOperations(E);
-  if (!IsConstexpr && !E->isValueDependent())
+  if (!IsConstexpr && !E->isValueDependent() && !E->isTypeDependent())
 CheckForIntOverflow(E);
   DiagnoseMisalignedMembers();
 }
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11088,6 +11088,8 @@
 /// will be applied to the result.
 bool Expr::EvaluateAsRValue(EvalResult , const ASTContext ,
 bool InConstantContext) const {
+  assert(!isValueDependent() && !isTypeDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   Info.InConstantContext = InConstantContext;
   return ::EvaluateAsRValue(this, Result, Ctx, Info);
@@ -11095,6 +11097,8 @@
 
 bool Expr::EvaluateAsBooleanCondition(bool ,
   const ASTContext ) const {
+  assert(!isValueDependent() && !isTypeDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalResult Scratch;
   return EvaluateAsRValue(Scratch, Ctx) &&
  HandleConversionToBool(Scratch.Val, Result);
@@ -11102,18 +11106,25 @@
 
 bool Expr::EvaluateAsInt(EvalResult , const ASTContext ,
  SideEffectsKind AllowSideEffects) const {
+  assert(!isValueDependent() && !isTypeDependent() &&
+ "Expression evaluator can't be called on a dependent expression.");
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
   return ::EvaluateAsInt(this, Result, Ctx, AllowSideEffects, Info);
 }
 
 bool Expr::EvaluateAsFixedPoint(EvalResult , const ASTContext ,