[PATCH] check for Incorrect logic in operator
Richard Trieu
rtrieu at google.com
Thu Dec 12 19:02:17 PST 2013
On Wed, Dec 11, 2013 at 3:59 AM, Anders Rönnholm <
Anders.Ronnholm at evidente.se> 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; cfe-commits at cs.uiuc.edu
> *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. However,
existing the existing warning groups that this would belong to are on by
default, which typically exclude CFG warnings. 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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131212/0c0965e9/attachment.html>
More information about the cfe-commits
mailing list