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

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D36564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D36564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D36564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D36564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Shaposhnikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310887: [analyzer] Fix SimpleSValBuilder::simplifySVal 
(authored by alexshap).

Changed prior to commit:
  https://reviews.llvm.org/D36564?vs=110502=111073#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36564

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


Index: cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ cfe/trunk/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
Index: cfe/trunk/test/Analysis/ptr-arith.cpp
===
--- cfe/trunk/test/Analysis/ptr-arith.cpp
+++ cfe/trunk/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: cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ cfe/trunk/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
Index: cfe/trunk/test/Analysis/ptr-arith.cpp
===
--- cfe/trunk/test/Analysis/ptr-arith.cpp
+++ cfe/trunk/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;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D36564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 RV = rhs.getAs()) {
 // 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 RV = rhs.getAs()) {
 // 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(), type);
  }

is executed instead of

  if (Optional LV = lhs.getAs()) {
if (Optional RV = rhs.getAs())
  return evalBinOpLL(state, op, *LV, *RV, type);
  
return evalBinOpLN(state, op, *LV, rhs.castAs(), 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


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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits