Tanks for your comments. New patch attached.
-- We can have some warnings in a group on by default and some off by default,
but this does suggest that we'd want a new warning subgroup for this warning.
Do you want a new subgroup and how do you create one? I have moved it back to
tautological and made a what i think is a subgroup.
-- Instead of looking for an integer literal, see if the expression can be
evaluated, and use that as the constant value. This will allow things like 1+2
and -1.
I will skip this because it will also generate false positives when it's x+1
and can be evaluated.
--Also, does your patch cause the unreachable-code warning to trigger in more
cases now?
I have tested it on llvm and the patch did not trigger more unreachable-code
warnings.
-- What is going on here?
Comments in code, also slight refactoring
Basicly it checks whether the expression is always true/false by checking
following scenarios.
x is less than the smallest literal.
x is equal to the smallest literal.
x is between smallest and largest literal.
x is equal to the largest literal.
x is greater than largest literal.
For && both RHS and LHS needs to be true or false in every scenario for it to
be regarded always true/false.
For || either RHS and LHS needs to be true or false in every scenario for it to
be regarded always true/false.
-- What's the plan with CFGCallBack? New methods added as needed for each new
warning? Does the CFGBuilder need to support multiple CFGCallBack's at once?
Yes new methods would have to be added. No i don't see a reason for the need of
supporting multiple CFGCallBack's at once.
//Anders
.......................................................................................................................
Anders Rönnholm Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
Mobile: +46 (0)70 912 42 54
E-mail: [email protected]
www.evidente.se
________________________________________
Från: [email protected] [[email protected]] för Richard Smith
[[email protected]]
Skickat: den 13 december 2013 19:18
Till: Richard Trieu
Cc: Anders Rönnholm; [email protected]
Ämne: Re: [PATCH] check for Incorrect logic in operator
On Thu, Dec 12, 2013 at 7:02 PM, Richard Trieu
<[email protected]<mailto:[email protected]>> wrote:
On Wed, Dec 11, 2013 at 3:59 AM, Anders Rönnholm
<[email protected]<mailto:[email protected]>> wrote:
Hi,
I haven’t received any comments on this patch. Is anyone reviewing it?
//Anders
From: Anders Rönnholm
Sent: den 21 november 2013 08:52
To: 'Anna Zaks'; Richard Trieu
Cc: Jordan Rose; Ted Kremenek;
[email protected]<mailto:[email protected]>
Subject: RE: [PATCH] check for Incorrect logic in operator
Hi,
Here is an updated patch I’d like to get reviewed. The warnings have been moved
to the unreachable-code group, I hope that’s fine.
Thanks,
Anders
I don't like the warnings in the unreachable-code group, since in the always
true case, it is the opposite of unreachable code.
The tautological comparison group makes more sense for this warning.
However, existing the existing warning groups that this would belong to are on
by default, which typically exclude CFG warnings.
We can have some warnings in a group on by default and some off by default, but
this does suggest that we'd want a new warning subgroup for this warning.
We may need a temporary solution until we merge the old and new warnings.
Also, does your patch cause the unreachable-code warning to trigger in more
cases now?
More comments inline.
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<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)) {
+ BuildOpts.Observer->compareBoolWithInt(B);
Check that BuildOpts.Observer is not null before dereferencing.
+ 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<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)) {
+ BuildOpts.Observer->compareBoolWithInt(B);
+ return TryResult(B->getOpcode() != BO_EQ);
+ }
+ return TryResult();
+ }
+
+ bool analyzeLogicOperatorCondition(BinaryOperatorKind Relation,
+ const llvm::APSInt Value1,
+ const llvm::APSInt Value2) {
A switch statement would be nicer here. Also, add an assert that the two
values have the same signedness and bit width.
+ 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<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();
+
Instead of looking for an integer literal, see if the expression can be
evaluated, and use that as the constant value. This will allow things like 1+2
and -1.
+ BinaryOperatorKind BO1 = LHS->getOpcode();
+ const DeclRefExpr *Decl1 =
+ dyn_cast<DeclRefExpr>(LHS->getLHS()->IgnoreParenImpCasts());
+ 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()->IgnoreParenImpCasts());
+ 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());
->IgnoreParenImpCasts() instead of ->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())
Decl1->getDecl() != Decl2->getDecl()
+ 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;
What is going on here?
+ 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<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;
+ }
+
+ 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.
//===----------------------------------------------------------------------===//
@@ -1723,8 +1777,11 @@
bool isTemplateInstantiation = false;
if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(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)
Add some tests with different variable types and different constant types.
@@ -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<MissingNoreturn>, DefaultIgnore;
def warn_unreachable : Warning<"will never be executed">,
InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;
Still don't like the logical disjunction text. I think it would be confusing
for users.
+def warn_operator_always_true_comparison : Warning<
+ "logical disjunction always evaluates to %select{false|true}0">,
+ InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;
+def warn_bool_and_int_comparison : Warning<
+ "comparison of a boolean expression with an integer other than 0 or 1">,
+ InGroup<DiagGroup<"unreachable-code">>, 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 @@
}
};
What's the plan with CFGCallBack? New methods added as needed for each new
warning? Does the CFGBuilder need to support multiple CFGCallBack's at once?
+/// \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<const Stmt *, const CFGBlock*> 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)
_______________________________________________
cfe-commits mailing list
[email protected]<mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Index: lib/Analysis/CFG.cpp
===================================================================
--- lib/Analysis/CFG.cpp (revision 197224)
+++ lib/Analysis/CFG.cpp (working copy)
@@ -483,6 +483,221 @@
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<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)) {
+ if(BuildOpts.Observer)
+ 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<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)) {
+ BuildOpts.Observer->compareBoolWithInt(B);
+ return TryResult(B->getOpcode() != BO_EQ);
+ }
+ return TryResult();
+ }
+
+ 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) {
+ 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()->IgnoreParenImpCasts());
+ 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()->IgnoreParenImpCasts());
+ Literal1 =
+ dyn_cast<IntegerLiteral>(LHS->getLHS()->IgnoreParens());
+ }
+
+ if (!Decl1 || !Literal1)
+ return TryResult();
+
+ BinaryOperatorKind BO2 = RHS->getOpcode();
+ const DeclRefExpr *Decl2 =
+ dyn_cast<DeclRefExpr>(RHS->getLHS()->IgnoreParenImpCasts());
+ 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()->IgnoreParenImpCasts());
+ Literal2 =
+ dyn_cast<IntegerLiteral>(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;
+ Literal1->EvaluateAsInt(L1,*Context);
+ Literal2->EvaluateAsInt(L2,*Context);
+
+ if (!L1 || !L2)
+ return TryResult();
+
+ // Can't compare signed with unsigned or with different bit width.
+ if (L1.isSigned() != L2.isSigned() ||
+ L1.getBitWidth() != L2.getBitWidth())
+ return TryResult();
+
+ const llvm::APSInt MinValue =
+ llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned());
+ const llvm::APSInt MaxValue =
+ llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned());
+
+ llvm::APSInt I1 = llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1),
+ L1.isUnsigned());
+
+ // 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
+ MinValue,
+ // L1
+ L1,
+ // Value between Value1 and Value2
+ ((L1 < L2) ? L1 : L2) + I1,
+ // L2
+ L2,
+ // Value greater than both Value1 and Value2
+ MaxValue,
+ };
+
+ // 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)
@@ -565,13 +780,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 +1537,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 197224)
+++ 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;
+ }
+
+ 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.
//===----------------------------------------------------------------------===//
@@ -1723,8 +1777,11 @@
bool isTemplateInstantiation = false;
if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(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 197224)
+++ lib/AST/Expr.cpp (working copy)
@@ -121,6 +121,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 197224)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -6263,6 +6263,12 @@
def warn_comparison_always : Warning<
"%select{self-|array }0comparison always evaluates to %select{false|true|a constant}1">,
InGroup<TautologicalCompare>;
+def warn_operator_always_true_comparison : Warning<
+ "overlapping comparisons always evaluate to %select{false|true}0">,
+ InGroup<TautologicalCompareOperator>;
+def warn_bool_and_int_comparison : Warning<
+ "comparison of a boolean expression with an integer other than 0 or 1">,
+ InGroup<TautologicalCompareOperator>;
def warn_stringcompare : Warning<
"result of comparison against %select{a string literal|@encode}0 is "
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td (revision 197224)
+++ include/clang/Basic/DiagnosticGroups.td (working copy)
@@ -282,6 +282,8 @@
def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
def TautologicalCompare : DiagGroup<"tautological-compare",
[TautologicalOutOfRangeCompare]>;
+def TautologicalCompareOperator : DiagGroup<"tautological-compare-operator",
+ [TautologicalCompare]>;
def HeaderHygiene : DiagGroup<"header-hygiene">;
def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
Index: include/clang/Analysis/CFG.h
===================================================================
--- include/clang/Analysis/CFG.h (revision 197224)
+++ 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<const Stmt *, const CFGBlock*> 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)
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 {{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) { }
+ 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 > 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 > 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 >= 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 == 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
