[Analyzer] Fix for SValBuilder expressions rearrangement

[baloghadamsoftware]

Summary

Expression rearrangement in SValBuilder (see rL329780) crashes with an assert if the type of the integer is different from the type of the symbol. This fix adds a check that prevents rearrangement in such cases.

[NoQ]

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

443

6. Therefore i conclude that this check should be moved to the branch around comment 3., and it'd become redundant here.

455–456

1. We have expected LSym to be of this type.

469

2. It holds on this branch.

474

3. But not necessarily on that branch.

486

4. We check that RSym is of the correct type, but LSym remains unchecked.

491

5. Type of LInt here is guaranteed to be equal to the type of LSym, because that's how we construct symbols. Which was supposed to also be equal to SingleTy, but we didn't check for that.

[baloghadamsoftware]

Updated according to the comments.

[NoQ]

Yup thanks!

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

return nonloc::SymbolVal(
       SymMgr.getSymIntExpr(ResultSym, ResultOp, PersistentResultInt, ResultTy));
 }
 
 // Rearrange if symbol type matches the result type and if the operator is a
 // comparison operator, both symbol and constant must be within constant
 // overflow bounds.
 static bool shouldRearrange(ProgramStateRef State, BinaryOperator::Opcode Op,
                             SymbolRef Sym, llvm::APSInt Int, QualType Ty) {
+  BasicValueFactory &BV = State->getStateManager().getBasicVals();
+
   return Sym->getType() == Ty &&
+    APSIntType(Int) == BV.getAPSIntType(Sym->getType()) &&
     (!BinaryOperator::isComparisonOp(Op) ||
      (isWithinConstantOverflowBounds(Sym, State) && 
       isWithinConstantOverflowBounds(Int)));
 }
 
 static Optional<NonLoc> tryRearrange(ProgramStateRef State,
                                      BinaryOperator::Opcode Op, NonLoc Lhs,
                                      NonLoc Rhs, QualType ResultTy) {
   ProgramStateManager &StateMgr = State->getStateManager();
   SValBuilder &SVB = StateMgr.getSValBuilder();
 
   // We expect everything to be of the same type - this type.
   QualType SingleTy;
 
   auto &Opts =
     StateMgr.getOwningEngine()->getAnalysisManager().getAnalyzerOptions();
 
   SymbolRef LSym = Lhs.getAsSymbol();
   if (!LSym)
     return None;
 
   // Always rearrange additive operations but rearrange comparisons only if
   // option is set.
   if (BinaryOperator::isComparisonOp(Op) &&
       Opts.shouldAggressivelySimplifyRelationalComparison()) {
     SingleTy = LSym->getType();
     if (ResultTy != SVB.getConditionType())
       return None;
     // Initialize SingleTy later with a symbol's type.
   } else if (BinaryOperator::isAdditiveOp(Op)) {
     SingleTy = ResultTy;
     // 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!");
 
   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);
   if (!shouldRearrange(State, Op, LSym, LInt, SingleTy) ||
       !shouldRearrange(State, Op, RSym, RInt, SingleTy))
     return None;
 
   // We know that no overflows can occur anymore.
   return doRearrangeUnchecked(State, Op, LSym, LInt, RSym, RInt);
 }

test/Analysis/svalbuilder-rearrange-comparisons.c

void compare_same_symbol_minus_left_minus_right_int_less_unsigned() {
   // expected-warning@-1{{FALSE}}
 }
 
 void overflow(signed char n, signed char m) {
   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;
+}