[analyzer] Fix SimpleSValBuilder::simplifySVal

[alexander-shaposhnikov]

Summary

This diff attempts to address a crash (triggered assert) on the newly-added test case.
The assert being discussed is inside SValBuilder::evalBinOp

if (Optional<Loc> RV = rhs.getAs<Loc>()) {
   // Support pointer arithmetic where the addend is on the left
   // and the pointer on the right.
   assert(op == BO_Add);

but the root cause seems to be in a different place
(if I'm not missing smth).

The method simplifySVal appears to be doing the wrong thing for SVal "(char*)End - (char*)Begin".
Call stack: evalIntegralCast -> evalBinOpNN -> simplifySVal -> ... -> simplifySVal -> Simplifier::VisitSymSymExpr -> ...

Inside Simplifier::VisitSymSymExpr the call Visit(S->getLHS()) returns a NonLoc
(where S->getLHS() represents char* End) while the call Visit(S->getRHS()) returns a Loc.

For Visit(S->getRHS()) this happens because in VisitSymbolData(const SymbolData *S) the "if" condition

if (const llvm::APSInt *I =
   SVB.getKnownValue(State, nonloc::SymbolVal(S)))

is satisfied and Loc is correctly chosen.
For Visit(S->getLHS()) nonloc::SymbolVal(S) is used instead.
Next those values will passed to SValBuilder::evalBinOp and the code path

if (Optional<Loc> RV = rhs.getAs<Loc>()) {
   // Support pointer arithmetic where the addend is on the left
   // and the pointer on the right.
  assert(op == BO_Add);
   // Commute the operands.
  return evalBinOpLN(state, op, *RV, lhs.castAs<NonLoc>(), type);
}

will be executed instead of the correct one

if (Optional<Loc> LV = lhs.getAs<Loc>()) {
  if (Optional<Loc> RV = rhs.getAs<Loc>())
    return evalBinOpLL(state, op, *LV, *RV, type);

  return evalBinOpLN(state, op, *LV, rhs.castAs<NonLoc>(), type);
}

Test plan: make check-all (green)

[NoQ]

Hmm, looks correct. Thanks! I guess this problem appeared when we enabled SymSymExprs, otherwise (End - Begin) would have been UnknownVal.

[alexfh]

I suspect this might have caused http://llvm.org/PR34309. Could you take a look?

[alexfh]

In D36564#851384, @alexfh wrote:

I suspect this might have caused http://llvm.org/PR34309. Could you take a look?

FYI, PR34309 doesn't reproduce with this patch reverted.

[NoQ]

I see, it seems that we've constructed $x - 1 somewhere, where $x is a pointer, while such stuff normally looks as &element{SymRegion{$x}, -1}. I guess i'd have to take a more careful look at it soon.

[alexander-shaposhnikov]

@alexfh, thanks for letting me know, i will take a closer look at https://bugs.llvm.org/show_bug.cgi?id=34309 soon.

cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

public:
     Simplifier(ProgramStateRef State)
         : State(State), SVB(State->getStateManager().getSValBuilder()) {}
 
     SVal VisitSymbolData(const SymbolData *S) {
       if (const llvm::APSInt *I =
               SVB.getKnownValue(State, nonloc::SymbolVal(S)))
         return Loc::isLocType(S->getType()) ? (SVal)SVB.makeIntLocVal(*I)
                                             : (SVal)SVB.makeIntVal(*I);
-      return nonloc::SymbolVal(S);
+      return Loc::isLocType(S->getType()) ? (SVal)SVB.makeLoc(S) 
+                                          : nonloc::SymbolVal(S);
     }
 
     // TODO: Support SymbolCast. Support IntSymExpr when/if we actually
     // start producing them.
 
     SVal VisitSymIntExpr(const SymIntExpr *S) {
       SVal LHS = Visit(S->getLHS());
       SVal RHS;

cfe/trunk/test/Analysis/ptr-arith.cpp

void checkConditionalArray() {
     int* maybeArray = getArray(0);
     InitState(maybeArray);
 }
 
 void checkMultiDimansionalArray() {
   int a[5][5];
    *(*(a+1)+2) = 2;
 }
+
+unsigned ptrSubtractionNoCrash(char *Begin, char *End) {
+  auto N = End - Begin;
+  if (Begin)
+    return 0;
+  return N;
+}