[PATCH] D36564: [analyzer] Fix SimpleSValBuilder::simplifySVal

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap created this revision.
Herald added a subscriber: xazax.hun.

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*)E - (char*)B".
Call stack: evalIntegralCast -> evalBinOpNN -> simplifySVal -> ... -> simplifySVal -> Simplifier::VisitSymSymExpr -> ...

Inside Simplifier::VisitSymSymExpr the call Visit(S->getLHS()) returns a NonLoc 
(where S->getLHS() represents char* E) 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);
  }

is executed instead of

  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)


Repository:
  rL LLVM

https://reviews.llvm.org/D36564

Files:
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  test/Analysis/ptr-arith.cpp


Index: test/Analysis/ptr-arith.cpp
===================================================================
--- test/Analysis/ptr-arith.cpp
+++ test/Analysis/ptr-arith.cpp
@@ -98,3 +98,10 @@
   int a[5][5];
    *(*(a+1)+2) = 2;
 }
+
+unsigned ptrSubtractionNoCrash(char *Begin, char *End) {
+  auto N = End - Begin;
+  if (Begin)
+    return 0;
+  return N;
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===================================================================
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1016,7 +1016,8 @@
               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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D36564.110502.patch
Type: text/x-patch
Size: 1062 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170810/55e5e9e3/attachment.bin>