Fixes an assertion when IntRange processes a throw expression

[Mordante]

Summary

Fixes PR46484: Clang crash in clang/lib/Sema/SemaChecking.cpp:10028

[rsmith]

Did this only crash during error recovery before, or also for your void g() example? If we were only crashing in error recovery, that would suggest that error recovery was producing a bad AST, and perhaps we should be fixing this elsewhere.

clang/lib/Sema/SemaChecking.cpp

10164

Can we handle this in code that's specific to conditional expressions instead? Presumably somewhere higher up in the call graph, some code is assuming that it can recurse from a conditional expression to its subexpressions, and that assumption is wrong.

[Mordante]

I added void g() since that's valid code which also caused an assertion failure. So the issue isn't in the error recovery but in determining the required IntRange. It seems the code doesn't take http://eel.is/c++draft/expr.cond#2.1 into account.

clang/lib/Sema/SemaChecking.cpp

10164

I can take a look at it if you want. However I feel this fix is better. If the conditional doesn't throw it can properly evaluate the required IntRange. If it throws the range doesn't matter, therefore I didn't want to increment the required range.
Do you agree?
Should I add more comment to clarify the design decission?

[rsmith]

clang/lib/Sema/SemaChecking.cpp

10164

In order to get here, we need to have called functions that are documented as taking only integer types but passing them a void type. So the bug has already occurred before we got here.

10310–10320

It seems to me that the bug is here. GetExprRange is documented as "Pseudo-evaluate the given integer expression", but the true and false expressions here might not be integer expressions -- specifically, one of them could be of type void if it's a throw-expression. In that case, we should only call GetExprRange on the other expression. The expression range of the void-typed expression is irrelevant in this case, because that expression never returns.

[Mordante]

clang/lib/Sema/SemaChecking.cpp

10310–10320

Thanks for the hint I'll look into this solution.

[Mordante]

Addresses review comments.

[rsmith]

Thanks!

clang/lib/Sema/SemaChecking.cpp

if (!C.getLangOpts().CPlusPlus) {
         return IntRange(std::max(NumPositive + 1, NumNegative),
                         false/*NonNegative*/);
     }
 
     if (const auto *EIT = dyn_cast<ExtIntType>(T))
       return IntRange(EIT->getNumBits(), EIT->isUnsigned());
 
     const BuiltinType *BT = cast<BuiltinType>(T);
     assert(BT->isInteger());
 
     return IntRange(C.getIntWidth(QualType(T, 0)), BT->isUnsignedInteger());
   }
 
   /// Returns the "target" range of a canonical integral type, i.e.
   /// the range of values expressible in the type.
   ///
   /// This matches forValueOfCanonicalType except that enums have the
static IntRange GetExprRange(ASTContext &C, const Expr *E, unsigned MaxWidth,
   if (const auto *CO = dyn_cast<ConditionalOperator>(E)) {
     // If we can fold the condition, just take that operand.
     bool CondResult;
     if (CO->getCond()->EvaluateAsBooleanCondition(CondResult, C))
       return GetExprRange(C,
                           CondResult ? CO->getTrueExpr() : CO->getFalseExpr(),
                           MaxWidth, InConstantContext);
 
     // Otherwise, conservatively merge.
-    IntRange L =
-        GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext);
-    IntRange R =
-        GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext);
+    // GetExprRange requires an integer expression, but a throw expression
+    // results in a void type.
+    Expr *E = CO->getTrueExpr();
+    IntRange L = E->getType()->isVoidType()
+                     ? IntRange{0, true}
+                     : GetExprRange(C, E, MaxWidth, InConstantContext);
+    E = CO->getFalseExpr();
+    IntRange R = E->getType()->isVoidType()
+                     ? IntRange{0, true}
+                     : GetExprRange(C, E, MaxWidth, InConstantContext);
     return IntRange::join(L, R);
   }
 
   if (const auto *BO = dyn_cast<BinaryOperator>(E)) {
     switch (BO->getOpcode()) {
     case BO_Cmp:
       llvm_unreachable("builtin <=> should have class type");
 

clang/test/SemaCXX/conditional-expr.cpp

void f(bool b) {
     A &&r = b ? static_cast<A&&>(B()) : static_cast<A&&>(C());
   }
 
   struct D { A &&a; };
   void f_indirect(bool b) {
     D d = b ? D{B()} : D{C()};
   }
 }
+
+namespace PR46484 {
+// expected-error@+4{{expected ':'}}
+// expected-note@+3{{to match this '?'}}
+// expected-warning@+2{{variable 'b' is uninitialized}}
+// expected-error@+1 2 {{expected ';' after top level declarator}}
+int a long b = a = b ? throw 0 1
+
+void g() {
+  extern int a;
+  extern long b;
+  long c = a = b ? throw 0 : 1;
+  long d = a = b ? 1 : throw 0;
+  // expected-error@+1 {{assigning to 'int' from incompatible type 'void'}}
+  long e = a = b ? throw 0 : throw 1;
+}
+} // namespace PR46484