[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<RecoveryExpr>(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