Hi,
I have a new patch I like to get reviewed. It checks for incorrect logics in
relational,equal and logic operators in cfgbuilder when trying to evaluate
bool. A warning is emitted in Sema when an error occurs.
eg.
if (x != 2 || x != 3) { } always true
if ((x < y) <= 10) {} always true
Thanks,
Anders
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 %s -fsyntax-only -Xclang -verify -fblocks -Wunreachable-code
+
+#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: 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 %s -fsyntax-only -Xclang -verify -fblocks -Wunreachable-code
+
+#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) { }
+ 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) { } // False negative. There should be a warning here. This is always false.
+
+ 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 < 1) { } // expected-warning {{logical disjunction always evaluates to false}}
+ if (x >= 2 && x < 2) { }
+ 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) { }
+ 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: lib/Analysis/CFG.cpp
===================================================================
--- lib/Analysis/CFG.cpp (revision 192595)
+++ lib/Analysis/CFG.cpp (working copy)
@@ -483,6 +483,207 @@
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) {
+ if (!B->isRelationalOp())
+ return TryResult();
+
+ const IntegerLiteral *IntLiteral =
+ dyn_cast<IntegerLiteral>(B->getLHS()->IgnoreParens());
+ const Expr *BoolExpr = B->getRHS()->IgnoreParens();
+ bool IntFirst = true;
+ if (!IntLiteral) {
+ IntLiteral = dyn_cast<IntegerLiteral>(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)) {
+ CFGObserver::notifyCompareBoolWithInt(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) {
+ if (!B->isEqualityOp())
+ return TryResult();
+
+ const IntegerLiteral *IntLiteral =
+ dyn_cast<IntegerLiteral>(B->getLHS()->IgnoreParens());
+ const Expr *BoolExpr = B->getRHS()->IgnoreParens();
+
+ if (!IntLiteral) {
+ IntLiteral = dyn_cast<IntegerLiteral>(B->getRHS()->IgnoreParens());
+ BoolExpr = B->getLHS()->IgnoreParens();
+ }
+
+ if (!IntLiteral || !BoolExpr->isKnownToHaveBooleanValue())
+ return TryResult();
+
+ llvm::APInt IntValue = IntLiteral->getValue();
+ if ((IntValue != 1) && (IntValue != 0)) {
+ CFGObserver::notifyCompareBoolWithInt(B);
+ return TryResult(B->getOpcode() != BO_EQ);
+ }
+ return TryResult();
+ }
+
+ enum LogicRelation { Equal, NotEqual, Less, LessEqual, More, MoreEqual };
+
+ bool analyzeLogicOperatorCondition(const IntegerLiteral *FirstConstant,
+ const IntegerLiteral *SecondConstant,
+ LogicRelation Relation) {
+ llvm::APSInt f1,f2;
+ if (FirstConstant->EvaluateAsInt(f1,*Context) &&
+ SecondConstant->EvaluateAsInt(f2,*Context))
+ return (Relation == Equal && (f1 == f2)) ||
+ (Relation == NotEqual && (f1 != f2)) ||
+ (Relation == Less && (f1 < f2)) ||
+ (Relation == LessEqual && (f1 <= f2)) ||
+ (Relation == More && (f1 > f2)) ||
+ (Relation == MoreEqual && (f1 >= f2));
+ else
+ return false;
+ }
+
+ /// \brief Find a pair of comparison expressions with or without parentheses
+ /// with a shared variable and constants and a logical operator between them.
+ /// e.g. if (x != 3 || x != 4)
+ TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
+ if (!B->isLogicalOp())
+ return TryResult();
+
+ const BinaryOperator *LHS =
+ dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
+ const BinaryOperator *RHS =
+ dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
+ if (!LHS || !RHS)
+ return TryResult();
+
+ if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
+ return TryResult();
+
+ BinaryOperatorKind BO1 = LHS->getOpcode();
+ const DeclRefExpr *Decl1 =
+ dyn_cast<DeclRefExpr>(LHS->getLHS()->IgnoreImpCasts()->IgnoreParens());
+ const IntegerLiteral *Literal1 =
+ dyn_cast<IntegerLiteral>(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<DeclRefExpr>(LHS->getRHS()->IgnoreImpCasts()->IgnoreParens());
+ Literal1 = dyn_cast<IntegerLiteral>(LHS->getLHS()->IgnoreParens());
+ }
+
+ if (!Decl1 || !Literal1)
+ return TryResult();
+
+ BinaryOperatorKind BO2 = RHS->getOpcode();
+ const DeclRefExpr *Decl2 =
+ dyn_cast<DeclRefExpr>(RHS->getLHS()->IgnoreImpCasts()->IgnoreParens());
+ const IntegerLiteral *Literal2 =
+ dyn_cast<IntegerLiteral>(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<DeclRefExpr>(RHS->getRHS()->IgnoreImpCasts()->IgnoreParens());
+ Literal2 = dyn_cast<IntegerLiteral>(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();
+
+ static const struct LinkedConditions {
+ BinaryOperatorKind C1;
+ BinaryOperatorKind Bop;
+ BinaryOperatorKind C2;
+ LogicRelation Relation;
+ bool Result;
+ } Conditions[] = {
+ { BO_NE, BO_LOr, BO_NE, NotEqual, true },
+ { BO_EQ, BO_LAnd, BO_EQ, NotEqual, false },
+ { BO_GT, BO_LOr, BO_LT, Less, true },
+ { BO_GE, BO_LOr, BO_LT, LessEqual, true },
+ { BO_GE, BO_LOr, BO_LE, LessEqual, true },
+ { BO_GT, BO_LOr, BO_LE, LessEqual, true },
+ { BO_LT, BO_LAnd, BO_GT, LessEqual, false },
+ { BO_LE, BO_LAnd, BO_GT, Less, false },
+ { BO_LE, BO_LAnd, BO_GE, Less, false },
+ { BO_LT, BO_LAnd, BO_GE, Less, false },
+ { BO_GT, BO_LAnd, BO_EQ, MoreEqual, false },
+ { BO_LT, BO_LAnd, BO_EQ, LessEqual, false },
+ { BO_GE, BO_LAnd, BO_EQ, More, false },
+ { BO_LE, BO_LAnd, BO_EQ, Less, false },
+ { BO_NE, BO_LAnd, BO_EQ, Equal, false },
+ { BO_NE, BO_LOr, BO_EQ, Equal, true }
+ };
+ BinaryOperatorKind Bok = B->getOpcode();
+ for (unsigned int i = 0; i < (sizeof(Conditions) / sizeof(Conditions[0]));
+ i++) {
+ if (Bok != Conditions[i].Bop)
+ continue;
+
+ bool Error = false;
+ if (BO1 == Conditions[i].C1 && BO2 == Conditions[i].C2) {
+ Error = analyzeLogicOperatorCondition( Literal1,
+ Literal2,
+ Conditions[i].Relation);
+ } else if (BO1 == Conditions[i].C2 && BO2 == Conditions[i].C1) {
+ Error = analyzeLogicOperatorCondition( Literal2,
+ Literal1,
+ Conditions[i].Relation);
+ } else {
+ // Continue looking..
+ continue;
+ }
+
+ if (Error) {
+ CFGObserver::notifyCompareAlwaysTrue(B,Conditions[i].Result);
+ return TryResult(Conditions[i].Result);
+ }
+
+ return TryResult();
+ }
+ return TryResult();
+ }
+
/// Try and evaluate an expression to an integer constant.
bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) {
if (!BuildOpts.PruneTriviallyFalseEdges)
@@ -565,13 +766,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 +1523,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);
}
@@ -3974,7 +4188,35 @@
}
}
+std::vector<CFGCallback*> CFGObserver::Observers;
+void CFGObserver::attach(CFGCallback *Observer) {
+ Observers.push_back(Observer);
+}
+void CFGObserver::detach(CFGCallback *Observer) {
+ Observers.erase(std::remove(Observers.begin(), Observers.end(), Observer),
+ Observers.end());
+}
+
+void CFGObserver::notifyCompareAlwaysTrue(const BinaryOperator* B,
+ bool IsAlwaysTrue) {
+ for(std::vector<CFGCallback*>::const_iterator I = Observers.begin();
+ I != Observers.end(); ++I) {
+ if(*I != 0) {
+ (*I)->compareAlwaysTrue(B,IsAlwaysTrue);
+ }
+ }
+}
+
+void CFGObserver::notifyCompareBoolWithInt(const BinaryOperator* B) {
+ for(std::vector<CFGCallback*>::const_iterator I = Observers.begin();
+ I != Observers.end(); ++I) {
+ if(*I != 0) {
+ (*I)->compareBoolWithInt(B);
+ }
+ }
+}
+
/// dump - A simple pretty printer of a CFG that outputs to stderr.
void CFG::dump(const LangOptions &LO, bool ShowColors) const {
print(llvm::errs(), LO, ShowColors);
Index: lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- lib/Sema/AnalysisBasedWarnings.cpp (revision 192595)
+++ 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<BinaryOperator>(B->getLHS()->IgnoreParens());
+ const BinaryOperator *RHS =
+ dyn_cast<BinaryOperator>(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 ? "true" : "false");
+ }
+
+ void compareBoolWithInt(const BinaryOperator* B) {
+ if (B->getLHS()->getExprLoc().isMacroID() ||
+ B->getRHS()->getExprLoc().isMacroID())
+ return;
+
+ const BinaryOperator *LHS =
+ dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
+ const BinaryOperator *RHS =
+ dyn_cast<BinaryOperator>(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.
//===----------------------------------------------------------------------===//
@@ -1702,8 +1756,12 @@
bool isTemplateInstantiation = false;
if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D))
isTemplateInstantiation = Function->isTemplateInstantiation();
- if (!isTemplateInstantiation)
- CheckUnreachable(S, AC);
+ if (!isTemplateInstantiation) {
+ LogicalErrorsHandler Leh(S);
+ CFGObserver::attach(&Leh);
+ CheckUnreachable(S, AC);
+ CFGObserver::detach(&Leh);
+ }
}
// Check for thread safety violations
Index: lib/AST/Expr.cpp
===================================================================
--- lib/AST/Expr.cpp (revision 192595)
+++ lib/AST/Expr.cpp (working copy)
@@ -151,6 +151,8 @@
switch (UO->getOpcode()) {
case UO_Plus:
return UO->getSubExpr()->isKnownToHaveBooleanValue();
+ case UO_LNot:
+ return true;
default:
return false;
}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 192595)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -4544,6 +4544,12 @@
def warn_lunsigned_always_true_comparison : Warning<
"comparison of unsigned%select{| enum}2 expression %0 is always %1">,
InGroup<TautologicalCompare>;
+def warn_operator_always_true_comparison : Warning<
+ "logical disjunction always evaluates to %0">,
+ InGroup<TautologicalCompare>;
+def warn_bool_and_int_comparison : Warning<
+ "comparison of a boolean expression with an integer other than 0 or 1">,
+ InGroup<TautologicalCompare>;
def warn_out_of_range_compare : Warning<
"comparison of constant %0 with expression of type %1 is always "
"%select{false|true}2">, InGroup<TautologicalOutOfRangeCompare>;
Index: include/clang/Analysis/CFG.h
===================================================================
--- include/clang/Analysis/CFG.h (revision 192595)
+++ 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,29 @@
}
};
+/// \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(){}
+};
+
+/// \brief Keeps track of all observers. Provides functionality to add and
+/// remove observers and notify observers on found logical operator errors.
+class CFGObserver {
+public:
+ static void attach(CFGCallback *Observer);
+ static void detach(CFGCallback *Observer);
+ static void notifyCompareAlwaysTrue(const BinaryOperator* B,
+ bool IsAlwaysTrue);
+ static void notifyCompareBoolWithInt(const BinaryOperator* B);
+private:
+ static std::vector<CFGCallback*> Observers;
+};
+
/// 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
