On Tue, Jul 30, 2013 at 12:38 PM, Josh Magee <span dir="ltr"><<a href="mailto:Joshua_Magee@playstation.sony.com" target="_blank">Joshua_Magee@playstation.sony.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
jmagee added you to the CC list for the revision "Catch more cases when diagnosing integer-constant-expression overflows.".<br>
<br>
Hi,<br>
<br>
This patch improves integer-constant-expression overflow diagnostics.<br>
<br>
Currently clang catches overflow in some cases:<br>
<br>
int i;<br>
...<br>
switch (i) {<br>
  case 4096 * 1024 * 1024:  // Generates overflow warning<br>
  ...<br>
}<br>
<br>
if (4096 * 1024 * 1024) { // Generates overflow warning<br>
}<br>
<br>
But it misses other cases:<br>
<br>
uint64_t x = 4096 * 1024 * 1024; // no warning<br>
<br>
if ((x = 4096 * 1024 * 1024)) { // no warning<br>
}<br>
<br>
switch (i) {<br>
  case (uint64_t)(4096 * 1024 * 1024): // no warning<br>
...<br>
}<br>
<br>
void f1(int);<br>
...<br>
f1(4096 * 1024 * 1024); // no warning<br>
<br>
<br>
The patch introduces the following changes:<br>
 1) In Sema::CheckForIntOverflow, when checking for a BinaryOperator ignore<br>
    any casts in addition to parentheses.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
   Ignoring only parentheses was not enough because it missed cases like:<br>
   x = 4608 * 1024 * 1024, which has an implicit cast:<br>
    ImplicitCastExpr 0x5145858 'uint64_t':'unsigned long' <IntegralCast><br>
    `-BinaryOperator 0x5145830 'int' '*'<br>
      |-BinaryOperator 0x51457e8 'int' '*'<br>
      | |-IntegerLiteral 0x51457a8 'int' 4608<br>
      | `-IntegerLiteral 0x51457c8 'int' 1024<br>
      `-IntegerLiteral 0x5145810 'int' 1024<br>
<br>
 2) In Sema::CheckForIntOverflow, check for CallExprs and recursively check for<br>
    overflow on each argument.<br></blockquote><div><br></div><div>It would be better to just pass the whole expression to the checker; that was the original intent.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

  This handles cases like f1(4096 * 1024 * 1024).<br>
<br>
 3) In IntExprEvaluator::VisitBinaryOperator(), bail out on AssignmentOps<br>
   _after_ running the DataRecursiveIntBinOpEvaluator. In fact, I am not sure<br>
   if check here for AssignmentOps is really necessary.  It seems like it is an<br>
   early-exit optimization.</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">  Presumably it was thought that assignment<br>
   operators would not have any integer expression operands that need<br>
   evaluating here, however this is not the case in "x = y = 4096 * 1024 * 1024"<br>
   or "if ((y = 4096 * 1024 *1024))".  Running the DataRecursiveIntBinOpEvaluator<br>
   on AssignmentOps catches these cases.  Note, this change was only needed to<br>
   catch these cases in C mode.  In C++ mode an implicit LValueToRValue cast was<br>
   added which changed the path taken through semantic analysis.<br>
<br>
<br>
Thanks,<br>
Josh<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1238" target="_blank">http://llvm-reviews.chandlerc.com/D1238</a><br>
<br>
Files:<br>
  lib/AST/ExprConstant.cpp<br>
  lib/Sema/SemaChecking.cpp<br>
  test/Sema/integer-overflow.c<br>
  test/SemaCXX/integer-overflow.cpp<br>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br>