Page MenuHomePhabricator

analyze all kinds of expressions for integer overflow
AcceptedPublic

Authored by nlewycky on Apr 23 2017, 8:06 PM.

Details

Reviewers
rsmith
Summary

Remove clang::Sema::CheckForIntOverflow(E) by calling into E->EvaluateForOverflow instead. CheckForIntOverflow implemented a whitelist of top-level expressions to check, currently BinaryOperator, InitListExpr and ObjCBoxedExpr.

Two changes are made to avoid regression with the existing test suite.
test/SemaCXX/eval-sizeof-dependent-type.cpp has an example of a value-dependent InitListExpr. Handle value-dependent initializers in init list exprs.
test/Sema/integer-overflow.c tests that initializers are checked for overflow, which was conditionally disabled for performance. Allow checking for overflow of init list exprs in the performance check.

Diff Detail

Event Timeline

nlewycky created this revision.Apr 23 2017, 8:06 PM
rsmith accepted this revision.Apr 26 2017, 11:56 PM

If we're now catching integer overflow in more cases, please add some relevant testcases. If this is a pure refactoring that enables those additional diagnostics to be produced in future, then this is fine as-is. (For the benefit of other people looking at this, the test changes here are caused by a pre-existing bug: the notes for why a reference is not usable in a constant expression are only produced if we happen to produce diagnostics the first time we try to evaluate them.)

I have an unsubstantiated performance concern: we've seen this overflow checking having a visible effect on compile times in LNT before, but I'm happy to let you figure out whether there's anything else you can do to measure this before bouncing it off the LNT bots. (And any cases we're checking with this patch but not without it seem to be arbitrary oversights rather than something principled.)

This revision is now accepted and ready to land.Apr 26 2017, 11:56 PM
nlewycky updated this revision to Diff 97178.Apr 28 2017, 6:10 PM
nlewycky edited the summary of this revision. (Show Details)

Rebase. Now that ObjCBoxedExpr change is in, we can remove Sema::CheckForIntOverflow entirely.

If we're now catching integer overflow in more cases, please add some relevant testcases.

Both more and fewer. More because we no longer have a whitelist of three kinds of expressions that we recurse into. Fewer because we no longer call IgnoreParenCasts() on the full-expression so "(void)(4608 * 1024 * 1024);" used to get a warning but now doesn't. The plan to fix this is a patch to call EvaluateIgnoredValue on non-literal types, which will get us that warning back.

I've added two cases to test/Sema/integer-overflow.c and changed one to demonstrate this patch. It previously had an additional + on it just to trigger the int overflow checking.

I have an unsubstantiated performance concern: we've seen this overflow checking having a visible effect on compile times in LNT before

I haven't observed slowdown like I did with my previous attempt at this change in D31839, but yes we may need to back this patch out if it causes problems.

nlewycky updated this revision to Diff 97179.Apr 28 2017, 6:36 PM

[No changes, just full context this time.]