[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
This revision was automatically updated to reflect the committed changes. Closed by commit rC338263: [analyzer] Add missing state transition in IteratorChecker. (authored by rkovacs, committed by ). Repository: rC Clang https://reviews.llvm.org/D47417 Files: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp === --- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -551,6 +551,8 @@ State = State->remove(Comp.first); } } + + C.addTransition(State); } ProgramStateRef IteratorChecker::evalAssume(ProgramStateRef State, SVal Cond, Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp === --- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -551,6 +551,8 @@ State = State->remove(Comp.first); } } + + C.addTransition(State); } ProgramStateRef IteratorChecker::evalAssume(ProgramStateRef State, SVal Cond, ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
This revision was automatically updated to reflect the committed changes. Closed by commit rL338263: [analyzer] Add missing state transition in IteratorChecker. (authored by rkovacs, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D47417?vs=148735=157983#toc Repository: rL LLVM https://reviews.llvm.org/D47417 Files: cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp Index: cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp === --- cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -551,6 +551,8 @@ State = State->remove(Comp.first); } } + + C.addTransition(State); } ProgramStateRef IteratorChecker::evalAssume(ProgramStateRef State, SVal Cond, Index: cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp === --- cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -551,6 +551,8 @@ State = State->remove(Comp.first); } } + + C.addTransition(State); } ProgramStateRef IteratorChecker::evalAssume(ProgramStateRef State, SVal Cond, ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
baloghadamsoftware added a comment. Did the tests execute? I am not sure. First problem is the the container may become dead before the iterator, so its `Begin` and `End` symbols may be inaccessible. This is easy to solve by marking the container of the iterator as live. However, there is a second problem that disables correct tracking of iterators: memory regions are marked as dead, however there are `LazyCompoundVal`s referring to them. Is this maybe a bug in `SymbolReaper`? Repository: rC Clang https://reviews.llvm.org/D47417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
baloghadamsoftware accepted this revision. baloghadamsoftware added a comment. Oh, it slipped through somehow. Thanks for fixing this! Repository: rC Clang https://reviews.llvm.org/D47417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
MTC added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:399 + + C.addTransition(State); } NoQ wrote: > MTC wrote: > > I have two questions may need @NoQ or @xazax.hun who is more familiar with > > the analyzer engine help to answer. > > > > - `State` may not change at all, do we need a check here like [[ > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp#L227 > > | if (state != originalState) ]] > > - A more basic problem is that do we need `originalState = State` trick. > > It seems that `addTransitionImpl()` has a check about same state > > transition, see [[ > > https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h#L339 > > | addTransitionImp() ]]. > > > > Thanks in advance! > > > > > > > > It seems that `addTransitionImpl()` has a check about same state > > transition, see `addTransitionImp()`. > > Yep, you pretty much answered your question. The check in the checker code is > unnecessary. Thanks, NoQ! It seems that `if (state != originalState)` in some checkers is misleading and may need to be cleaned up. Repository: rC Clang https://reviews.llvm.org/D47417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
NoQ added a comment. Nice catch! Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:399 + + C.addTransition(State); } MTC wrote: > I have two questions may need @NoQ or @xazax.hun who is more familiar with > the analyzer engine help to answer. > > - `State` may not change at all, do we need a check here like [[ > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp#L227 > | if (state != originalState) ]] > - A more basic problem is that do we need `originalState = State` trick. It > seems that `addTransitionImpl()` has a check about same state transition, see > [[ > https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h#L339 > | addTransitionImp() ]]. > > Thanks in advance! > > > > It seems that `addTransitionImpl()` has a check about same state transition, > see `addTransitionImp()`. Yep, you pretty much answered your question. The check in the checker code is unnecessary. Repository: rC Clang https://reviews.llvm.org/D47417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
MTC added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:399 + + C.addTransition(State); } I have two questions may need @NoQ or @xazax.hun who is more familiar with the analyzer engine help to answer. - `State` may not change at all, do we need a check here like [[ https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp#L227 | if (state != originalState) ]] - A more basic problem is that do we need `originalState = State` trick. It seems that `addTransitionImpl()` has a check about same state transition, see [[ https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h#L339 | addTransitionImp() ]]. Thanks in advance! Repository: rC Clang https://reviews.llvm.org/D47417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
rnkovacs created this revision. rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov, baloghadamsoftware. Herald added subscribers: a.sidorin, dkrupp, szepet, whisperity. After cleaning up program state maps in `checkDeadSymbols()`, a transition should be added to generate the new state. Repository: rC Clang https://reviews.llvm.org/D47417 Files: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp === --- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -395,6 +395,8 @@ State = State->remove(Comp.first); } } + + C.addTransition(State); } ProgramStateRef IteratorChecker::evalAssume(ProgramStateRef State, SVal Cond, Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp === --- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -395,6 +395,8 @@ State = State->remove(Comp.first); } } + + C.addTransition(State); } ProgramStateRef IteratorChecker::evalAssume(ProgramStateRef State, SVal Cond, ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
