From: Richard Trieu Date: Sat, 5 Apr 2014 05:17:01 +0000 (+0000) Subject: Add a new subgroup to -Wtautological-compare, -Wtautological-overlap-compare, X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8123887315f245efdafb56f151dd5048afb2f38c;p=clang Add a new subgroup to -Wtautological-compare, -Wtautological-overlap-compare, which warns on compound conditionals that always evaluate to the same value. For instance, (x > 5 && x < 3) will always be false since no value for x can satisfy both conditions. This patch also changes the CFG to use these tautological values for better branch analysis. The test for -Wunreachable-code shows how this change catches additional dead code. Patch by Anders Rönnholm. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@205665 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/CFG.h b/include/clang/Analysis/CFG.h index 123d1443a2..87ba26db4f 100644 --- a/include/clang/Analysis/CFG.h +++ b/include/clang/Analysis/CFG.h @@ -47,6 +47,7 @@ namespace clang { class CXXRecordDecl; class CXXDeleteExpr; class CXXNewExpr; + class BinaryOperator; /// CFGElement - Represents a top-level expression in a basic block. class CFGElement { @@ -698,6 +699,15 @@ public: } }; +/// \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 ~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 @@ -716,7 +726,7 @@ public: public: typedef llvm::DenseMap ForcedBlkExprs; ForcedBlkExprs **forcedBlkExprs; - + CFGCallback *Observer; bool PruneTriviallyFalseEdges; bool AddEHEdges; bool AddInitializers; @@ -740,7 +750,7 @@ public: } BuildOptions() - : forcedBlkExprs(0), PruneTriviallyFalseEdges(true) + : forcedBlkExprs(0), Observer(0), PruneTriviallyFalseEdges(true) ,AddEHEdges(false) ,AddInitializers(false) ,AddImplicitDtors(false) diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 0414f4cd21..c21a313386 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -291,9 +291,11 @@ def StringPlusChar : DiagGroup<"string-plus-char">; def StrncatSize : DiagGroup<"strncat-size">; def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">; def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">; +def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">; def TautologicalCompare : DiagGroup<"tautological-compare", [TautologicalOutOfRangeCompare, - TautologicalPointerCompare]>; + TautologicalPointerCompare, + TautologicalOverlapCompare]>; def HeaderHygiene : DiagGroup<"header-hygiene">; def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">; def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index fad654f3a5..1dd7f815be 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -6383,6 +6383,9 @@ def note_ref_subobject_of_member_declared_here : Note< def warn_comparison_always : Warning< "%select{self-|array }0comparison always evaluates to %select{false|true|a constant}1">, InGroup; +def warn_tautological_overlap_comparison : Warning< + "overlapping comparisons always evaluate to %select{false|true}0">, + InGroup, DefaultIgnore; def warn_stringcompare : Warning< "result of comparison against %select{a string literal|@encode}0 is " diff --git a/lib/Analysis/CFG.cpp b/lib/Analysis/CFG.cpp index 400c2025bc..7c716116eb 100644 --- a/lib/Analysis/CFG.cpp +++ b/lib/Analysis/CFG.cpp @@ -495,6 +495,215 @@ private: 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 Expr *LHSExpr = B->getLHS()->IgnoreParens(); + const Expr *RHSExpr = B->getRHS()->IgnoreParens(); + + const IntegerLiteral *IntLiteral = dyn_cast(LHSExpr); + const Expr *BoolExpr = RHSExpr; + bool IntFirst = true; + if (!IntLiteral) { + IntLiteral = dyn_cast(RHSExpr); + BoolExpr = LHSExpr; + IntFirst = false; + } + + if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue()) + return TryResult(); + + llvm::APInt IntValue = IntLiteral->getValue(); + if ((IntValue == 1) || (IntValue == 0)) + return TryResult(); + + bool IntLarger = IntLiteral->getType()->isUnsignedIntegerType() || + !IntValue.isNegative(); + + BinaryOperatorKind Bok = B->getOpcode(); + if (Bok == BO_GT || Bok == BO_GE) { + // Always true for 10 > bool and bool > -1 + // Always false for -1 > bool and bool > 10 + return TryResult(IntFirst == IntLarger); + } else { + // Always true for -1 < bool and bool < 10 + // Always false for 10 < bool and bool < -1 + return TryResult(IntFirst != IntLarger); + } + } + + /// 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 Expr *LHSExpr = B->getLHS()->IgnoreParens(); + const Expr *RHSExpr = B->getRHS()->IgnoreParens(); + + const IntegerLiteral *IntLiteral = dyn_cast(LHSExpr); + const Expr *BoolExpr = RHSExpr; + + if (!IntLiteral) { + IntLiteral = dyn_cast(RHSExpr); + BoolExpr = LHSExpr; + } + + if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue()) + return TryResult(); + + llvm::APInt IntValue = IntLiteral->getValue(); + if ((IntValue == 1) || (IntValue == 0)) { + return TryResult(); + } + + return TryResult(B->getOpcode() != BO_EQ); + } + + TryResult analyzeLogicOperatorCondition(BinaryOperatorKind Relation, + const llvm::APSInt &Value1, + const llvm::APSInt &Value2) { + assert(Value1.isSigned() == Value2.isSigned()); + switch (Relation) { + default: + return TryResult(); + case BO_EQ: + return TryResult(Value1 == Value2); + case BO_NE: + return TryResult(Value1 != Value2); + case BO_LT: + return TryResult(Value1 < Value2); + case BO_LE: + return TryResult(Value1 <= Value2); + case BO_GT: + return TryResult(Value1 > Value2); + case BO_GE: + return TryResult(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) { + assert(B->isLogicalOp()); + 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()->IgnoreParenImpCasts()); + 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()->IgnoreParenImpCasts()); + Literal2 = dyn_cast(RHS->getLHS()->IgnoreParens()); + } + + if (!Decl2 || !Literal2) + return TryResult(); + + // Check that it is the same variable on both sides. + if (Decl1->getDecl() != Decl2->getDecl()) + return TryResult(); + + llvm::APSInt L1, L2; + + if (!Literal1->EvaluateAsInt(L1, *Context) || + !Literal2->EvaluateAsInt(L2, *Context)) + return TryResult(); + + // Can't compare signed with unsigned or with different bit width. + if (L1.isSigned() != L2.isSigned() || L1.getBitWidth() != L2.getBitWidth()) + return TryResult(); + + // Values that will be used to determine if result of logical + // operator is always true/false + const llvm::APSInt Values[] = { + // Value less than both Value1 and Value2 + llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned()), + // L1 + L1, + // Value between Value1 and Value2 + ((L1 < L2) ? L1 : L2) + llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1), + L1.isUnsigned()), + // L2 + L2, + // Value greater than both Value1 and Value2 + llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned()), + }; + + // Check whether expression is always true/false by evaluating the following + // * variable x is less than the smallest literal. + // * variable x is equal to the smallest literal. + // * Variable x is between smallest and largest literal. + // * Variable x is equal to the largest literal. + // * Variable x is greater than largest literal. + bool AlwaysTrue = true, AlwaysFalse = true; + for (unsigned int ValueIndex = 0; + ValueIndex < sizeof(Values) / sizeof(Values[0]); + ++ValueIndex) { + llvm::APSInt Value = Values[ValueIndex]; + TryResult Res1, Res2; + Res1 = analyzeLogicOperatorCondition(BO1, Value, L1); + Res2 = analyzeLogicOperatorCondition(BO2, Value, L2); + + if (!Res1.isKnown() || !Res2.isKnown()) + return TryResult(); + + if (B->getOpcode() == BO_LAnd) { + AlwaysTrue &= (Res1.isTrue() && Res2.isTrue()); + AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue()); + } else { + AlwaysTrue &= (Res1.isTrue() || Res2.isTrue()); + AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue()); + } + } + + if (AlwaysTrue || AlwaysFalse) { + if (BuildOpts.Observer) + 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) @@ -577,10 +786,22 @@ private: // 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(); } } @@ -1320,6 +1541,8 @@ CFGBuilder::VisitLogicalOperator(BinaryOperator *B, else { RHSBlock->setTerminator(Term); TryResult KnownVal = tryEvaluateBool(RHS); + if (!KnownVal.isKnown()) + KnownVal = tryEvaluateBool(B); addSuccessor(RHSBlock, TrueBlock, !KnownVal.isFalse()); addSuccessor(RHSBlock, FalseBlock, !KnownVal.isTrue()); } diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index a489e8c911..32bdef6da9 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -117,6 +117,38 @@ static void CheckUnreachable(Sema &S, AnalysisDeclContext &AC) { reachable_code::FindUnreachableCode(AC, S.getPreprocessor(), UC); } +/// \brief Warn on logical operator errors in CFGBuilder +class LogicalErrorHandler : public CFGCallback { + Sema &S; + +public: + LogicalErrorHandler(Sema &S) : CFGCallback(), S(S) {} + + static bool HasMacroID(const Expr *E) { + if (E->getExprLoc().isMacroID()) + return true; + + // Recurse to children. + for (ConstStmtRange SubStmts = E->children(); SubStmts; ++SubStmts) + if (*SubStmts) + if (const Expr *SubExpr = dyn_cast(*SubStmts)) + if (HasMacroID(SubExpr)) + return true; + + return false; + } + + void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) { + if (HasMacroID(B)) + return; + + SourceRange DiagRange = B->getSourceRange(); + S.Diag(B->getExprLoc(), diag::warn_tautological_overlap_comparison) + << DiagRange << isAlwaysTrue; + } +}; + + //===----------------------------------------------------------------------===// // Check for infinite self-recursion in functions //===----------------------------------------------------------------------===// @@ -1806,6 +1838,13 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, .setAlwaysAdd(Stmt::AttributedStmtClass); } + if (Diags.getDiagnosticLevel(diag::warn_tautological_overlap_comparison, + D->getLocStart())) { + LogicalErrorHandler LEH(S); + AC.getCFGBuildOptions().Observer = &LEH; + AC.getCFG(); + AC.getCFGBuildOptions().Observer = 0; + } // Emit delayed diagnostics. if (!fscope->PossiblyUnreachableDiags.empty()) { diff --git a/test/Sema/warn-overlap.c b/test/Sema/warn-overlap.c new file mode 100644 index 0000000000..c98547153c --- /dev/null +++ b/test/Sema/warn-overlap.c @@ -0,0 +1,58 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-overlap-compare %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 {{overlapping comparisons always evaluate to true}} + if (x > 2 || x < 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x > 0 || x < 0) { } + + if (x > 2 || x <= 1) { } + if (x > 2 || x <= 2) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x > 2 || x <= 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} + + if (x >= 2 || x < 1) { } + if (x >= 2 || x < 2) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x >= 2 || x < 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} + + if (x >= 2 || x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x >= 2 || x <= 2) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x >= 2 || x <= 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x >= 0 || x <= 0) { } // expected-warning {{overlapping comparisons always evaluate to true}} + + // > && < + if (x > 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x > 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x > 2 && x < 3) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x > 0 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + + if (x > 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x > 2 && x <= 2) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x > 2 && x <= 3) { } + + if (x >= 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x >= 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x >= 2 && x < 3) { } + if (x >= 0 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}} + + if (x >= 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x >= 2 && x <= 2) { } + if (x >= 2 && x <= 3) { } + + // !=, ==, .. + if (x != 2 || x != 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x != 2 || x < 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} + if (x == 2 && x == 3) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x == 2 && x > 3) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x == 3 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (3 == x && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}} + + if (x == mydefine && x > 3) { } + if (x == (mydefine + 1) && x > 3) { } +} + diff --git a/test/SemaCXX/warn-unreachable.cpp b/test/SemaCXX/warn-unreachable.cpp index 04bd74304b..744825e226 100644 --- a/test/SemaCXX/warn-unreachable.cpp +++ b/test/SemaCXX/warn-unreachable.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 %s -fcxx-exceptions -fexceptions -fsyntax-only -verify -fblocks -std=c++11 -Wunreachable-code-aggressive -Wno-unused-value +// RUN: %clang_cc1 %s -fcxx-exceptions -fexceptions -fsyntax-only -verify -fblocks -std=c++11 -Wunreachable-code-aggressive -Wno-unused-value -Wno-tautological-compare int &halt() __attribute__((noreturn)); int &live(); @@ -327,3 +327,53 @@ void test_with_paren_silencing(int x) { calledFun(); } +void tautological_compare(bool x, int y) { + if (x > 10) // expected-note {{silence}} + calledFun(); // expected-warning {{will never be executed}} + if (10 < x) // expected-note {{silence}} + calledFun(); // expected-warning {{will never be executed}} + if (x == 10) // expected-note {{silence}} + calledFun(); // expected-warning {{will never be executed}} + + if (x < 10) // expected-note {{silence}} + calledFun(); + else + calledFun(); // expected-warning {{will never be executed}} + if (10 > x) // expected-note {{silence}} + calledFun(); + else + calledFun(); // expected-warning {{will never be executed}} + if (x != 10) // expected-note {{silence}} + calledFun(); + else + calledFun(); // expected-warning {{will never be executed}} + + if (y != 5 && y == 5) // expected-note {{silence}} + calledFun(); // expected-warning {{will never be executed}} + + if (y > 5 && y < 4) // expected-note {{silence}} + calledFun(); // expected-warning {{will never be executed}} + + if (y < 10 || y > 5) // expected-note {{silence}} + calledFun(); + else + calledFun(); // expected-warning {{will never be executed}} + + // TODO: Extend warning to the following code: + if (x < -1) + calledFun(); + if (x == -1) + calledFun(); + + if (x != -1) + calledFun(); + else + calledFun(); + if (-1 > x) + calledFun(); + else + calledFun(); + + if (y == -1 && y != -1) + calledFun(); +}