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

[Richard Trieu]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120222/e27915e3/attachment.html>