[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 <https://reviews.llvm.org/D153296#4446016>, @shafik wrote:

> In D153296#4444612 <https://reviews.llvm.org/D153296#4444612>, @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<ReturnStmt>(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 <https://reviews.llvm.org/D153296#4446662>, @yronglin wrote:

> In D153296#4444612 <https://reviews.llvm.org/D153296#4444612>, @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 &Result, EvalInfo &Info,
>          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 &Result, EvalInfo &Info,
>          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 &Result, EvalInfo &Info,
>        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;
>        }

Instead of the `CondHasSideEffects`, just check the validity of `Value` (which is a 1 bit zero value... WHICH is actually a valid value here unfortunately thanks to Bitint, so we'd have to check if the LHS/RHS bit size matches the Value here I think)..  We shouldn't be using the HasSideEffects, because that is overly dependent on the implementation of EvaluateDependentExpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296