[Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

[baloghadamsoftware]

Summary

During the review of D41938 a condition check with an early exit accidentally slipped into a branch, leaving the other branch unprotected. This may result in an assertion later on. This hotfix moves this contition check outside of the branch.

Please review it quickly! It should be released in clang 7.0.1.

[Szelethus]

Have you seen a crash resulting from this? Is supplying a test case feasable? I know that some of these errors are extremely hard to reproduce.

Edit: Nevertheless, looks good to me.

[baloghadamsoftware]

In D55051#1312660, @Szelethus wrote:

Have you seen a crash resulting from this? Is supplying a test case feasable? I know that some of these errors are extremely hard to reproduce.

Edit: Nevertheless, looks good to me.

There could be probably a test provided, but the bug is very obvious if you look at the code. Also if you compare it with the first version of the original patch.

[Szelethus]

In D55051#1312666, @baloghadamsoftware wrote:

In D55051#1312660, @Szelethus wrote:

Have you seen a crash resulting from this? Is supplying a test case feasable? I know that some of these errors are extremely hard to reproduce.

Edit: Nevertheless, looks good to me.

There could be probably a test provided, but the bug is very obvious if you look at the code. Also if you compare it with the first version of the original patch.

I'm sold on that. Let's wait for someone else then to formally accept. Please note that AFAIK the merge request deadline for 7.0.1 is tomorrow (nov. 30), so if putting this in is critical, writing a cfe-dev mail with a scary title may be a good idea.

[NoQ]

Test?

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

485

I guess it's no longer only about subtracting?

[NoQ]

Whoops, i was reviewing so quickly i didn't notice other comments :D

I still believe that this needs a regression test, for the same reason for which we need tests at all. If the deadline for committing comes before you manage to reduce the test case, please commit, but please add a test case as soon as possible.

[baloghadamsoftware]

Test added.

[baloghadamsoftware]

Committed because I was notified that the deadline for 7.0.1 is today.

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

if (BinaryOperator::isComparisonOp(Op)) {
     SingleTy = LSym->getType();
     if (ResultTy != SVB.getConditionType())
       return None;
     // Initialize SingleTy later with a symbol's type.
   } else if (BinaryOperator::isAdditiveOp(Op)) {
     SingleTy = ResultTy;
     if (LSym->getType() != SingleTy)
       return None;
-    // Substracting unsigned integers is a nightmare.
-    if (!SingleTy->isSignedIntegerOrEnumerationType())
-      return None;
   } else {
     // Don't rearrange other operations.
     return None;
   }
 
   assert(!SingleTy.isNull() && "We should have figured out the type by now!");
 
+  // Rearrange signed symbolic expressions only
+  if (!SingleTy->isSignedIntegerOrEnumerationType())
+    return None;
+
   SymbolRef RSym = Rhs.getAsSymbol();
   if (!RSym || RSym->getType() != SingleTy)
     return None;
 
   BasicValueFactory &BV = State->getBasicVals();
   llvm::APSInt LInt, RInt;
   std::tie(LSym, LInt) = decomposeSymbol(LSym, BV);
   std::tie(RSym, RInt) = decomposeSymbol(RSym, BV);

test/Analysis/svalbuilder-rearrange-comparisons.c

if (n + 0 > m + 0) {
     clang_analyzer_eval(n - 126 == m + 3); // expected-warning {{UNKNOWN}}
   }
 }
 
 int mixed_integer_types(int x, int y) {
   short a = x - 1U;
   return a - y;
 }
+
+unsigned gu();
+unsigned fu() {
+  unsigned x = gu();
+  // Assert that no overflows occur in this test file.
+  // Assuming that concrete integers are also within that range.
+  assert(x <= ((unsigned)UINT_MAX / 4));
+  return x;
+}
+
+void unsigned_concrete_int_no_crash() {
+  unsigned x = fu() + 1U, y = fu() + 1U;
+  clang_analyzer_denote(x - 1U, "$x");
+  clang_analyzer_denote(y - 1U, "$y");
+  clang_analyzer_express(y); // expected-warning {{$y}}
+  clang_analyzer_express(x == y); // expected-warning {{$x + 1U == $y + 1U}}
+}