Index: lib/Analysis/CFG.cpp =================================================================== --- lib/Analysis/CFG.cpp (revision 194562) +++ lib/Analysis/CFG.cpp (working copy) @@ -483,6 +483,192 @@ B->addSuccessor(S, cfg->getBumpVectorContext()); } + /// \brief Find a relational comparison with an expression evaluating to a + /// boolean and a constant other than 0 and 1. + /// e.g. if ((x < y) == 10) + TryResult checkIncorrectRelationalOperator(const BinaryOperator *B) { + const IntegerLiteral *IntLiteral = + dyn_cast(B->getLHS()->IgnoreParens()); + const Expr *BoolExpr = B->getRHS()->IgnoreParens(); + bool IntFirst = true; + if (!IntLiteral) { + IntLiteral = dyn_cast(B->getRHS()->IgnoreParens()); + BoolExpr = B->getLHS()->IgnoreParens(); + IntFirst = false; + } + + if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue()) + return TryResult(); + + llvm::APInt IntValue = IntLiteral->getValue(); + if ((IntValue != 1) && (IntValue != 0)) { + BuildOpts.Observer->compareBoolWithInt(B); + bool IntLarger = !IntValue.isNegative(); + BinaryOperatorKind Bok = B->getOpcode(); + if (Bok == BO_GT || Bok == BO_GE) { + return TryResult(IntFirst != IntLarger); + } else { + return TryResult(IntFirst == IntLarger); + } + } + return TryResult(); + } + + /// Find a equality comparison with an expression evaluating to a boolean and + /// a constant other than 0 and 1. + /// e.g. if (!x == 10) + TryResult checkIncorrectEqualityOperator(const BinaryOperator *B) { + const IntegerLiteral *IntLiteral = + dyn_cast(B->getLHS()->IgnoreParens()); + const Expr *BoolExpr = B->getRHS()->IgnoreParens(); + + if (!IntLiteral) { + IntLiteral = dyn_cast(B->getRHS()->IgnoreParens()); + BoolExpr = B->getLHS()->IgnoreParens(); + } + + if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue()) + return TryResult(); + + llvm::APInt IntValue = IntLiteral->getValue(); + if ((IntValue != 1) && (IntValue != 0)) { + BuildOpts.Observer->compareBoolWithInt(B); + return TryResult(B->getOpcode() != BO_EQ); + } + return TryResult(); + } + + bool analyzeLogicOperatorCondition(BinaryOperatorKind Relation, + const llvm::APSInt Value1, + const llvm::APSInt Value2) { + return (Relation == BO_EQ && (Value1 == Value2)) || + (Relation == BO_NE && (Value1 != Value2)) || + (Relation == BO_LT && (Value1 < Value2)) || + (Relation == BO_LE && (Value1 <= Value2)) || + (Relation == BO_GT && (Value1 > Value2)) || + (Relation == BO_GE && (Value1 >= Value2)); + } + + /// \brief Find a pair of comparison expressions with or without parentheses + /// with a shared variable and constants and a logical operator between them + /// that always evaluates to either true or false. + /// e.g. if (x != 3 || x != 4) + TryResult checkIncorrectLogicOperator(const BinaryOperator *B) { + const BinaryOperator *LHS = + dyn_cast(B->getLHS()->IgnoreParens()); + const BinaryOperator *RHS = + dyn_cast(B->getRHS()->IgnoreParens()); + if (!LHS || !RHS) + return TryResult(); + + if (!LHS->isComparisonOp() || !RHS->isComparisonOp()) + return TryResult(); + + BinaryOperatorKind BO1 = LHS->getOpcode(); + const DeclRefExpr *Decl1 = + dyn_cast(LHS->getLHS()->IgnoreParenImpCasts()); + const IntegerLiteral *Literal1 = + dyn_cast(LHS->getRHS()->IgnoreParens()); + + if (!Decl1 && !Literal1) { + if (BO1 == BO_GT) + BO1 = BO_LT; + else if (BO1 == BO_GE) + BO1 = BO_LE; + else if (BO1 == BO_LT) + BO1 = BO_GT; + else if (BO1 == BO_LE) + BO1 = BO_GE; + Decl1 = + dyn_cast(LHS->getRHS()->IgnoreParenImpCasts()); + Literal1 = dyn_cast(LHS->getLHS()->IgnoreParens()); + } + + if (!Decl1 || !Literal1) + return TryResult(); + + BinaryOperatorKind BO2 = RHS->getOpcode(); + const DeclRefExpr *Decl2 = + dyn_cast(RHS->getLHS()->IgnoreImpCasts()->IgnoreParens()); + const IntegerLiteral *Literal2 = + dyn_cast(RHS->getRHS()->IgnoreParens()); + + if (!Decl2 && !Literal2) { + if (BO2 == BO_GT) + BO2 = BO_LT; + else if (BO2 == BO_GE) + BO2 = BO_LE; + else if (BO2 == BO_LT) + BO2 = BO_GT; + else if (BO2 == BO_LE) + BO2 = BO_GE; + Decl2 = + dyn_cast(RHS->getRHS()->IgnoreImpCasts()->IgnoreParens()); + Literal2 = dyn_cast(RHS->getLHS()->IgnoreParens()); + } + + if (!Decl2 || !Literal2) + return TryResult(); + + // Check that it is the same variable on both sides. + if (Decl1->getNameInfo().getAsString() != Decl2->getNameInfo().getAsString()) + return TryResult(); + + // evaluate if expression is always true/false + llvm::APSInt L1,L2; + if (!Literal1->EvaluateAsInt(L1,*Context) || + !Literal2->EvaluateAsInt(L2,*Context)) + return TryResult(); + + const llvm::APSInt MinValueL1 = + llvm::APSInt::getMinValue(L1.getBitWidth(),L1.isUnsigned()); + const llvm::APSInt MaxValueL1 = + llvm::APSInt::getMaxValue(L1.getBitWidth(),L1.isUnsigned()); + + const llvm::APSInt MinValueL2 = + llvm::APSInt::getMinValue(L2.getBitWidth(),L2.isUnsigned()); + const llvm::APSInt MaxValueL2 = + llvm::APSInt::getMaxValue(L2.getBitWidth(),L2.isUnsigned()); + bool AlwaysTrue = true, AlwaysFalse = true; + for (int Offset = -3; Offset <=3; ++Offset) { + for (int Selvalue = 1; Selvalue <= 2; Selvalue++) { + + llvm::APSInt SelInt = (Selvalue==1 ? L1 : L2); + llvm::APSInt SelMin = (Selvalue==1 ? MinValueL1 : MinValueL2); + llvm::APSInt SelMax = (Selvalue==1 ? MaxValueL1 : MaxValueL2); + + llvm::APSInt OffsetAPS = + llvm::APSInt(llvm::APInt(SelInt.getBitWidth(), Offset), + SelInt.isUnsigned()); + + if ((SelInt + OffsetAPS) > SelMax) + continue; + else if ((SelInt + OffsetAPS) < SelMin) + continue; + else + SelInt += OffsetAPS; + + bool Res1,Res2; + Res1 = analyzeLogicOperatorCondition(BO1,SelInt, L1); + Res2 = analyzeLogicOperatorCondition(BO2,SelInt, L2); + if (B->getOpcode() == BO_LAnd) { + AlwaysTrue &= (Res1 && Res2); + AlwaysFalse &= !(Res1 && Res2); + } else { + AlwaysTrue &= (Res1 || Res2); + AlwaysFalse &= !(Res1 || Res2); + } + } + } + + if(AlwaysTrue || AlwaysFalse) { + BuildOpts.Observer->compareAlwaysTrue(B,AlwaysTrue); + return TryResult(AlwaysTrue); + } + + return TryResult(); + } + /// Try and evaluate an expression to an integer constant. bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) { if (!BuildOpts.PruneTriviallyFalseEdges) @@ -565,13 +751,24 @@ // is determined by the RHS: X && 0 -> 0, X || 1 -> 1. if (RHS.isTrue() == (Bop->getOpcode() == BO_LOr)) return RHS.isTrue(); + } else { + TryResult BopRes = checkIncorrectLogicOperator(Bop); + if (BopRes.isKnown()) + return BopRes.isTrue(); } } return TryResult(); + } else if (Bop->isEqualityOp()) { + TryResult BopRes = checkIncorrectEqualityOperator(Bop); + if (BopRes.isKnown()) + return BopRes.isTrue(); + } else if (Bop->isRelationalOp()) { + TryResult BopRes = checkIncorrectRelationalOperator(Bop); + if (BopRes.isKnown()) + return BopRes.isTrue(); } } - bool Result; if (E->EvaluateAsBooleanCondition(Result, *Context)) return Result; @@ -1311,6 +1508,8 @@ else { RHSBlock->setTerminator(Term); TryResult KnownVal = tryEvaluateBool(RHS); + if (!KnownVal.isKnown()) + KnownVal = tryEvaluateBool(B); addSuccessor(RHSBlock, KnownVal.isFalse() ? NULL : TrueBlock); addSuccessor(RHSBlock, KnownVal.isTrue() ? NULL : FalseBlock); } Index: lib/Sema/AnalysisBasedWarnings.cpp =================================================================== --- lib/Sema/AnalysisBasedWarnings.cpp (revision 194562) +++ lib/Sema/AnalysisBasedWarnings.cpp (working copy) @@ -77,6 +77,60 @@ reachable_code::FindUnreachableCode(AC, UC); } +/// \brief Warn on logical operator errors in CFGBuilder +class LogicalErrorsHandler : public CFGCallback { + Sema &S; +public: + LogicalErrorsHandler(Sema &S) : CFGCallback(),S(S) {} + void compareAlwaysTrue(const BinaryOperator* B, bool isAlwaysTrue) { + if (B->getLHS()->getExprLoc().isMacroID() || + B->getRHS()->getExprLoc().isMacroID()) + return; + + const BinaryOperator *LHS = + dyn_cast(B->getLHS()->IgnoreParens()); + const BinaryOperator *RHS = + dyn_cast(B->getRHS()->IgnoreParens()); + + if (LHS) + if (LHS->getLHS()->getExprLoc().isMacroID() || + LHS->getRHS()->getExprLoc().isMacroID()) + return; + if (RHS) + if (RHS->getLHS()->getExprLoc().isMacroID() || + RHS->getRHS()->getExprLoc().isMacroID()) + return; + + SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()->getLocEnd()); + S.Diag(B->getExprLoc(), diag::warn_operator_always_true_comparison) + << DiagRange << isAlwaysTrue; + } + + void compareBoolWithInt(const BinaryOperator* B) { + if (B->getLHS()->getExprLoc().isMacroID() || + B->getRHS()->getExprLoc().isMacroID()) + return; + + const BinaryOperator *LHS = + dyn_cast(B->getLHS()->IgnoreParens()); + const BinaryOperator *RHS = + dyn_cast(B->getRHS()->IgnoreParens()); + + if (LHS) + if (LHS->getLHS()->getExprLoc().isMacroID() || + LHS->getRHS()->getExprLoc().isMacroID()) + return; + if (RHS) + if (RHS->getLHS()->getExprLoc().isMacroID() || + RHS->getRHS()->getExprLoc().isMacroID()) + return; + + SourceRange DiagRange(B->getLHS()->getLocStart(), B->getRHS()->getLocEnd()); + S.Diag(B->getExprLoc(), diag::warn_bool_and_int_comparison) << DiagRange; + } +}; + + //===----------------------------------------------------------------------===// // Check for missing return value. //===----------------------------------------------------------------------===// @@ -1723,8 +1777,11 @@ bool isTemplateInstantiation = false; if (const FunctionDecl *Function = dyn_cast(D)) isTemplateInstantiation = Function->isTemplateInstantiation(); - if (!isTemplateInstantiation) + if (!isTemplateInstantiation) { + LogicalErrorsHandler Leh(S); + AC.getCFGBuildOptions().Observer = &Leh; CheckUnreachable(S, AC); + } } // Check for thread safety violations Index: lib/AST/Expr.cpp =================================================================== --- lib/AST/Expr.cpp (revision 194562) +++ lib/AST/Expr.cpp (working copy) @@ -152,6 +152,8 @@ switch (UO->getOpcode()) { case UO_Plus: return UO->getSubExpr()->isKnownToHaveBooleanValue(); + case UO_LNot: + return true; default: return false; } Index: test/Sema/warn-overlap.c =================================================================== --- test/Sema/warn-overlap.c (revision 0) +++ test/Sema/warn-overlap.c (working copy) @@ -0,0 +1,51 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s + +#define mydefine 2 + +void f(int x) { + int y = 0; + // > || < + if (x > 2 || x < 1) { } + if (x > 2 || x < 2) { } + if (x != 2 || x != 3) { } // expected-warning {{logical disjunction always evaluates to true}} + if (x > 2 || x < 3) { } // expected-warning {{logical disjunction always evaluates to true}} + + if (x > 2 || x <= 1) { } + if (x > 2 || x <= 2) { } // expected-warning {{logical disjunction always evaluates to true}} + if (x > 2 || x <= 3) { } // expected-warning {{logical disjunction always evaluates to true}} + + if (x >= 2 || x < 1) { } + if (x >= 2 || x < 2) { } // expected-warning {{logical disjunction always evaluates to true}} + if (x >= 2 || x < 3) { } // expected-warning {{logical disjunction always evaluates to true}} + + if (x >= 2 || x <= 1) { } // expected-warning {{logical disjunction always evaluates to true}} + if (x >= 2 || x <= 2) { } // expected-warning {{logical disjunction always evaluates to true}} + if (x >= 2 || x <= 3) { } // expected-warning {{logical disjunction always evaluates to true}} + + // > && < + if (x > 2 && x < 1) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x > 2 && x < 2) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x > 2 && x < 3) { } // expected-warning {{logical disjunction always evaluates to false}} + + if (x > 2 && x <= 1) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x > 2 && x <= 2) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x > 2 && x <= 3) { } + + if (x >= 2 && x < 1) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x >= 2 && x < 2) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x >= 2 && x < 3) { } + + if (x >= 2 && x <= 1) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x >= 2 && x <= 2) { } + if (x >= 2 && x <= 3) { } + + // !=, ==, .. + if (x != 2 || x != 3) { } // expected-warning {{logical disjunction always evaluates to true}} + if (x != 2 || x < 3) { } // expected-warning {{logical disjunction always evaluates to true}} + if (x == 2 && x == 3) { } // expected-warning {{logical disjunction always evaluates to false}} + if (x == 2 && x > 3) { } // expected-warning {{logical disjunction always evaluates to false}} + + if (x == mydefine && x > 3) { } // no-warning + if (x == 2 && y > 3) { } // no-warning +} + Index: test/Sema/warn-compint.c =================================================================== --- test/Sema/warn-compint.c (revision 0) +++ test/Sema/warn-compint.c (working copy) @@ -0,0 +1,39 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -Wunreachable-code %s + +#define mydefine 2 + +void err(int x, int y) { + + if (!x == 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if (!x != 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if (!x < 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if (!x > 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if (!x >= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if (!x <= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + + if ((x < y) == 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if ((x < y) != 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if ((x < y) < 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if ((x < y) > 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if ((x < y) >= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} + if ((x < y) <= 10) {} // expected-warning {{comparison of a boolean expression with an integer other than 0 or 1}} +} + +void noerr(int x, int y) { + + if (!x == 1) {} + if (!x != 0) {} + if (!x < 1) {} + if (!x > 0) {} + if (!x >= 1) {} + if (!x <= 0) {} + + if ((x < y) == 1) {} + if ((x < y) != 0) {} + if ((x < y) < 1) {} + if ((x < y) > 0) {} + if ((x < y) >= 1) {} + if ((x < y) <= 0) {} + + if ((x < mydefine) <= 10) {} +} // no-warning Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td (revision 194562) +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy) @@ -339,6 +339,12 @@ InGroup, DefaultIgnore; def warn_unreachable : Warning<"will never be executed">, InGroup>, DefaultIgnore; +def warn_operator_always_true_comparison : Warning< + "logical disjunction always evaluates to %select{false|true}0">, + InGroup>, DefaultIgnore; +def warn_bool_and_int_comparison : Warning< + "comparison of a boolean expression with an integer other than 0 or 1">, + InGroup>, DefaultIgnore; /// Built-in functions. def ext_implicit_lib_function_decl : ExtWarn< Index: include/clang/Analysis/CFG.h =================================================================== --- include/clang/Analysis/CFG.h (revision 194562) +++ include/clang/Analysis/CFG.h (working copy) @@ -45,6 +45,7 @@ class ASTContext; class CXXRecordDecl; class CXXDeleteExpr; + class BinaryOperator; /// CFGElement - Represents a top-level expression in a basic block. class CFGElement { @@ -609,6 +610,16 @@ } }; +/// \brief CFGCallback defines methods that should be called when a logical +/// operator error is found when building the CFG. +class CFGCallback { +public: + CFGCallback(){} + virtual void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}; + virtual void compareBoolWithInt(const BinaryOperator *B) {}; + virtual ~CFGCallback(){} +}; + /// CFG - Represents a source-level, intra-procedural CFG that represents the /// control-flow of a Stmt. The Stmt can represent an entire function body, /// or a single expression. A CFG will always contain one empty block that @@ -627,7 +638,7 @@ public: typedef llvm::DenseMap ForcedBlkExprs; ForcedBlkExprs **forcedBlkExprs; - + CFGCallback* Observer; bool PruneTriviallyFalseEdges; bool AddEHEdges; bool AddInitializers; @@ -650,7 +661,7 @@ } BuildOptions() - : forcedBlkExprs(0), PruneTriviallyFalseEdges(true) + : forcedBlkExprs(0), Observer(0), PruneTriviallyFalseEdges(true) ,AddEHEdges(false) ,AddInitializers(false) ,AddImplicitDtors(false)