[analyzer] Fix crash in RangedConstraintManager.cpp

[vabridgers]

Summary

This change fixes a crash in RangedConstraintManager.cpp:assumeSym due to an
unhandled BO_Div case.

clang: <root>clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp:51:

virtual clang::ento::ProgramStateRef
clang::ento::RangedConstraintManager::assumeSym(clang::ento::ProgramStateRef,
  clang::ento::SymbolRef, bool):
Assertion `BinaryOperator::isComparisonOp(Op)' failed.

[vabridgers]

remove assert that was commented out

[NoQ]

Thanks! I agree that this assertion doesn't make much sense. Judging by the name, the function should be able to handle all symbols.

clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp

61

The new line break in the middle of a != b doesn't make much sense.

[vabridgers]

update comment

clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp

if (const SymIntExpr *SIE = dyn_cast<SymIntExpr>(Sym)) {
 
     // Handle adjustment with non-comparison ops.
     const llvm::APSInt &Zero = getBasicVals().getValue(0, SIE->getType());
     return assumeSymRel(State, SIE, (Assumption ? BO_NE : BO_EQ), Zero);
   }
 
   if (const auto *SSE = dyn_cast<SymSymExpr>(Sym)) {
     BinaryOperator::Opcode Op = SSE->getOpcode();
-    assert(BinaryOperator::isComparisonOp(Op));
+    if (BinaryOperator::isComparisonOp(Op)) {
 
       // We convert equality operations for pointers only.
       if (Loc::isLocType(SSE->getLHS()->getType()) &&
           Loc::isLocType(SSE->getRHS()->getType())) {
         // Translate "a != b" to "(b - a) != 0".
         // We invert the order of the operands as a heuristic for how loop
         // conditions are usually written ("begin != end") as compared to length
-      // calculations ("end - begin"). The more correct thing to do would be to
-      // canonicalize "a - b" and "b - a", which would allow us to treat
+        // calculations ("end - begin"). The more correct thing to do would be
+        // to canonicalize "a - b" and "b - a", which would allow us to treat
         // "a != b" and "b != a" the same.
 
         SymbolManager &SymMgr = getSymbolManager();
         QualType DiffTy = SymMgr.getContext().getPointerDiffType();
         SymbolRef Subtraction =
             SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
 
         const llvm::APSInt &Zero = getBasicVals().getValue(0, DiffTy);
         Op = BinaryOperator::reverseComparisonOp(Op);
         if (!Assumption)
           Op = BinaryOperator::negateComparisonOp(Op);
         return assumeSymRel(State, Subtraction, Op, Zero);
       }
 
       if (BinaryOperator::isEqualityOp(Op)) {
         SymbolManager &SymMgr = getSymbolManager();
 
         QualType ExprType = SSE->getType();
         SymbolRef CanonicalEquality =
             SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
 
         bool WasEqual = SSE->getOpcode() == BO_EQ;
         bool IsExpectedEqual = WasEqual == Assumption;
 
         const llvm::APSInt &Zero = getBasicVals().getValue(0, ExprType);
 
         if (IsExpectedEqual) {
           return assumeSymNE(State, CanonicalEquality, Zero, Zero);
         }
 
         return assumeSymEQ(State, CanonicalEquality, Zero, Zero);
       }
     }
+  }
 
   // If we get here, there's nothing else we can do but treat the symbol as
   // opaque.
   return assumeSymUnsupported(State, Sym, Assumption);
 }
 
 ProgramStateRef RangedConstraintManager::assumeSymInclusiveRange(
     ProgramStateRef State, SymbolRef Sym, const llvm::APSInt &From,

clang/test/Analysis/symbol-simplification-bo-div.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core %s \
+// RUN:    -triple x86_64-pc-linux-gnu -verify
+
+// don't crash
+// expected-no-diagnostics
+
+int a, b;
+int c(void) {
+  unsigned d = a;
+  --d;
+  short e = b / b - a;
+  ++e;
+  return d <= 0 && e && e;
+}