[cfe-commits] [Patch] Add a warning for for loops with conditions that do not change

[Douglas Gregor]

On Feb 14, 2012, at 3:42 PM, Richard Trieu wrote:

> On Tue, Feb 7, 2012 at 10:38 AM, Douglas Gregor <dgregor at apple.com> wrote:
> 
> On Feb 6, 2012, at 1:00 PM, Eli Friedman wrote:
> 
> > On Mon, Feb 6, 2012 at 2:50 PM, Richard Trieu <rtrieu at google.com> wrote:
> >> On Mon, Feb 6, 2012 at 2:02 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> >>>
> >>> On Mon, Feb 6, 2012 at 1:55 PM, Richard Trieu <rtrieu at google.com> wrote:
> >>>> The motivation of this path is to catch code like this:
> >>>>
> >>>> for (int i = 0; i < 10; ++i)
> >>>>   for (int j = 0; j < 10; ++i)
> >>>>     { }
> >>>>
> >>>> The second for loop increments i instead of j causing an infinite loop.
> >>>>  The
> >>>> warning also checks the body of the for loop so it will trigger on:
> >>>>
> >>>> for (int i; i <10; ) { }
> >>>>
> >>>> But not trigger on:
> >>>>
> >>>> for (int i; i< 10; ) { ++i; }
> >>>>
> >>>> I'm still fine-tuning the trigger conditions, but would like some
> >>>> feedback
> >>>> on this patch.
> >>>
> >>> Adding an additional recursive traversal of the body of every parsed
> >>> for loop is very expensive.
> >>>
> >>> -Eli
> >>
> >>
> >> Is it that expensive?  I would think that once the AST was constructed, the
> >> visitor would be pretty fast, especially if no action is taken on most of
> >> the nodes.  I also made the warning default ignore and put in an early
> >> return to prevent the visitors from running in the default case.
> >>
> >> Do you have any suggestions on removing the recursive traversal?
> >
> > Okay, thinking about it a bit more, maybe it's not that expensive, but
> > you should at least measure to make sure.
> 
> I'm still very nervous about adding such an AST traversal. Presumably, it's not going to be a concern in practice because most of the time, the increment statement of the 'for' loop will mention one of the variables in the condition, and therefore we'll short-circuit the expensive walk of the loop body. It would be helpful to know that's the case.
> 
> > I don't have any good suggestion for an alternative.
> 
> 
> Nor do I.
> 
> A couple more comments:
> 
> +    ValueDecl *VD = E->getDecl();
> +    for (SmallVectorImpl<ValueDecl*>::iterator I = Decls.begin(),
> +                                               E = Decls.end();
> +         I != E; ++I)
> +      if (*I == VD) {
> +        FoundDecl = true;
> +        return;
> +      }
> 
> This is linear; please use a SmallPtrSet instead.
> Done. 
> 
> Plus, I think you want to narrow this check to only consider VarDecls. Functions and enumerators are not interesting.
> Done. 
> 
> +      S.Diag(Second->getLocStart(), diag::warn_variables_not_in_loop_body)
> +          << Second->getSourceRange();
> 
> This warning needs to specify which variables were not used or point to them in the source.
> The diagnostic now underlines all the variables in the condition.   
> 
> Do we want to actually look for modification, e.g., any use of the variable that isn't immediately consumed by an lvalue-to-rvalue conversion?
> Yes, that is exactly what we are checking for.  The VisitCastExpr checks for LvalueToRvalue casts and skips any DeclRefExpr's that are direct sub-expressions.
> 
>        - Doug
> 
> I also did a comparison between runs with and without -Wloop-analysis.  [snip]

Thanks for all of the performance testing. A few more comments on the actual code:

Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td	(revision 150501)
+++ include/clang/Basic/DiagnosticSemaKinds.td	(working copy)
@@ -14,6 +14,11 @@
 let Component = "Sema" in {
 let CategoryName = "Semantic Issue" in {
 
+// For loop analysis
+def warn_variables_not_in_loop_body : Warning<
+  "variables used in loop condition not modified in loop body">,
+  InGroup<DiagGroup<"loop-analysis">>, DefaultIgnore;

It would be really nice if this diagnostic could mention the variables by name ("variables 'i' and 'n' used in loop condition are not modified in loop body"), like we do with missing enumerators in a switch. It'll take some special casing for 1/2/many variables, but it will be much nicer for the user.

+  void VisitDeclRefExpr(DeclRefExpr *E) {
+    VarDecl *VD = dyn_cast<VarDecl>(E->getDecl());
+    if (!VD) return;
+
+    PDiag << E->getSourceRange();
+
+    // Dont do analysis on pointers.
+    if (VD->getType()->isAnyPointerType()) {
+      Simple = false;
+      return;
+    }
+
+    Decls.insert(VD);
+  }

Presumably, anything volatile-qualified should make the expression non-simple.

+  void VisitCastExpr(CastExpr *E) {
+    // Don't do analysis on function calls or arrays.
+    if (E->getCastKind() == CK_FunctionToPointerDecay ||
+        E->getCastKind() == CK_ArrayToPointerDecay) {
+      Simple = false;
+      return;
+    }
+
+     Visit(E->getSubExpr());
+  }

If you want to avoid calls, why not just intercept CallExpr and CXXConstructExpr and call those non-simple?

Backing up a step, it seems strange that DeclExtractor assumes that an expression is 'simple' unless it sees some very small number of cases that aren't consider simple. That feels very brittle to me because, e.g., CXXConstructExpr is certainly not simple (for non-trivial constructors), yet it would be considered simple by this approach. Similarly for CXXNewExpr and a host of other non-trivial expressions that aren't covered. If we have to have the notion of 'simple', then we need to build it constructively by white-listing expressions that are known to be simple and banning (by default) all other expressions.

+  // DeclMatcher checks to see if the decls are used in a non-evauluated
+  // context.  
+  class DeclMatcher : public StmtVisitor<DeclMatcher> {

Should this use an EvaluatedExprVisitor to provide more accurate diagnostics? e.g., the presence of sizeof(x) doesn't mean that 'x' has been used in a meaningful way.

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120311/1f4e83f4/attachment.html>