[PATCH] Catch more cases when diagnosing integer-constant-expression overflows.

[Joshua Magee]

At 1375227924 seconds past the Epoch, Richard Smith wrote:
> On Tue, Jul 30, 2013 at 12:38 PM, Josh Magee <
> Joshua_Magee at playstation.sony.com> wrote:
> 
> > The patch introduces the following changes:
> >  1) In Sema::CheckForIntOverflow, when checking for a BinaryOperator ignore
> >     any casts in addition to parentheses.
> >
> 
> The BinaryOperator check is highly dubious. It would be interesting to
> measure the performance cost of running the check on all expressions, not
> just binary operators.
> 

I tried running the check on all expressions but it caused eight regression
tests to fail (7 of them with assertions).  The failures all look different
enough that it seems unlikely that they have the same underlying cause.
Perhaps there is some implicit pre-condition of EvaluateForOverflow that it
only handles certain expressions, but if there is then it is not clear to me.
Certainly, it seems reasonable that it should be callable on any Expr without
asserting.

For reference, the failing tests were:
Clang :: CXX/class/class.local/p1-0x.cpp
Clang :: CodeGen/c11atomics-ios.c
Clang :: CodeGen/c11atomics.c
Clang :: CodeGenCXX/sel-address.mm
Clang :: Index/annotate-deep-statements.cpp
Clang :: SemaCXX/ast-print.cpp
Clang :: SemaCXX/implicit-exception-spec.cpp
Clang :: SemaOpenCL/init.cl

All but the Index test failed with assertions.

> >    Ignoring only parentheses was not enough because it missed cases like:
> >    x = 4608 * 1024 * 1024, which has an implicit cast:
> >     ImplicitCastExpr 0x5145858 'uint64_t':'unsigned long' <IntegralCast>
> >     `-BinaryOperator 0x5145830 'int' '*'
> >       |-BinaryOperator 0x51457e8 'int' '*'
> >       | |-IntegerLiteral 0x51457a8 'int' 4608
> >       | `-IntegerLiteral 0x51457c8 'int' 1024
> >       `-IntegerLiteral 0x5145810 'int' 1024
> >
> >  2) In Sema::CheckForIntOverflow, check for CallExprs and recursively
> > check for
> >     overflow on each argument.
> >
> 
> It would be better to just pass the whole expression to the checker; that
> was the original intent.
> 
> 

Unfortunately, passing the whole CallExpr does not catch overflow in the
arguments.  The problem is that the call to HandleFunctionCall() in
VisitCallExpr() is predicated on a call to CheckConstExprFunction:
 if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition) ||             
     !HandleFunctionCall(E->getExprLoc(), Definition, This, Args, Body,            
                         Info, Result))  

With the examples from my testcase, HandleFunctionCall never gets called because
CheckConstantExprFunction will return false.  I can work around that by forcing
a call to EvaluateArgs even if CheckConstantExprFunction fails, but I am not
sure if that is the right solution.



> >   This handles cases like f1(4096 * 1024 * 1024).
> >
> >  3) In IntExprEvaluator::VisitBinaryOperator(), bail out on AssignmentOps
> >    _after_ running the DataRecursiveIntBinOpEvaluator. In fact, I am not
> > sure
> >    if check here for AssignmentOps is really necessary.  It seems like it
> > is an
> >    early-exit optimization.
> 
> 
> Please only do this if Info.keepEvaluatingAfterFailure() is true.
> Otherwise, we want to bail out as early as we can once we find that an
> expression cannot be evaluated.
> 

Will do.


Jumping up a level:  Between the failures when checking all expressions and
handling CallExpr Args, there looks like a decent amount of further
investigation and work required to do things correctly (Any pointers or hints
from those with more experience in this area are welcome.)  In the interim,
would a subset of this patch be more palatable?  Specifically, the change from
IgnoreParens() to IgnoreParenCasts() catches quite a few more cases and is not
any worse than what is there now.
On the other hand, I understand if it is preferred to hold off until a
better solution is obtained.

Thoughts?

- Josh


> 
> >  Presumably it was thought that assignment
> >    operators would not have any integer expression operands that need
> >    evaluating here, however this is not the case in "x = y = 4096 * 1024 *
> > 1024"
> >    or "if ((y = 4096 * 1024 *1024))".  Running the
> > DataRecursiveIntBinOpEvaluator
> >    on AssignmentOps catches these cases.  Note, this change was only
> > needed to
> >    catch these cases in C mode.  In C++ mode an implicit LValueToRValue
> > cast was
> >    added which changed the path taken through semantic analysis.
> >
> >
> > Thanks,
> > Josh
> >
> > http://llvm-reviews.chandlerc.com/D1238
> >
> > Files:
> >   lib/AST/ExprConstant.cpp
> >   lib/Sema/SemaChecking.cpp
> >   test/Sema/integer-overflow.c
> >   test/SemaCXX/integer-overflow.cpp