[PATCH] D36564: [analyzer] Fix SimpleSValBuilder::simplifySVal
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
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
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
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
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
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
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