Page MenuHomePhabricator

Catch more cases when diagnosing integer-constant-expression overflows.
ClosedPublic

Authored by jmagee on Jul 30 2013, 12:34 PM.

Details

Summary

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

Diff Detail

Event Timeline

jmagee updated this revision to Diff 19020.Jan 29 2015, 7:22 PM

Refreshing a _very_ old patch.

This patch catches more cases of overflow in integer constant expressions and is
an improvement over the current state. That said, the robustness of
EvaluateForIntOverflow can be improved by allowing it to handle more than just
BinaryOperator expression (which this patch does not address.)

The changes to catch additional cases of overflow caused a problem in
SemaChecking when synthesizing the imaginary part of complex numbers.
Specifically, it was necessary to handle cases where the LHS is not complex and
also cases where the binary operator is an AssignmentOp.

I think the changes to the complex synthesis code is potentially better split
of as a separate patch, but those changes are only required if the integer
overflow changes go in. (Similarly, the integer overflow patch needs the
complex changes to get a clean test run.)
-> In short, I'm happy to split them apart, but I included
everything as one patch so the context of why I'm proposing the complex related
changes is clearer.

Thanks,

  • Josh
asl added a subscriber: asl.Jan 30 2015, 8:44 AM
rsmith added a subscriber: rsmith.Jan 30 2015, 3:07 PM
rsmith added inline comments.
lib/AST/ExprConstant.cpp
6837–6838

This comment seems redundant: it's obvious why keepEvaluatingAfterFailure should cause us to keep evaluating after a failure ;-)

6857–6858

Unnecessary nesting here, which makes the code harder to read; switch to

if (E->isAssignmentOp()) {
  // handle it
} else if (LHSTy->isRealFloatingType()) {
  // as currently
6862

You should evaluate the LHS as an lvalue in this case; there could be integer overflow within it. Eg, arr[A * B] = 3i;

6864–6869

These changes look unrelated to your patch: we only get here for a non-assignment-op; you're only changing how we handle assignments.

6865–6871

Please factor out this process rather than duplicating it here and above.

6873–6902

This code certainly won't do the right thing if we reach it for an assignment operator. Should you instead be setting LHSOK to false for an assignment op?

lib/Sema/SemaChecking.cpp
7028–7039 ↗(On Diff #19020)

The original design for the checker was that it would recursively evaluate the entire expression, including all subexpressions. There is no principled reason for it to only apply it when the top-level expression is a BinaryOperator (or a CallExpr); IIRC that restriction was applied over (possibly-misplaced) performance concerns. So: I'd like to see some performance measurements showing that either we can, or we cannot, simply run the checker over all expressions. Let's not make an arbitrary restriction more complex for no reason...

Also, this appears orthogonal to the other changes in the patch; it would be better to separate it, especially since it has performance implications.

jmagee added inline comments.Feb 3 2015, 3:55 PM
lib/AST/ExprConstant.cpp
6864–6869

Hmm... Quite right; I'll remove this (and the similar treatment for the RHS).

6873–6902

Yes, you are absolutely correct. I am surprised that I managed to run the regression test without something blowing up from this. Setting LHSOK to false seems like the best thing to do. (Alternatively I could handle assignment in the above, but in that case I would just return false, so setting LHSOK seems more natural.)

lib/Sema/SemaChecking.cpp
7028–7039 ↗(On Diff #19020)

Gotcha - I'll drop these changes. I haven't done any performance measurements here yet, so I agree with holding off on fiddling with the restrictions until the impact of running the checker over all expressions is better understood.

jmagee updated this revision to Diff 19280.Feb 3 2015, 4:01 PM

Thanks for the review, Richard. Here is a new patch that hopefully addresses your feedback.
I'm honestly a bit uncertain about how I'm handling the LHS of an assignment. Evaluating it as an LValue makes sense because, as you pointed out, there may be overflow there. However I don't see anything particularly useful that can be done with the result, particularly since I go on to set LHSOK to false. That said, the code looks awkward enough that it feels like I could be overlooking something here.

I also added 2 lines to an existing complex test that would trigger this assertion if it is reached on assignment operator. It's not terribly interesting, but it would have caught a mistake in the previous version of the patch at least.

rsmith accepted this revision.Feb 3 2015, 4:29 PM
rsmith added a reviewer: rsmith.

LGTM, thanks!

lib/AST/ExprConstant.cpp
6851

Dead store; remove the LHSOK = .

This revision is now accepted and ready to land.Feb 3 2015, 4:29 PM
This revision was automatically updated to reflect the committed changes.