On Wed, Feb 22, 2012 at 11:43 AM, Richard Trieu <[email protected]> wrote:
> On Tue, Feb 14, 2012 at 6:37 PM, Richard Trieu <[email protected]> wrote: > >> On Tue, Feb 14, 2012 at 3:42 PM, Richard Trieu <[email protected]> wrote: >> >>> On Tue, Feb 7, 2012 at 10:38 AM, Douglas Gregor <[email protected]>wrote: >>> >>>> >>>> On Feb 6, 2012, at 1:00 PM, Eli Friedman wrote: >>>> >>>> > On Mon, Feb 6, 2012 at 2:50 PM, Richard Trieu <[email protected]> >>>> wrote: >>>> >> On Mon, Feb 6, 2012 at 2:02 PM, Eli Friedman <[email protected]> >>>> wrote: >>>> >>> >>>> >>> On Mon, Feb 6, 2012 at 1:55 PM, Richard Trieu <[email protected]> >>>> 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. >>> Even with the heavily nested loop, the extra recursive checks don't take >>> too much extra time to run. >>> >>> clang loop.cc -fsyntax-only >>> .460 - .480s >>> >>> clang loop.cc -fsyntax-only -Wloop-analysis >>> .520 - .540s >>> >>> Code with increments removed to trigger 2044 for loop warnings >>> clang loop.cc -fsyntax-only >>> .310 - .330s >>> >>> clang loop.cc -fsyntax-only -Wloop-analysis 2>/dev/null >>> .740 - .780s >>> >>> clang loop.cc -fsyntax-only -Wloop-analysis >>> 1.430 - 1.550s >>> >>> // Test loop code. >>> #define M(A) A A >>> #define L1 for(int a1 = 0; a1 < 10;){M(L2) } >>> #define L2 for(int a2 = 0; a2 < 10;){M(L3) a1++; } >>> #define L3 for(int a3 = 0; a3 < 10;){M(L4) a2++; } >>> #define L4 for(int a4 = 0; a4 < 10;){M(L5) a3++; } >>> #define L5 for(int a5 = 0; a5 < 10;){M(L6) a4++; } >>> #define L6 for(int a6 = 0; a6 < 10;){M(L7) a5++; } >>> #define L7 for(int a7 = 0; a7 < 10;){M(L8) a6++; } >>> #define L8 for(int a8 = 0; a8 < 10;){M(L9) a7++; } >>> #define L9 for(int a9 = 0; a9 < 10;){M(L) a8++; } >>> #define L a9++; >>> >>> void foo() { >>> L1 >>> L1 >>> L1 >>> L1 >>> } >>> >> >> Some data from real code. Took Clang source and preprocessed it into >> 300k lines of code (if that seems high, I accidentally copied the files >> twice). Ran the patched Clang across the files with either -Wloop-analysis >> set or omitted. -fsyntax-only was also used. >> >> Without -Wloop-analysis, >> Min time = 24.99s >> Max time = 25.63s >> Average = 25.218s >> >> With -Wloop-analysis, >> Min time = 25s >> Max time = 25.5s >> Average = 25.214s >> >> >> Ping. Someone asked on IRC why only for loops and not while loops. I gave it a try to extend it to while loops. The results were very noisy, due to the common idiom of: bool done = false; start_thread(&done); while (!done) { sleep(); } resulting in false positives all over threaded code. There's also global variables and a few other cases. For loops are more self-contained and a better place to start implementing this warning.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
