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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
