From: Zhongxing Xu Date: Wed, 2 Sep 2009 13:26:26 +0000 (+0000) Subject: Refactor the check for bad divide into a checker. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6403b57eda05a22273d920ad0bd2991d11eaa7b8;p=clang Refactor the check for bad divide into a checker. Also fix a checker context bug: the Dst set is not always empty initially. Because in GRExprEngine::CheckerVisit(), *CurrSet is used repeatedly. So we removed the Dst.empty() condition in ~CheckerContext() when deciding whether to do autotransision. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@80786 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/PathSensitive/Checker.h b/include/clang/Analysis/PathSensitive/Checker.h index 77373699a2..3dc484569d 100644 --- a/include/clang/Analysis/PathSensitive/Checker.h +++ b/include/clang/Analysis/PathSensitive/Checker.h @@ -49,14 +49,15 @@ public: : Dst(dst), B(builder), Eng(eng), Pred(pred), OldSink(B.BuildSinks), OldTag(B.Tag), OldPointKind(B.PointKind), OldHasGen(B.HasGeneratedNode) { - assert(Dst.empty()); + //assert(Dst.empty()); // This is a fake assertion. + // See GRExprEngine::CheckerVisit(), CurrSet is repeatedly used. B.Tag = tag; if (preVisit) B.PointKind = ProgramPoint::PreStmtKind; } ~CheckerContext() { - if (!B.BuildSinks && Dst.empty() && !B.HasGeneratedNode) + if (!B.BuildSinks && !B.HasGeneratedNode) Dst.Add(Pred); } @@ -71,8 +72,12 @@ public: ASTContext &getASTContext() { return Eng.getContext(); } + + ExplodedNode *GenerateNode(const Stmt *S, bool markAsSink = false) { + return GenerateNode(S, getState(), markAsSink); + } - ExplodedNode *generateNode(const Stmt* S, const GRState *state, + ExplodedNode *GenerateNode(const Stmt* S, const GRState *state, bool markAsSink = false) { ExplodedNode *node = B.generateNode(S, state, Pred); diff --git a/include/clang/Analysis/PathSensitive/CheckerVisitor.def b/include/clang/Analysis/PathSensitive/CheckerVisitor.def index b3d09d0f91..ff6528dae8 100644 --- a/include/clang/Analysis/PathSensitive/CheckerVisitor.def +++ b/include/clang/Analysis/PathSensitive/CheckerVisitor.def @@ -13,5 +13,6 @@ PREVISIT(CallExpr) PREVISIT(ObjCMessageExpr) +PREVISIT(BinaryOperator) #undef PREVISIT diff --git a/include/clang/Analysis/PathSensitive/CheckerVisitor.h b/include/clang/Analysis/PathSensitive/CheckerVisitor.h index 35afe73f4d..6ec192adee 100644 --- a/include/clang/Analysis/PathSensitive/CheckerVisitor.h +++ b/include/clang/Analysis/PathSensitive/CheckerVisitor.h @@ -36,6 +36,10 @@ public: default: assert(false && "Unsupport statement."); return; + case Stmt::CompoundAssignOperatorClass: + static_cast(this)->PreVisitBinaryOperator(C, + static_cast(S)); + break; #define PREVISIT(NAME) \ case Stmt::NAME ## Class:\ static_cast(this)->PreVisit ## NAME(C,static_cast(S));\ diff --git a/lib/Analysis/BugReporterVisitors.cpp b/lib/Analysis/BugReporterVisitors.cpp index fce31e70f0..8b3502805d 100644 --- a/lib/Analysis/BugReporterVisitors.cpp +++ b/lib/Analysis/BugReporterVisitors.cpp @@ -55,7 +55,7 @@ clang::bugreporter::GetReceiverExpr(const ExplodedNode *N){ const Stmt* clang::bugreporter::GetDenomExpr(const ExplodedNode *N) { - const Stmt *S = N->getLocationAs()->getStmt(); + const Stmt *S = N->getLocationAs()->getStmt(); if (const BinaryOperator *BE = dyn_cast(S)) return BE->getRHS(); return NULL; diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp index a6b580b865..a36916ba2e 100644 --- a/lib/Analysis/GRExprEngine.cpp +++ b/lib/Analysis/GRExprEngine.cpp @@ -2700,44 +2700,6 @@ void GRExprEngine::VisitReturnStmt(ReturnStmt* S, ExplodedNode* Pred, ExplodedNo // Transfer functions: Binary operators. //===----------------------------------------------------------------------===// -const GRState* GRExprEngine::CheckDivideZero(Expr* Ex, const GRState* state, - ExplodedNode* Pred, SVal Denom) { - - // Divide by undefined? (potentially zero) - - if (Denom.isUndef()) { - ExplodedNode* DivUndef = Builder->generateNode(Ex, state, Pred); - - if (DivUndef) { - DivUndef->markAsSink(); - ExplicitBadDivides.insert(DivUndef); - } - - return 0; - } - - // Check for divide/remainder-by-zero. - // First, "assume" that the denominator is 0 or undefined. - const GRState* zeroState = state->assume(Denom, false); - - // Second, "assume" that the denominator cannot be 0. - state = state->assume(Denom, true); - - // Create the node for the divide-by-zero (if it occurred). - if (zeroState) - if (ExplodedNode* DivZeroNode = Builder->generateNode(Ex, zeroState, Pred)) { - DivZeroNode->markAsSink(); - - if (state) - ImplicitBadDivides.insert(DivZeroNode); - else - ExplicitBadDivides.insert(DivZeroNode); - - } - - return state; -} - void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, ExplodedNode* Pred, ExplodedNodeSet& Dst) { @@ -2765,10 +2727,14 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, ExplodedNodeSet Tmp2; Visit(RHS, *I1, Tmp2); + + ExplodedNodeSet CheckedSet; + CheckerVisit(B, CheckedSet, Tmp2, true); // With both the LHS and RHS evaluated, process the operation itself. - for (ExplodedNodeSet::iterator I2=Tmp2.begin(), E2=Tmp2.end(); I2 != E2; ++I2) { + for (ExplodedNodeSet::iterator I2=CheckedSet.begin(), E2=CheckedSet.end(); + I2 != E2; ++I2) { const GRState* state = GetState(*I2); const GRState* OldSt = state; @@ -2799,17 +2765,6 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, continue; } - case BinaryOperator::Div: - case BinaryOperator::Rem: - - // Special checking for integer denominators. - if (RHS->getType()->isIntegerType() && - RHS->getType()->isScalarType()) { - - state = CheckDivideZero(B, state, *I2, RightV); - if (!state) continue; - } - // FALL-THROUGH. default: { @@ -2875,24 +2830,12 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, SVal location = state->getSVal(LHS); EvalLoad(Tmp3, LHS, *I2, state, location); - for (ExplodedNodeSet::iterator I3=Tmp3.begin(), E3=Tmp3.end(); I3!=E3; ++I3) { + for (ExplodedNodeSet::iterator I3=Tmp3.begin(), E3=Tmp3.end(); I3!=E3; + ++I3) { state = GetState(*I3); SVal V = state->getSVal(LHS); - // Check for divide-by-zero. - if ((Op == BinaryOperator::Div || Op == BinaryOperator::Rem) - && RHS->getType()->isIntegerType() - && RHS->getType()->isScalarType()) { - - // CheckDivideZero returns a new state where the denominator - // is assumed to be non-zero. - state = CheckDivideZero(B, state, *I3, RightV); - - if (!state) - continue; - } - // Propagate undefined values (left-side). if (V.isUndef()) { EvalStore(Dst, B, LHS, *I3, state->BindExpr(B, V), diff --git a/lib/Analysis/GRExprEngineInternalChecks.cpp b/lib/Analysis/GRExprEngineInternalChecks.cpp index 4c6371c44c..28e763376a 100644 --- a/lib/Analysis/GRExprEngineInternalChecks.cpp +++ b/lib/Analysis/GRExprEngineInternalChecks.cpp @@ -193,14 +193,10 @@ public: class VISIBILITY_HIDDEN DivZero : public BuiltinBug { public: - DivZero(GRExprEngine* eng) + DivZero(GRExprEngine* eng = 0) : BuiltinBug(eng,"Division-by-zero", "Division by zero or undefined value.") {} - void FlushReportsImpl(BugReporter& BR, GRExprEngine& Eng) { - Emit(BR, Eng.explicit_bad_divides_begin(), Eng.explicit_bad_divides_end()); - } - void registerInitialVisitors(BugReporterContext& BRC, const ExplodedNode* N, BuiltinBugReport *R) { @@ -575,7 +571,7 @@ public: if (stateNull && !stateNotNull) { // Generate an error node. Check for a null node in case // we cache out. - if (ExplodedNode *errorNode = C.generateNode(CE, stateNull, true)) { + if (ExplodedNode *errorNode = C.GenerateNode(CE, stateNull, true)) { // Lazily allocate the BugType object if it hasn't already been // created. Ownership is transferred to the BugReporter object once @@ -611,7 +607,7 @@ public: // If we reach here all of the arguments passed the nonnull check. // If 'state' has been updated generated a new node. if (state != originalState) - C.addTransition(C.generateNode(CE, state)); + C.addTransition(C.GenerateNode(CE, state)); } }; } // end anonymous namespace @@ -639,7 +635,7 @@ void CheckUndefinedArg::PreVisitCallExpr(CheckerContext &C, const CallExpr *CE){ for (CallExpr::const_arg_iterator I = CE->arg_begin(), E = CE->arg_end(); I != E; ++I) { if (C.getState()->getSVal(*I).isUndef()) { - if (ExplodedNode *ErrorNode = C.generateNode(CE, C.getState(), true)) { + if (ExplodedNode *ErrorNode = C.GenerateNode(CE, true)) { if (!BT) BT = new BadArg(); // Generate a report for this bug. @@ -672,7 +668,7 @@ void CheckBadCall::PreVisitCallExpr(CheckerContext &C, const CallExpr *CE) { SVal L = C.getState()->getSVal(Callee); if (L.isUndef() || isa(L)) { - if (ExplodedNode *N = C.generateNode(CE, C.getState(), true)) { + if (ExplodedNode *N = C.GenerateNode(CE, true)) { if (!BT) BT = new BadCall(); C.EmitReport(new BuiltinBugReport(*BT, BT->getDescription().c_str(), N)); @@ -680,6 +676,68 @@ void CheckBadCall::PreVisitCallExpr(CheckerContext &C, const CallExpr *CE) { } } +class VISIBILITY_HIDDEN CheckBadDiv : public CheckerVisitor { + DivZero *BT; +public: + CheckBadDiv() : BT(0) {} + ~CheckBadDiv() {} + + const void *getTag() { + static int x; + return &x; + } + + void PreVisitBinaryOperator(CheckerContext &C, const BinaryOperator *B); +}; + +void CheckBadDiv::PreVisitBinaryOperator(CheckerContext &C, + const BinaryOperator *B) { + BinaryOperator::Opcode Op = B->getOpcode(); + if (Op != BinaryOperator::Div && + Op != BinaryOperator::Rem && + Op != BinaryOperator::DivAssign && + Op != BinaryOperator::RemAssign) + return; + + if (!B->getRHS()->getType()->isIntegerType() || + !B->getRHS()->getType()->isScalarType()) + return; + + printf("should not check!\n"); + + // Check for divide by undefined. + SVal Denom = C.getState()->getSVal(B->getRHS()); + + if (Denom.isUndef()) { + if (ExplodedNode *N = C.GenerateNode(B, true)) { + if (!BT) + BT = new DivZero(); + + C.EmitReport(new BuiltinBugReport(*BT, BT->getDescription().c_str(), N)); + } + return; + } + + // Check for divide by zero. + ConstraintManager &CM = C.getConstraintManager(); + const GRState *stateNotZero, *stateZero; + llvm::tie(stateNotZero, stateZero) = CM.AssumeDual(C.getState(), + cast(Denom)); + + if (stateZero && !stateNotZero) { + if (ExplodedNode *N = C.GenerateNode(B, stateZero, true)) { + if (!BT) + BT = new DivZero(); + + C.EmitReport(new BuiltinBugReport(*BT, BT->getDescription().c_str(), N)); + } + return; + } + + // If we get here, then the denom should not be zero. + if (stateNotZero != C.getState()) + C.addTransition(C.GenerateNode(B, stateNotZero)); +} } //===----------------------------------------------------------------------===// // Check registration. @@ -694,7 +752,6 @@ void GRExprEngine::RegisterInternalChecks() { BR.Register(new NullDeref(this)); BR.Register(new UndefinedDeref(this)); BR.Register(new UndefBranch(this)); - BR.Register(new DivZero(this)); BR.Register(new UndefResult(this)); BR.Register(new RetStack(this)); BR.Register(new RetUndef(this)); @@ -713,4 +770,5 @@ void GRExprEngine::RegisterInternalChecks() { registerCheck(new CheckAttrNonNull()); registerCheck(new CheckUndefinedArg()); registerCheck(new CheckBadCall()); + registerCheck(new CheckBadDiv()); }