[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

This revision was automatically updated to reflect the committed changes. Closed by commit rL291430: [analyzer] Add checker for iterators dereferenced beyond their range. (authored by xazax). Changed prior to commit: https://reviews.llvm.org/D25660?vs=81224=83596#toc Repository: rL LLVM

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added a comment. And thank you for the awesome work and addressing the review comments!!! https://reviews.llvm.org/D25660 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added a comment. > I am doing it right now. Unfortunately I found a crash which I fixed, Is it fixed in this patch? > but then it turned out that overwrites of the iterator variable are not > handled. I am working on this > problem. My suggestion is to commit this patch and

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added a comment. In https://reviews.llvm.org/D25660#613519, @zaks.anna wrote: > Also, have you evaluated this on real codebases? What results do you see? Are > there any false positives found? Are there any true positives found? I am doing it right now. Unfortunately I

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware updated this revision to Diff 81224. baloghadamsoftware added a comment. Now isInStdNamespace is used. Hack is back so https://reviews.llvm.org/D27202 is not a dependency now. https://reviews.llvm.org/D25660 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added a comment. Thanks Artem! Just to be clear, I think this patch should be committed once "inTopLevelNamespace" issue is addressed. That is the only issue pending as far as I can see. The visitor should be a separate patch. https://reviews.llvm.org/D25660

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

NoQ added a comment. A quick example of how a bug reporter visitor for this checker may look like - it needs to be expanded significantly but here's a start. F2675703: report-11.html <== example of how it looks. See, for example, `MallocChecker` to

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:721 + +static bool inTopLevelNamespace(const Decl *D, IdentifierInfo *II) { + const auto *ND = dyn_cast(D->getDeclContext()); zaks.anna wrote: > Would

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added a comment. https://reviews.llvm.org/D27202 is now a dependency, but cannot add it. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:166 +IteratorPastEndChecker::IteratorPastEndChecker() { + PastEndBugType.reset(new BugType(this,

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware updated this revision to Diff 80728. baloghadamsoftware marked 4 inline comments as done. baloghadamsoftware added a comment. Minor corrections, comments, some new tests, test input headers merged. https://reviews.llvm.org/D25660 Files:

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added a comment. Also, have you evaluated this on real codebases? What results do you see? Are there any false positives found? Are there any true positives found? https://reviews.llvm.org/D25660 ___ cfe-commits mailing list

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added a comment. It's awesome to see that all the major issues have been addressed. Thank you for working on this and diligently working through the code review!!! I have a few minor comments below. Could you add this example yours as a "no-warning" test case: const auto start =

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:204 + CheckerContext ) const { + const auto *ThisExpr = COCE->getArg(0); + baloghadamsoftware wrote: > NoQ

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added inline comments. Comment at: test/Analysis/Inputs/system-header-simulator-for-iterators.h:62 + ForwardIterator2 first2, ForwardIterator2 last2); +} Maybe we should merge this file with the

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware updated this revision to Diff 78527. baloghadamsoftware added a comment. Test updated to include test case where system headers are inlined. https://reviews.llvm.org/D25660 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423 + +void IteratorPastEndChecker::handleComparison(CheckerContext , + const SVal , baloghadamsoftware wrote: >

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware marked 2 inline comments as done. baloghadamsoftware added a comment. In https://reviews.llvm.org/D25660#590778, @NoQ wrote: > - Agree on the `evalAssume()` implementation (i'm still not quite > understanding what the problem is here, see the new inline comments); I think

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware marked 10 inline comments as done. baloghadamsoftware added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:209 + CheckerContext ) const { + const auto *Func =

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware updated this revision to Diff 78352. baloghadamsoftware added a comment. Updated according to comments. https://reviews.llvm.org/D25660 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/CMakeLists.txt

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:219 + + assert(LCtx->getKind() == LocationContext::StackFrame && + "Function does not begin with stack frame context"); a.sidorin wrote: > `isa(LCtx)`? >

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:204 + CheckerContext ) const { + const auto *ThisExpr = COCE->getArg(0); + NoQ wrote: > This code definitely

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423 + +void IteratorPastEndChecker::handleComparison(CheckerContext , + const SVal , baloghadamsoftware

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423 + +void IteratorPastEndChecker::handleComparison(CheckerContext , +

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware updated this revision to Diff 77033. baloghadamsoftware added a comment. Interim version, updated according to some of the comments. https://reviews.llvm.org/D25660 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/CMakeLists.txt

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423 + +void IteratorPastEndChecker::handleComparison(CheckerContext , + const SVal , NoQ wrote: > a.sidorin

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:580 + C.addTransition(stateFound); + C.addTransition(stateNotFound); +} NoQ wrote: > Ouch, i have one more concern, which can be expressed with the following >

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:580 + C.addTransition(stateFound); + C.addTransition(stateNotFound); +} Ouch, i have one more concern, which can be expressed with the following false-positive test

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

NoQ added a comment. I think i managed to understand the reasoning behind your solutions! Right now i definitely approve all the high-level logic apart from the handling of left/right `SVal`s for `evalAssume`, which i think could be easily improved upon without significant drawbacks. See the

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

zaks.anna added a comment. >> Actually, I always test first on real code, and it seemed to be inlined. But >> now, even if I >> removed the pragma it was not inlined. Looks like this patch is interfering with this inlining suppression. We had many false positives without it. Mainly, the

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

a.sidorin added a comment. Thank you for this patch! I like some solutions used in it but I also have some comments (inline). https://reviews.llvm.org/D25660 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:209 + CheckerContext ) const { + const auto *Func = Call.getDecl()->getAsFunction(); + if (Func->isOverloadedOperator()) {

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added inline comments. Comment at: test/Analysis/iterator-past-end.cpp:3 + +template struct __iterator { + typedef __iterator iterator; NoQ wrote: > baloghadamsoftware wrote: > > NoQ wrote: > > > We should probably separate this

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

NoQ added a comment. Thanks!! Will try to look at the rest of the stuff as soon as possible>< Comment at: test/Analysis/iterator-past-end.cpp:3 + +template struct __iterator { + typedef __iterator iterator; baloghadamsoftware wrote: > NoQ wrote:

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:195 +auto Param = State->getLValue(P, LCtx); +auto Arg = State->getSVal(CE->getArg(idx++), LCtx->getParent()); +const auto *Pos = getIteratorPosition(State,

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

baloghadamsoftware updated this revision to Diff 75875. baloghadamsoftware added a comment. Updated according to the comments. Also fixed a bug and moved access check to pre-call instead of post-call. https://reviews.llvm.org/D25660 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

NoQ added a subscriber: a.sidorin. NoQ added a comment. Wow, you managed to check something that could be checked without going through a hell of modeling dozens of STL methods, and probably even without stepping on poor C++ temporary object modeling in the analyzer, which sounds great. These