[cfe-commits] r155835 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaStmt.cpp test/SemaCXX/warn-loop-analysis.cpp
Nico Weber
thakis at chromium.org
Thu May 3 13:51:54 PDT 2012
Hi Richard,
if you're looking for data if this should be on by default: This has 0
false positives in all of chromium (including tests, all dependencies) and
found 0 bugs.
(I did put in a true positive ("for (int i; i < 5;) {}") somewhere and
verified that that got detected, to make sure I enabled the warning
correctly. My first attempt to introduce a true positive, "int i; while (i
> 0) {}" didn't fire, I suppose while loops aren't handled intentionally?)
Nico
On Mon, Apr 30, 2012 at 11:01 AM, Richard Trieu <rtrieu at google.com> wrote:
> Author: rtrieu
> Date: Mon Apr 30 13:01:30 2012
> New Revision: 155835
>
> URL: http://llvm.org/viewvc/llvm-project?rev=155835&view=rev
> Log:
> Add -Wloop-analysis. This warning will fire on for loops which the
> variables
> in the loop conditional do not change.
>
> Added:
> cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/lib/Sema/SemaStmt.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=155835&r1=155834&r2=155835&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Apr 30
> 13:01:30 2012
> @@ -14,6 +14,12 @@
> let Component = "Sema" in {
> let CategoryName = "Semantic Issue" in {
>
> +// For loop analysis
> +def warn_variables_not_in_loop_body : Warning<
> + "variable%select{s| %1|s %1 and %2|s %1 %2 and %3|s %1 %2 %3 and %4}0 "
> + "used in loop condition not modified in loop body">,
> + InGroup<DiagGroup<"loop-analysis">>, DefaultIgnore;
> +
> // Constant expressions
> def err_expr_not_ice : Error<
> "expression is not an %select{integer|integral}0 constant expression">;
>
> Modified: cfe/trunk/lib/Sema/SemaStmt.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=155835&r1=155834&r2=155835&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaStmt.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Mon Apr 30 13:01:30 2012
> @@ -19,6 +19,7 @@
> #include "clang/AST/ASTContext.h"
> #include "clang/AST/CharUnits.h"
> #include "clang/AST/DeclObjC.h"
> +#include "clang/AST/EvaluatedExprVisitor.h"
> #include "clang/AST/ExprCXX.h"
> #include "clang/AST/ExprObjC.h"
> #include "clang/AST/StmtObjC.h"
> @@ -28,6 +29,7 @@
> #include "clang/Basic/TargetInfo.h"
> #include "llvm/ADT/ArrayRef.h"
> #include "llvm/ADT/STLExtras.h"
> +#include "llvm/ADT/SmallPtrSet.h"
> #include "llvm/ADT/SmallVector.h"
> using namespace clang;
> using namespace sema;
> @@ -1037,6 +1039,218 @@
> return Owned(new (Context) DoStmt(Body, Cond, DoLoc, WhileLoc,
> CondRParen));
> }
>
> +namespace {
> + // This visitor will traverse a conditional statement and store all
> + // the evaluated decls into a vector. Simple is set to true if none
> + // of the excluded constructs are used.
> + class DeclExtractor : public EvaluatedExprVisitor<DeclExtractor> {
> + llvm::SmallPtrSet<VarDecl*, 8> &Decls;
> + llvm::SmallVector<SourceRange, 10> &Ranges;
> + bool Simple;
> + PartialDiagnostic &PDiag;
> +public:
> + typedef EvaluatedExprVisitor<DeclExtractor> Inherited;
> +
> + DeclExtractor(Sema &S, llvm::SmallPtrSet<VarDecl*, 8> &Decls,
> + llvm::SmallVector<SourceRange, 10> &Ranges,
> + PartialDiagnostic &PDiag) :
> + Inherited(S.Context),
> + Decls(Decls),
> + Ranges(Ranges),
> + Simple(true),
> + PDiag(PDiag) {}
> +
> + bool isSimple() { return Simple; }
> +
> + // Replaces the method in EvaluatedExprVisitor.
> + void VisitMemberExpr(MemberExpr* E) {
> + Simple = false;
> + }
> +
> + // Any Stmt not whitelisted will cause the condition to be marked
> complex.
> + void VisitStmt(Stmt *S) {
> + Simple = false;
> + }
> +
> + void VisitBinaryOperator(BinaryOperator *E) {
> + Visit(E->getLHS());
> + Visit(E->getRHS());
> + }
> +
> + void VisitCastExpr(CastExpr *E) {
> + Visit(E->getSubExpr());
> + }
> +
> + void VisitUnaryOperator(UnaryOperator *E) {
> + // Skip checking conditionals with derefernces.
> + if (E->getOpcode() == UO_Deref)
> + Simple = false;
> + else
> + Visit(E->getSubExpr());
> + }
> +
> + void VisitConditionalOperator(ConditionalOperator *E) {
> + Visit(E->getCond());
> + Visit(E->getTrueExpr());
> + Visit(E->getFalseExpr());
> + }
> +
> + void VisitParenExpr(ParenExpr *E) {
> + Visit(E->getSubExpr());
> + }
> +
> + void VisitBinaryConditionalOperator(BinaryConditionalOperator *E) {
> + Visit(E->getOpaqueValue()->getSourceExpr());
> + Visit(E->getFalseExpr());
> + }
> +
> + void VisitIntegerLiteral(IntegerLiteral *E) { }
> + void VisitFloatingLiteral(FloatingLiteral *E) { }
> + void VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *E) { }
> + void VisitCharacterLiteral(CharacterLiteral *E) { }
> + void VisitGNUNullExpr(GNUNullExpr *E) { }
> + void VisitImaginaryLiteral(ImaginaryLiteral *E) { }
> +
> + void VisitDeclRefExpr(DeclRefExpr *E) {
> + VarDecl *VD = dyn_cast<VarDecl>(E->getDecl());
> + if (!VD) return;
> +
> + Ranges.push_back(E->getSourceRange());
> +
> + Decls.insert(VD);
> + }
> +
> + }; // end class DeclExtractor
> +
> + // DeclMatcher checks to see if the decls are used in a non-evauluated
> + // context.
> + class DeclMatcher : public EvaluatedExprVisitor<DeclMatcher> {
> + llvm::SmallPtrSet<VarDecl*, 8> &Decls;
> + bool FoundDecl;
> + //bool EvalDecl;
> +
> +public:
> + typedef EvaluatedExprVisitor<DeclMatcher> Inherited;
> +
> + DeclMatcher(Sema &S, llvm::SmallPtrSet<VarDecl*, 8> &Decls, Stmt
> *Statement) :
> + Inherited(S.Context), Decls(Decls), FoundDecl(false) {
> + if (!Statement) return;
> +
> + Visit(Statement);
> + }
> +
> + void VisitReturnStmt(ReturnStmt *S) {
> + FoundDecl = true;
> + }
> +
> + void VisitBreakStmt(BreakStmt *S) {
> + FoundDecl = true;
> + }
> +
> + void VisitGotoStmt(GotoStmt *S) {
> + FoundDecl = true;
> + }
> +
> + void VisitCastExpr(CastExpr *E) {
> + if (E->getCastKind() == CK_LValueToRValue)
> + CheckLValueToRValueCast(E->getSubExpr());
> + else
> + Visit(E->getSubExpr());
> + }
> +
> + void CheckLValueToRValueCast(Expr *E) {
> + E = E->IgnoreParenImpCasts();
> +
> + if (isa<DeclRefExpr>(E)) {
> + return;
> + }
> +
> + if (ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E)) {
> + Visit(CO->getCond());
> + CheckLValueToRValueCast(CO->getTrueExpr());
> + CheckLValueToRValueCast(CO->getFalseExpr());
> + return;
> + }
> +
> + if (BinaryConditionalOperator *BCO =
> + dyn_cast<BinaryConditionalOperator>(E)) {
> + CheckLValueToRValueCast(BCO->getOpaqueValue()->getSourceExpr());
> + CheckLValueToRValueCast(BCO->getFalseExpr());
> + return;
> + }
> +
> + Visit(E);
> + }
> +
> + void VisitDeclRefExpr(DeclRefExpr *E) {
> + if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))
> + if (Decls.count(VD))
> + FoundDecl = true;
> + }
> +
> + bool FoundDeclInUse() { return FoundDecl; }
> +
> + }; // end class DeclMatcher
> +
> + void CheckForLoopConditionalStatement(Sema &S, Expr *Second,
> + Expr *Third, Stmt *Body) {
> + // Condition is empty
> + if (!Second) return;
> +
> + if (S.Diags.getDiagnosticLevel(diag::warn_variables_not_in_loop_body,
> + Second->getLocStart())
> + == DiagnosticsEngine::Ignored)
> + return;
> +
> + PartialDiagnostic PDiag =
> S.PDiag(diag::warn_variables_not_in_loop_body);
> + llvm::SmallPtrSet<VarDecl*, 8> Decls;
> + llvm::SmallVector<SourceRange, 10> Ranges;
> + DeclExtractor DE(S, Decls, Ranges, PDiag);
> + DE.Visit(Second);
> +
> + // Don't analyze complex conditionals.
> + if (!DE.isSimple()) return;
> +
> + // No decls found.
> + if (Decls.size() == 0) return;
> +
> + // Don't warn on volatile decls.
> + for (llvm::SmallPtrSet<VarDecl*, 8>::iterator I = Decls.begin(),
> + E = Decls.end();
> + I != E; ++I)
> + if ((*I)->getType().isVolatileQualified()) return;
> +
> + if (DeclMatcher(S, Decls, Second).FoundDeclInUse() ||
> + DeclMatcher(S, Decls, Third).FoundDeclInUse() ||
> + DeclMatcher(S, Decls, Body).FoundDeclInUse())
> + return;
> +
> + // Load decl names into diagnostic.
> + if (Decls.size() > 4)
> + PDiag << 0;
> + else {
> + PDiag << Decls.size();
> + for (llvm::SmallPtrSet<VarDecl*, 8>::iterator I = Decls.begin(),
> + E = Decls.end();
> + I != E; ++I)
> + PDiag << (*I)->getDeclName();
> + }
> +
> + // Load SourceRanges into diagnostic if there is room.
> + // Otherwise, load the SourceRange of the conditional expression.
> + if (Ranges.size() <= PartialDiagnostic::MaxArguments)
> + for (llvm::SmallVector<SourceRange, 10>::iterator I =
> Ranges.begin(),
> + E = Ranges.end();
> + I != E; ++I)
> + PDiag << *I;
> + else
> + PDiag << Second->getSourceRange();
> +
> + S.Diag(Ranges.begin()->getBegin(), PDiag);
> + }
> +
> +} // end namespace
> +
> StmtResult
> Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
> Stmt *First, FullExprArg second, Decl *secondVar,
> @@ -1059,6 +1273,8 @@
> }
> }
>
> + CheckForLoopConditionalStatement(*this, second.get(), third.get(),
> Body);
> +
> ExprResult SecondResult(second.release());
> VarDecl *ConditionVar = 0;
> if (secondVar) {
>
> Added: cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp?rev=155835&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp (added)
> +++ cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp Mon Apr 30 13:01:30 2012
> @@ -0,0 +1,146 @@
> +// RUN: %clang_cc1 -fsyntax-only -Wloop-analysis -verify %s
> +
> +struct S {
> + bool stop() { return false; }
> + bool keep_running;
> +};
> +
> +void by_ref(int &value) { }
> +void by_value(int value) { }
> +void by_pointer(int *value) {}
> +
> +void test1() {
> + S s;
> + for (; !s.stop();) {}
> + for (; s.keep_running;) {}
> + for (int i; i < 1; ++i) {}
> + for (int i; i < 1; ) {} // expected-warning {{variable 'i' used in
> loop condition not modified in loop body}}
> + for (int i; i < 1; ) { ++i; }
> + for (int i; i < 1; ) { return; }
> + for (int i; i < 1; ) { break; }
> + for (int i; i < 1; ) { goto exit_loop; }
> +exit_loop:
> + for (int i; i < 1; ) { by_ref(i); }
> + for (int i; i < 1; ) { by_value(i); } // expected-warning {{variable
> 'i' used in loop condition not modified in loop body}}
> + for (int i; i < 1; ) { by_pointer(&i); }
> +
> + for (int i; i < 1; ++i)
> + for (int j; j < 1; ++j)
> + { }
> + for (int i; i < 1; ++i)
> + for (int j; j < 1; ++i) // expected-warning {{variable 'j' used in
> loop condition not modified in loop body}}
> + { }
> + for (int i; i < 1; ++i)
> + for (int j; i < 1; ++j) // expected-warning {{variable 'i' used in
> loop condition not modified in loop body}}
> + { }
> +
> + for (int *i, *j; i < j; ++i) {}
> + for (int *i, *j; i < j;) {} // expected-warning {{variables 'i' and
> 'j' used in loop condition not modified in loop body}}
> +
> + // Dereferencing pointers is ignored for now.
> + for (int *i; *i; ) {}
> +}
> +
> +void test2() {
> + int i, j, k;
> + int *ptr;
> +
> + // Testing CastExpr
> + for (; i; ) {} // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; i; ) { i = 5; }
> +
> + // Testing BinaryOperator
> + for (; i < j; ) {} // expected-warning {{variables 'i' and 'j' used in
> loop condition not modified in loop body}}
> + for (; i < j; ) { i = 5; }
> + for (; i < j; ) { j = 5; }
> +
> + // Testing IntegerLiteral
> + for (; i < 5; ) {} // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; i < 5; ) { i = 5; }
> +
> + // Testing FloatingLiteral
> + for (; i < 5.0; ) {} // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; i < 5.0; ) { i = 5; }
> +
> + // Testing CharacterLiteral
> + for (; i == 'a'; ) {} // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; i == 'a'; ) { i = 5; }
> +
> + // Testing CXXBoolLiteralExpr
> + for (; i == true; ) {} // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; i == true; ) { i = 5; }
> +
> + // Testing GNUNullExpr
> + for (; ptr == __null; ) {} // expected-warning {{variable 'ptr' used in
> loop condition not modified in loop body}}
> + for (; ptr == __null; ) { ptr = &i; }
> +
> + // Testing UnaryOperator
> + for (; -i > 5; ) {} // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; -i > 5; ) { ++i; }
> +
> + // Testing ImaginaryLiteral
> + for (; i != 3i; ) {} // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; i != 3i; ) { ++i; }
> +
> + // Testing ConditionalOperator
> + for (; i ? j : k; ) {} // expected-warning {{variables 'i' 'j' and 'k'
> used in loop condition not modified in loop body}}
> + for (; i ? j : k; ) { ++i; }
> + for (; i ? j : k; ) { ++j; }
> + for (; i ? j : k; ) { ++k; }
> + for (; i; ) { j = i ? i : i; } // expected-warning {{variable 'i' used
> in loop condition not modified in loop body}}
> + for (; i; ) { j = (i = 1) ? i : i; }
> + for (; i; ) { j = i ? i : ++i; }
> +
> + // Testing BinaryConditionalOperator
> + for (; i ?: j; ) {} // expected-warning {{variables 'i' and 'j' used in
> loop condition not modified in loop body}}
> + for (; i ?: j; ) { ++i; }
> + for (; i ?: j; ) { ++j; }
> + for (; i; ) { j = i ?: i; } // expected-warning {{variable 'i' used in
> loop condition not modified in loop body}}
> +
> + // Testing ParenExpr
> + for (; (i); ) { } // expected-warning {{variable 'i' used in loop
> condition not modified in loop body}}
> + for (; (i); ) { ++i; }
> +
> + // Testing non-evaluated variables
> + for (; i < sizeof(j); ) { } // expected-warning {{variable 'i' used in
> loop condition not modified in loop body}}
> + for (; i < sizeof(j); ) { ++j; } // expected-warning {{variable 'i'
> used in loop condition not modified in loop body}}
> + for (; i < sizeof(j); ) { ++i; }
> +}
> +
> +// False positive and how to silence.
> +void test3() {
> + int x;
> + int *ptr = &x;
> + for (;x<5;) { *ptr = 6; } // expected-warning {{variable 'x' used in
> loop condition not modified in loop body}}
> +
> + for (;x<5;) {
> + *ptr = 6;
> + (void)x;
> + }
> +}
> +
> +// Check ordering and printing of variables. Max variables is currently
> 4.
> +void test4() {
> + int a, b, c, d, e, f;
> + for (; a;); // expected-warning {{variable 'a' used in loop condition
> not modified in loop body}}
> + for (; a + b;); // expected-warning {{variables 'a' and 'b' used in
> loop condition not modified in loop body}}
> + for (; a + b + c;); // expected-warning {{variables 'a' 'b' and 'c'
> used in loop condition not modified in loop body}}
> + for (; a + b + c + d;); // expected-warning {{variables 'a' 'b' 'c'
> and 'd' used in loop condition not modified in loop body}}
> + for (; a + b + c + d + e;); // expected-warning {{variables used in
> loop condition not modified in loop body}}
> + for (; a + b + c + d + e + f;); // expected-warning {{variables used
> in loop condition not modified in loop body}}
> + for (; a + c + d + b;); // expected-warning {{variables 'a' 'c' 'd'
> and 'b' used in loop condition not modified in loop body}}
> + for (; d + c + b + a;); // expected-warning {{variables 'd' 'c' 'b'
> and 'a' used in loop condition not modified in loop body}}
> +}
> +
> +// Ensure that the warning doesn't fail when lots of variables are used
> +// in the conditional.
> +void test5() {
> + for (int a; a+a+a+a+a+a+a+a+a+a;); // \
> + // expected-warning {{variable 'a' used in loop condition not modified
> in loop body}}
> + for (int a; a+a+a+a+a+a+a+a+a+a+a;); // \
> + // expected-warning {{variable 'a' used in loop condition not modified
> in loop body}}
> + for (int a; a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a;); // \
> + // expected-warning {{variable 'a' used in loop condition not modified
> in loop body}}
> + for (int a;
> a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a+a;);//\
> + // expected-warning {{variable 'a' used in loop condition not modified
> in loop body}}
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120503/f326ef19/attachment.html>
More information about the cfe-commits
mailing list