On Mar 12, 2012, at 1:38 PM, Richard Trieu wrote:
>
>
> On Sun, Mar 11, 2012 at 1:58 PM, Douglas Gregor <[email protected]> wrote:
>
> On Feb 14, 2012, at 3:42 PM, Richard Trieu 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.
>> [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.
>
> I will look into this.
>
> + 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.
> The check for volatile is performed after the DeclMatcher finishes.
>
> + 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.
>
> I'm a bit worried about how many visitor methods I need to write to, but I'll
> give it a try and see what happens.
I could settle for a full audit of all of the expressions, but the fact that
there are several obvious missing cases worries me a lot.
> + // 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.
>
> An EvaluatedExprVisitor is used to find the decls used in the loop condition,
> which is called the DeclExtractor. A separate visitor, the DeclMatch, is
> used on the loop conditional, the loop increment, and the loop body to see if
> the decls are in use. For this, non-evaluated contexts that could change the
> decl are important, so an EvaluatedExprVisitor can not be used here.
How would a non-evaluated context change the value of the variable?
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
