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

[Richard Trieu]

On Wed, Feb 22, 2012 at 11:43 AM, Richard Trieu <rtrieu at google.com> wrote:

> On Tue, Feb 14, 2012 at 6:37 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> On Tue, Feb 14, 2012 at 3:42 PM, Richard Trieu <rtrieu at google.com> 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.
>>>  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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120228/70d5b2d9/attachment.html>