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>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
