[cfe-commits] [Patch][review request]update to IteratorsChecker

[Ted Kremenek]

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>