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

[Richard Smith]

On Tue, Jul 30, 2013 at 12:38 PM, Josh Magee <
Joshua_Magee at playstation.sony.com> wrote:

> jmagee added you to the CC list for the revision "Catch more cases when
> diagnosing integer-constant-expression overflows.".
>
> Hi,
>
> This patch improves integer-constant-expression overflow diagnostics.
>
> Currently clang catches overflow in some cases:
>
> int i;
> ...
> switch (i) {
>   case 4096 * 1024 * 1024:  // Generates overflow warning
>   ...
> }
>
> if (4096 * 1024 * 1024) { // Generates overflow warning
> }
>
> But it misses other cases:
>
> uint64_t x = 4096 * 1024 * 1024; // no warning
>
> if ((x = 4096 * 1024 * 1024)) { // no warning
> }
>
> switch (i) {
>   case (uint64_t)(4096 * 1024 * 1024): // no warning
> ...
> }
>
> void f1(int);
> ...
> f1(4096 * 1024 * 1024); // no warning
>
>
> 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.


>    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.


>   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.


>  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
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130730/bfd975c4/attachment.html>