[cfe-commits] [Patch][review request]update to IteratorsChecker
Jim Goodnow II
jim at thegoodnows.net
Fri Mar 18 04:04:18 PDT 2011
Hi Ted,
Okay, I agree and that was what I had originally intended, but ran
into difficulties implementing it. So, as a first step, let's talk
just about something simple:
it = v.begin();
Conceptually, the MemberCall for begin() returns a symbol region
which I mark as BeginValid. It is also linked with the MemRegion
associated with the instance or really any 'TypedRegion' associated
with the Base of the MemberCall which is the instance. When the
symbol region is assigned to the iterator, 'it', the BeginValid state
gets propogated to the MemRegion associated with the iterator.
First, does the returned symbol region actually get created by the
engine or do I have to do that in checkPostStmt(CXXMemberCall) or
somewhere else?
Second, do I still need to handle the assignment in OperatorCall
since it is essentially a structure/class assignment or will that be
handled by the Engine logic?
- jim
At 10:54 PM 3/15/2011, Ted Kremenek wrote:
>Hi Jim,
>
>While a nice increment on top of the existing checker, I think this
>takes the checker in a more syntactic direction, which isn't going
>to be a general solution. Instead of pattern matching on specific
>expressions, we should instead move the checker in a direction where
>it just lets the analyzer engine handle evaluating the semantics of
>expressions and have the checker look at SVals and MemRegions (i.e.,
>the "semantics" instead of the "syntax").
>
>For example, in checkPreStmt(CXXMemberCallExpr), there is no reason
>we need to rely on the base expression being a
>DeclRefExpr. Instead, we can just let the analyzer engine evaluate
>that subexpression; if that expression evaluates to a MemRegion,
>then we can proceed. There is no reason to make this dependent on
>DeclRefExpr, or any other expression, that evaluates to what we care
>about. Here's a suggested change:
>
>@@ -549,11 +554,11 @@ void IteratorsChecker::checkPreStmt(const
>CXXMemberCallExpr *MCE,
> const MemberExpr *ME = dyn_cast<MemberExpr>(MCE->getCallee());
> if (!ME)
> return;
>+ const Expr *exp = ME->getBase();
> // Make sure we have the right kind of container.
>- const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(ME->getBase());
>- if (!DRE || getTemplateKind(DRE->getType()) != VectorKind)
>+ if (getTemplateKind(exp->getType()) != VectorKind)
> return;
>- SVal tsv = C.getState()->getSVal(DRE);
>+ SVal tsv = C.getState()->getSVal(exp);
>
>Note that this differs from your patch. There is no reason to match
>against implicit casts or pointer types. We just reason about the
>values the analyzer engine gives us.
>
>Probably what needs to be reworked to fully generalize the checker
>to work with arbitrary expressions is 'handleAssign'. handleAssign
>does a bunch of AST pattern matching, looking that specific
>expressions appeared on the RHS of the assignment. While syntactic
>pattern matching is easy to understand, it isn't fully
>general. Instead, we should just let the analyzer engine evaluate
>each subexpression, and track what information we need to implement
>the checker through the associated SVals.
>
>For example, I would expect the handling of 'begin', 'insert',
>'erase', and 'end' to be in a checkerPostStmt(const
>CXXMemberCallExpr). These would then toggle the state associated
>with the target of the method call (which would be a
>MemRegion). For 'begin' and 'end', the call returns a value, which
>the analyzer engine will likely make a SymbolRegion. For that
>SymbolicRegion, we can associate checker-specific state in the GDM
>saying that MemRegion represents and iterator associated with a
>specific container. Indeed, your checker already does this:
>
> typedef llvm::ImmutableMap<const MemRegion *, RefState> EntryMap;
>
>In other words, let the analyzer engine do the work of propagating
>values around and evaluating subexpressions, and let your checker
>add "instrumentation" at key points (namely before and after method
>calls) to toggle the iterator state. Once this restructuring is in
>place, you can probably remove 'handleAssign' entirely.
>
>There's also no reason to restrict the container to
>'VarRegions'. Any 'TypedRegion' should probably work. This will
>allow your checker to immediately fully generalize to cases the
>checker doesn't consider, e.g.:
>
> void test(std::vector<int> **x) {
> std::vector<int>::iterator itr = (*x)->begin();
> ...
> }
>
>If you let the analyzer handle the abstraction of memory and the
>evaluation of expressions, you don't need to reason syntactically
>about all these corner cases. Everything just composes, and works
>as you would expect.
>
>On Mar 15, 2011, at 9:13 PM, Jim Goodnow II wrote:
>
> > Added support for handling pointers to containers that wasn't handled.
> >
> > - jim
> >
> > Index: test/Analysis/iterators.cpp
> > ===================================================================
> > --- test/Analysis/iterators.cpp (revision 127607)
> > +++ test/Analysis/iterators.cpp (working copy)
> > @@ -5,8 +5,7 @@
> >
> > void fum(std::vector<int>::iterator t);
> >
> > -void foo1()
> > -{
> > +void foo1() {
> > // iterators that are defined but not initialized
> > std::vector<int>::iterator it2;
> > fum(it2); // expected-warning{{Use of iterator that is not defined}}
> > @@ -64,8 +63,7 @@
> > }
> >
> > // work with using namespace
> > -void foo2()
> > -{
> > +void foo2() {
> > using namespace std;
> >
> > vector<int> v;
> > @@ -78,8 +76,7 @@
> > }
> >
> > // using reserve eliminates some warnings
> > -void foo3()
> > -{
> > +void foo3() {
> > std::vector<long> v;
> > std::vector<long>::iterator b = v.begin();
> > v.reserve( 100 );
> > @@ -94,8 +91,7 @@
> > }
> >
> > // check on copying one iterator to another
> > -void foo4()
> > -{
> > +void foo4() {
> > std::vector<float> v, vv;
> > std::vector<float>::iterator it = v.begin();
> > *it; // no-warning
> > @@ -103,3 +99,14 @@
> > *it; // expected-warning{{Attempt to use an iterator made
> invalid by copying another container to its container}}
> > }
> >
> > +// check for pointers to containers as well
> > +void foo5() {
> > + std::vector<short> v, *vp;
> > + vp = &v;
> > + std::vector<short>::iterator it;
> > + it = vp->begin();
> > + *it; // no-warning
> > + vp->insert( it, 1 );
> > + *it; // expected-warning{{Attempt to use an iterator made
> invalid by call to 'insert'.}}
> > +}
> > +
> > Index: lib/StaticAnalyzer/Checkers/IteratorsChecker.cpp
> > ===================================================================
> > --- lib/StaticAnalyzer/Checkers/IteratorsChecker.cpp (revision 127607)
> > +++ lib/StaticAnalyzer/Checkers/IteratorsChecker.cpp (working copy)
> > @@ -197,6 +197,8 @@
> > }
> >
> > static RefKind getTemplateKind(QualType T) {
> > + if ( isa<PointerType>(T) )
> > + T = T->getPointeeType();
> > if (const TemplateSpecializationType *tsp =
> > T->getAs<TemplateSpecializationType>()) {
> > return getTemplateKind(tsp);
> > @@ -266,7 +268,10 @@
> > if (const CallExpr *CE = dyn_cast<CallExpr>(rexp)) {
> > // Handle MemberCall.
> > if (const MemberExpr *ME = dyn_cast<MemberExpr>(CE->getCallee())) {
> > - const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(ME->getBase());
> > + const Expr *exp = ME->getBase();
> > + if (isa<ImplicitCastExpr>(exp))
> > + exp = dyn_cast<ImplicitCastExpr>(exp)->getSubExpr();
> > + const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(exp);
> > if (!DRE)
> > return state;
> > // Verify that the type is std::vector<T>.
> > @@ -549,8 +554,11 @@
> > const MemberExpr *ME = dyn_cast<MemberExpr>(MCE->getCallee());
> > if (!ME)
> > return;
> > + const Expr *exp = ME->getBase();
> > + if (isa<ImplicitCastExpr>(exp))
> > + exp = dyn_cast<ImplicitCastExpr>(exp)->getSubExpr();
> > // Make sure we have the right kind of container.
> > - const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(ME->getBase());
> > + const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(exp);
> > if (!DRE || getTemplateKind(DRE->getType()) != VectorKind)
> > return;
> > SVal tsv = C.getState()->getSVal(DRE);<itptr.patch>
More information about the cfe-commits
mailing list