We don't get into the if-case because we never get a Loc from Sval therefore it
always returns false and our tests are passed.
Optional<Loc> L = S.getAs<Loc>();
if (!L)
return false;
Um. Right, we shouldn't get a Loc at that point. But we don't need a Loc. We
just need whatever NonLoc symbol is there, and we can check to see if that is
0. Why did we need a Loc again?
Anyway, we shouldn't have features that don't show up in the tests. I think
this code was either trying to avoid emitting duplicate messages if the
denominator is known to be 0 already (which won't happen because that's a fatal
error), or trying to avoid adding the symbol to the map if it's known not to be
0 (which is not what it's doing now, or at least not what it says it's doing).
The latter is kind of useful but it's just optimization.
Jordan
Are you suggesting that we use NonLoc instead of Loc? Can we get an Sval from
NonLoc? We needed Loc to get sval from state instead of looping the exploded
node to find the range before the division.
Let's stop for a second. We want to check if the denominator's value is equal
to 0. At this point in execution, the denominator expression is an rvalue, i.e.
the integer value has already been loaded from the variable. That means the
value of the denominator expression is going to be a NonLoc (because it
represents an integer value rather than a location). Additionally, we don't
need to load anything, because we're already past the point in analysis where
the variable is loaded.
So if I'm right, you should be able to just take the denominator SVal as is,
and feed it into the constraint manager if it's defined.
Jordan
Yes it seems like we can do that, don't know why it was a problem previously
(before we began looping the exploded node) because this was what we did.
I have made a new patch now.
//Anders
Index: lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp (revision 0)
+++ lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp (revision 0)
@@ -0,0 +1,271 @@
+//== TestAfterDivZeroChecker.cpp - Test after division by zero checker --*--==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines TestAfterDivZeroChecker, a builtin check that performs checks
+// for division by zero where the division occurs before comparison with zero.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/FoldingSet.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class ZeroState {
+private:
+ SymbolRef ZeroSymbol;
+ unsigned BlockID;
+ const StackFrameContext *SFC;
+
+public:
+ ZeroState(SymbolRef S, unsigned B, const StackFrameContext *SFC)
+ : ZeroSymbol(S), BlockID(B), SFC(SFC) {}
+
+ const StackFrameContext *getStackFrameContext() const { return SFC; }
+
+ bool operator==(const ZeroState &X) const {
+ return BlockID == X.BlockID && SFC == X.SFC && ZeroSymbol == X.ZeroSymbol;
+ }
+
+ bool operator<(const ZeroState &X) const {
+ if (BlockID != X.BlockID)
+ return BlockID < X.BlockID;
+ if (SFC != X.SFC)
+ return SFC < X.SFC;
+ return ZeroSymbol < X.ZeroSymbol;
+ }
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ ID.AddInteger(BlockID);
+ ID.AddPointer(SFC);
+ ID.AddPointer(ZeroSymbol);
+ }
+};
+
+class DivisionBRVisitor : public BugReporterVisitorImpl<DivisionBRVisitor> {
+private:
+ SymbolRef ZeroSymbol;
+ const StackFrameContext *SFC;
+ bool Satisfied = false;
+
+public:
+ DivisionBRVisitor(SymbolRef ZeroSymbol, const StackFrameContext *SFC)
+ : ZeroSymbol(ZeroSymbol), SFC(SFC) {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ ID.Add(ZeroSymbol);
+ ID.Add(SFC);
+ }
+
+ PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ,
+ const ExplodedNode *Pred,
+ BugReporterContext &BRC,
+ BugReport &BR) override;
+};
+
+class TestAfterDivZeroChecker
+ : public Checker<check::PreStmt<BinaryOperator>, check::BranchCondition,
+ check::EndFunction> {
+ mutable std::unique_ptr<BuiltinBug> DivZeroBug;
+ void reportBug(SVal Val, CheckerContext &C) const;
+
+public:
+ void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
+ void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const;
+ void checkEndFunction(CheckerContext &C) const;
+ void setDivZeroMap(SVal Var, CheckerContext &C) const;
+ bool hasDivZeroMap(SVal Var, const CheckerContext &C) const;
+ bool isZero(SVal S, CheckerContext &C) const;
+};
+} // end anonymous namespace
+
+REGISTER_SET_WITH_PROGRAMSTATE(DivZeroMap, ZeroState)
+
+PathDiagnosticPiece *DivisionBRVisitor::VisitNode(const ExplodedNode *Succ,
+ const ExplodedNode *Pred,
+ BugReporterContext &BRC,
+ BugReport &BR) {
+ if (Satisfied)
+ return nullptr;
+
+ const Expr *E = nullptr;
+
+ if (Optional<PostStmt> P = Succ->getLocationAs<PostStmt>())
+ if (const BinaryOperator *BO = P->getStmtAs<BinaryOperator>()) {
+ BinaryOperator::Opcode Op = BO->getOpcode();
+ if (Op == BO_Div || Op == BO_Rem || Op == BO_DivAssign ||
+ Op == BO_RemAssign) {
+ E = BO->getRHS();
+ }
+ }
+
+ if (!E)
+ return nullptr;
+
+ ProgramStateRef State = Succ->getState();
+ SVal S = State->getSVal(E, Succ->getLocationContext());
+ if (ZeroSymbol == S.getAsSymbol() && SFC == Succ->getStackFrame()) {
+ Satisfied = true;
+
+ // Construct a new PathDiagnosticPiece.
+ ProgramPoint P = Succ->getLocation();
+ PathDiagnosticLocation L =
+ PathDiagnosticLocation::create(P, BRC.getSourceManager());
+
+ if (!L.isValid() || !L.asLocation().isValid())
+ return nullptr;
+
+ return new PathDiagnosticEventPiece(
+ L, "Division with compared value made here");
+ }
+
+ return nullptr;
+}
+
+bool TestAfterDivZeroChecker::isZero(SVal S, CheckerContext &C) const {
+ Optional<DefinedSVal> DSV = S.getAs<DefinedSVal>();
+
+ if (!DSV)
+ return false;
+
+ ConstraintManager &CM = C.getConstraintManager();
+ ProgramStateRef stateNotZero, stateZero;
+ std::tie(stateNotZero, stateZero) = CM.assumeDual(C.getState(), *DSV);
+
+ if (!stateNotZero) {
+ assert(stateZero);
+ return true;
+ }
+ return false;
+}
+
+void TestAfterDivZeroChecker::setDivZeroMap(SVal Var, CheckerContext &C) const {
+ SymbolRef SR = Var.getAsSymbol();
+ if (!SR)
+ return;
+
+ ProgramStateRef State = C.getState();
+ State =
+ State->add<DivZeroMap>(ZeroState(SR, C.getBlockID(), C.getStackFrame()));
+ C.addTransition(State);
+}
+
+bool TestAfterDivZeroChecker::hasDivZeroMap(SVal Var,
+ const CheckerContext &C) const {
+ SymbolRef SR = Var.getAsSymbol();
+ if (!SR)
+ return false;
+
+ ZeroState ZS(SR, C.getBlockID(), C.getStackFrame());
+ return C.getState()->contains<DivZeroMap>(ZS);
+}
+
+void TestAfterDivZeroChecker::reportBug(SVal Val, CheckerContext &C) const {
+ if (ExplodedNode *N = C.generateSink(C.getState())) {
+ if (!DivZeroBug)
+ DivZeroBug.reset(new BuiltinBug(this, "Division by zero"));
+
+ BugReport *R =
+ new BugReport(*DivZeroBug, "Value being compared against zero has "
+ "already been used for division",
+ N);
+
+ R->addVisitor(new DivisionBRVisitor(Val.getAsSymbol(), C.getStackFrame()));
+ C.emitReport(R);
+ }
+}
+
+void TestAfterDivZeroChecker::checkEndFunction(CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+
+ DivZeroMapTy DivZeroes = State->get<DivZeroMap>();
+ if (DivZeroes.isEmpty())
+ return;
+
+ DivZeroMapTy::Factory &F = State->get_context<DivZeroMap>();
+ for (llvm::ImmutableSet<ZeroState>::iterator I = DivZeroes.begin(),
+ E = DivZeroes.end();
+ I != E; ++I) {
+ ZeroState ZS = *I;
+ if (ZS.getStackFrameContext() == C.getStackFrame())
+ DivZeroes = F.remove(DivZeroes, ZS);
+ }
+ C.addTransition(State->set<DivZeroMap>(DivZeroes));
+}
+
+void TestAfterDivZeroChecker::checkPreStmt(const BinaryOperator *B,
+ CheckerContext &C) const {
+ BinaryOperator::Opcode Op = B->getOpcode();
+ if (Op == BO_Div || Op == BO_Rem || Op == BO_DivAssign ||
+ Op == BO_RemAssign) {
+ SVal S = C.getSVal(B->getRHS());
+
+ if (!isZero(S, C))
+ setDivZeroMap(S, C);
+ }
+}
+
+void TestAfterDivZeroChecker::checkBranchCondition(const Stmt *Condition,
+ CheckerContext &C) const {
+ if (const BinaryOperator *B = dyn_cast<BinaryOperator>(Condition)) {
+ if (B->isComparisonOp()) {
+ const IntegerLiteral *IntLiteral = dyn_cast<IntegerLiteral>(B->getRHS());
+ bool LRHS = true;
+ if (!IntLiteral) {
+ IntLiteral = dyn_cast<IntegerLiteral>(B->getLHS());
+ LRHS = false;
+ }
+
+ if (!IntLiteral || IntLiteral->getValue() != 0)
+ return;
+
+ SVal Val = C.getSVal(LRHS ? B->getLHS() : B->getRHS());
+ if (hasDivZeroMap(Val, C))
+ reportBug(Val, C);
+ }
+ } else if (const UnaryOperator *U = dyn_cast<UnaryOperator>(Condition)) {
+ if (U->getOpcode() == UO_LNot) {
+ SVal Val;
+ if (const ImplicitCastExpr *I =
+ dyn_cast<ImplicitCastExpr>(U->getSubExpr()))
+ Val = C.getSVal(I->getSubExpr());
+
+ if (hasDivZeroMap(Val, C))
+ reportBug(Val, C);
+ else {
+ Val = C.getSVal(U->getSubExpr());
+ if (hasDivZeroMap(Val, C))
+ reportBug(Val, C);
+ }
+ }
+ } else if (const ImplicitCastExpr *IE =
+ dyn_cast<ImplicitCastExpr>(Condition)) {
+ SVal Val = C.getSVal(IE->getSubExpr());
+
+ if (hasDivZeroMap(Val, C))
+ reportBug(Val, C);
+ else {
+ SVal Val = C.getSVal(Condition);
+
+ if (hasDivZeroMap(Val, C))
+ reportBug(Val, C);
+ }
+ }
+}
+
+void ento::registerTestAfterDivZeroChecker(CheckerManager &mgr) {
+ mgr.registerChecker<TestAfterDivZeroChecker>();
+}
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt (revision 211588)
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt (working copy)
@@ -64,6 +64,7 @@
StackAddrEscapeChecker.cpp
StreamChecker.cpp
TaintTesterChecker.cpp
+ TestAfterDivZeroChecker.cpp
TraversalChecker.cpp
UndefBranchChecker.cpp
UndefCapturedBlockVarChecker.cpp
Index: lib/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- lib/StaticAnalyzer/Checkers/Checkers.td (revision 211588)
+++ lib/StaticAnalyzer/Checkers/Checkers.td (working copy)
@@ -124,6 +124,10 @@
HelpText<"Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers, and pointer to undefined variables)">,
DescFile<"CallAndMessageChecker.cpp">;
+def TestAfterDivZeroChecker : Checker<"TestAfterDivZero">,
+ HelpText<"Check for division by variable that is later compared against 0. Either the comparison is useless or there is division by zero.">,
+ DescFile<"TestAfterDivZeroChecker.cpp">;
+
} // end "alpha.core"
//===----------------------------------------------------------------------===//
Index: test/Analysis/test-after-div-zero.c
===================================================================
--- test/Analysis/test-after-div-zero.c (revision 0)
+++ test/Analysis/test-after-div-zero.c (revision 0)
@@ -0,0 +1,198 @@
+// RUN: %clang_cc1 -std=c99 -Dbool=_Bool -analyze -analyzer-checker=core,alpha.core.TestAfterDivZero -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -x c++ -analyze -analyzer-checker=core,alpha.core.TestAfterDivZero -analyzer-output=text -verify %s
+
+int var;
+
+void err_eq(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (x == 0) { } // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_eq2(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (0 == x) { } // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_ne(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (x != 0) { } // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_ge(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (x >= 0) { } // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_le(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (x <= 0) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_yes(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (x) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+void err_not(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (!x) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_pnot(int x) {
+ int *y = &x;
+ var = 77 / *y; // expected-note {{Division with compared value made here}}
+ if (!x) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_pnot2(int x) {
+ int *y = &x;
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (!*y) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_ppnot(int x) {
+ int *y = &x;
+ int **z = &y;
+ var = 77 / **z; // expected-note {{Division with compared value made here}}
+ if (!x) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void ok_other(int x, int y) {
+ var = 77 / y;
+ if (x == 0) {
+ }
+}
+
+void ok_assign(int x) {
+ var = 77 / x;
+ x = var / 77; // <- assignment => don't warn
+ if (x == 0) {
+ }
+}
+
+void ok_assign2(int x) {
+ var = 77 / x;
+ x = var / 77; // <- assignment => don't warn
+ if (0 == x) {
+ }
+}
+
+void ok_dec(int x) {
+ var = 77 / x;
+ x--; // <- assignment => don't warn
+ if (x == 0) {
+ }
+}
+
+void ok_inc(int x) {
+ var = 77 / x;
+ x++; // <- assignment => don't warn
+ if (x == 0) {
+ }
+}
+
+void do_something_ptr(int *x);
+void ok_callfunc_ptr(int x) {
+ var = 77 / x;
+ do_something_ptr(&x); // <- pass address of x to function => don't warn
+ if (x == 0) {
+ }
+}
+
+void do_something(int x);
+void nok_callfunc(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ do_something(x);
+ if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void ok_if(int x) {
+ if (x > 3)
+ var = 77 / x;
+ if (x == 0) {
+ }
+}
+
+void ok_if2(int x) {
+ if (x < 3)
+ var = 77 / x;
+ if (x == 0) {
+ } // TODO warn here
+}
+
+void ok_pif(int x) {
+ int *y = &x;
+ if (x < 3)
+ var = 77 / *y;
+ if (x == 0) {
+ } // TODO warn here
+}
+
+int getValue(bool *isPositive);
+void use(int a);
+void foo() {
+ bool isPositive;
+ int x = getValue(&isPositive);
+ if (isPositive) {
+ use(5 / x);
+ }
+
+ if (x == 0) {
+ }
+}
+
+int getValue2();
+void foo2() {
+ int x = getValue2();
+ int y = x;
+
+ use(5 / x); // expected-note {{Division with compared value made here}}
+ if (y == 0) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void ok_while(int x) {
+ int n = 100 / x;
+ while (x != 0) { // <- do not warn
+ x--;
+ }
+}
+
+void err_not2(int x, int y) {
+ int v;
+ var = 77 / x;
+
+ if (y)
+ v = 0;
+
+ if (!x) {
+ } // TODO warn here
+}
+
+inline void inline_func(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+void err_inline(int x) {
+ var = 77 / x;
+ inline_func(x); // expected-note {{Calling 'inline_func'}}
+ if (x == 0) {
+ }
+}
+
+inline void inline_func2(int x) {}
+
+void err_inline2(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ inline_func2(x);
+ if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
+inline void inline_func3(int x) {
+ var = 77 / x;
+}
+void ok_inline(int x) {
+ var = 77 / x; // expected-note {{Division with compared value made here}}
+ inline_func3(x);
+ if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}}
+} // expected-note@-1 {{Value being compared against zero has already been used for division}}
+
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (revision 211588)
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (working copy)
@@ -174,8 +172,13 @@
return Pred->getLocationContext()->getAnalysisDeclContext();
}
- /// \brief If the given node corresponds to a PostStore program point, retrieve
- /// the location region as it was uttered in the code.
+ /// \brief Get the blockID.
+ unsigned getBlockID() const {
+ return NB.getContext().getBlock()->getBlockID();
+ }
+
+ /// \brief If the given node corresponds to a PostStore program point,
+ /// retrieve the location region as it was uttered in the code.
///
/// This utility can be useful for generating extensive diagnostics, for
/// example, for finding variables that the given symbol was assigned to.
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
