[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression

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

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10164
     const BuiltinType *BT = cast<BuiltinType>(T);
-    assert(BT->isInteger());
+    if (!BT->isInteger()) {
+      // This can happen in a conditional expression with a throw statement
----------------
Mordante wrote:
> rsmith wrote:
> > 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.
> 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?
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10317-10320
     IntRange L =
         GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext);
     IntRange R =
         GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85601