Excellent point. Can you take a look at the patch I just committed in r89882 to hopefully address this issue?
On Nov 25, 2009, at 7:15 AM, Zhongxing Xu wrote: > Hi Ted, > > I'm afraid this 'early stop evaluating' might not be correct. Since > checkers might bifurcate states, we may end up with some nodes being > done evaluation and some not. But all of them are not evaluated any > more. > > 2009/11/25 Ted Kremenek <[email protected]>: >> Author: kremenek >> Date: Tue Nov 24 15:41:28 2009 >> New Revision: 89804 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=89804&view=rev >> Log: >> Cleanups and fixes to the nil-receiver checker, some of it fallout the >> initial transition of the nil-receiver checker to the Checker >> interface as done in r89745. Some important changes include: >> >> 1) We consolidate the BugType object used for nil receiver bug >> reports, and don't include the type of the returned value in the >> BugType (which would be wrong if a nil receiver bug was reported more >> than once) >> >> 2) Added a new (temporary) flag to CheckerContext: DoneEvauating. >> This is used by GRExprEngine when evaluating message expressions to >> not continue evaluating the message expression if this flag is set. >> This flag is currently set by the nil receiver checker. This is an >> intermediate solution to allow the nil-receiver checker to properly >> work as a plug-in outside of GRExprEngine. Basically, this flag >> indicates that the entire message expression has been evaluated, not >> just a precondition (which is what the nil-receiver checker does). >> This flag *should not* be repurposed for general use, but just to pull >> more things out of GRExprEngine that already in there as we devise a >> better interface in the Checker class. >> >> 3) Cleaned up the logic in the nil-receiver checker, making the >> control-flow a lot easier to read. >> >> Modified: >> cfe/trunk/include/clang/Analysis/PathSensitive/Checker.h >> cfe/trunk/include/clang/Analysis/PathSensitive/ExplodedGraph.h >> cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h >> cfe/trunk/lib/Analysis/CallAndMessageChecker.cpp >> cfe/trunk/lib/Analysis/GRExprEngine.cpp >> cfe/trunk/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m >> cfe/trunk/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m >> >> Modified: cfe/trunk/include/clang/Analysis/PathSensitive/Checker.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/Checker.h?rev=89804&r1=89803&r2=89804&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/PathSensitive/Checker.h (original) >> +++ cfe/trunk/include/clang/Analysis/PathSensitive/Checker.h Tue Nov 24 >> 15:41:28 2009 >> @@ -42,6 +42,7 @@ >> const GRState *state; >> const Stmt *statement; >> const unsigned size; >> + bool DoneEvaluating; // FIXME: This is not a permanent API change. >> public: >> CheckerContext(ExplodedNodeSet &dst, GRStmtNodeBuilder &builder, >> GRExprEngine &eng, ExplodedNode *pred, >> @@ -52,10 +53,22 @@ >> OldTag(B.Tag, tag), >> OldPointKind(B.PointKind, K), >> OldHasGen(B.HasGeneratedNode), >> - state(st), statement(stmt), size(Dst.size()) {} >> + state(st), statement(stmt), size(Dst.size()), >> + DoneEvaluating(false) {} >> >> ~CheckerContext(); >> >> + // FIXME: This were added to support CallAndMessageChecker to indicating >> + // to GRExprEngine to "stop evaluating" a message expression under certain >> + // cases. This is *not* meant to be a permanent API change, and was added >> + // to aid in the transition of removing logic for checks from >> GRExprEngine. >> + void setDoneEvaluating() { >> + DoneEvaluating = true; >> + } >> + bool isDoneEvaluating() const { >> + return DoneEvaluating; >> + } >> + >> ConstraintManager &getConstraintManager() { >> return Eng.getConstraintManager(); >> } >> @@ -152,7 +165,7 @@ >> friend class GRExprEngine; >> >> // FIXME: Remove the 'tag' option. >> - void GR_Visit(ExplodedNodeSet &Dst, >> + bool GR_Visit(ExplodedNodeSet &Dst, >> GRStmtNodeBuilder &Builder, >> GRExprEngine &Eng, >> const Stmt *S, >> @@ -164,6 +177,7 @@ >> _PreVisit(C, S); >> else >> _PostVisit(C, S); >> + return C.isDoneEvaluating(); >> } >> >> // FIXME: Remove the 'tag' option. >> >> Modified: cfe/trunk/include/clang/Analysis/PathSensitive/ExplodedGraph.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/ExplodedGraph.h?rev=89804&r1=89803&r2=89804&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/PathSensitive/ExplodedGraph.h (original) >> +++ cfe/trunk/include/clang/Analysis/PathSensitive/ExplodedGraph.h Tue Nov >> 24 15:41:28 2009 >> @@ -352,10 +352,16 @@ >> typedef ImplTy::iterator iterator; >> typedef ImplTy::const_iterator const_iterator; >> >> - inline unsigned size() const { return Impl.size(); } >> - inline bool empty() const { return Impl.empty(); } >> + unsigned size() const { return Impl.size(); } >> + bool empty() const { return Impl.empty(); } >> >> - inline void clear() { Impl.clear(); } >> + void clear() { Impl.clear(); } >> + void insert(const ExplodedNodeSet &S) { >> + if (empty()) >> + Impl = S.Impl; >> + else >> + Impl.insert(S.begin(), S.end()); >> + } >> >> inline iterator begin() { return Impl.begin(); } >> inline iterator end() { return Impl.end(); } >> >> Modified: cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h?rev=89804&r1=89803&r2=89804&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h (original) >> +++ cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h Tue Nov 24 >> 15:41:28 2009 >> @@ -222,7 +222,7 @@ >> protected: >> /// CheckerVisit - Dispatcher for performing checker-specific logic >> /// at specific statements. >> - void CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, ExplodedNodeSet &Src, >> + bool CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, ExplodedNodeSet &Src, >> bool isPrevisit); >> >> void CheckerVisitBind(const Stmt *AssignE, const Stmt *StoreE, >> >> Modified: cfe/trunk/lib/Analysis/CallAndMessageChecker.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CallAndMessageChecker.cpp?rev=89804&r1=89803&r2=89804&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Analysis/CallAndMessageChecker.cpp (original) >> +++ cfe/trunk/lib/Analysis/CallAndMessageChecker.cpp Tue Nov 24 15:41:28 2009 >> @@ -27,21 +27,27 @@ >> BugType *BT_call_arg; >> BugType *BT_msg_undef; >> BugType *BT_msg_arg; >> - BugType *BT_struct_ret; >> - BugType *BT_void_ptr; >> + BugType *BT_msg_ret; >> public: >> CallAndMessageChecker() : >> BT_call_null(0), BT_call_undef(0), BT_call_arg(0), >> - BT_msg_undef(0), BT_msg_arg(0), BT_struct_ret(0), BT_void_ptr(0) {} >> + BT_msg_undef(0), BT_msg_arg(0), BT_msg_ret(0) {} >> >> static void *getTag() { >> static int x = 0; >> return &x; >> } >> + >> void PreVisitCallExpr(CheckerContext &C, const CallExpr *CE); >> void PreVisitObjCMessageExpr(CheckerContext &C, const ObjCMessageExpr *ME); >> + >> private: >> void EmitBadCall(BugType *BT, CheckerContext &C, const CallExpr *CE); >> + void EmitNilReceiverBug(CheckerContext &C, const ObjCMessageExpr *ME, >> + ExplodedNode *N); >> + >> + void HandleNilReceiver(CheckerContext &C, const GRState *state, >> + const ObjCMessageExpr *ME); >> }; >> } // end anonymous namespace >> >> @@ -142,111 +148,107 @@ >> } >> } >> >> - // Check if the receiver was nil and then return value a struct. >> + // Check if the receiver was nil and then returns a value that may >> + // be garbage. >> if (const Expr *Receiver = ME->getReceiver()) { >> - SVal L_untested = state->getSVal(Receiver); >> - // Assume that the receiver is not NULL. >> - DefinedOrUnknownSVal L = cast<DefinedOrUnknownSVal>(L_untested); >> - const GRState *StNotNull = state->Assume(L, true); >> - >> - // Assume that the receiver is NULL. >> - const GRState *StNull = state->Assume(L, false); >> - >> - if (StNull) { >> - QualType RetTy = ME->getType(); >> - if (RetTy->isRecordType()) { >> - if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { >> - // The [0 ...] expressions will return garbage. Flag either an >> - // explicit or implicit error. Because of the structure of this >> - // function we currently do not bifurfacte the state graph at >> - // this point. >> - // FIXME: We should bifurcate and fill the returned struct with >> - // garbage. >> - if (ExplodedNode* N = C.GenerateSink(StNull)) { >> - if (!StNotNull) { >> - if (!BT_struct_ret) { >> - std::string sbuf; >> - llvm::raw_string_ostream os(sbuf); >> - os << "The receiver in the message expression is 'nil' and " >> - "results in the returned value (of type '" >> - << ME->getType().getAsString() >> - << "') to be garbage or otherwise undefined"; >> - BT_struct_ret = new BuiltinBug(os.str().c_str()); >> - } >> - >> - EnhancedBugReport *R = new EnhancedBugReport(*BT_struct_ret, >> - >> BT_struct_ret->getName(), N); >> - R->addRange(Receiver->getSourceRange()); >> - >> R->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, >> - Receiver); >> - C.EmitReport(R); >> - return; >> - } >> - else >> - // Do not report implicit bug. >> - return; >> - } >> - } >> - } else { >> - ASTContext &Ctx = C.getASTContext(); >> - if (RetTy != Ctx.VoidTy) { >> - if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { >> - // sizeof(void *) >> - const uint64_t voidPtrSize = Ctx.getTypeSize(Ctx.VoidPtrTy); >> - // sizeof(return type) >> - const uint64_t returnTypeSize = Ctx.getTypeSize(ME->getType()); >> - >> - if (voidPtrSize < returnTypeSize) { >> - if (ExplodedNode* N = C.GenerateSink(StNull)) { >> - if (!StNotNull) { >> - if (!BT_struct_ret) { >> - std::string sbuf; >> - llvm::raw_string_ostream os(sbuf); >> - os << "The receiver in the message expression is 'nil' >> and " >> - "results in the returned value (of type '" >> - << ME->getType().getAsString() >> - << "' and of size " >> - << returnTypeSize / 8 >> - << " bytes) to be garbage or otherwise undefined"; >> - BT_void_ptr = new BuiltinBug(os.str().c_str()); >> - } >> - >> - EnhancedBugReport *R = new EnhancedBugReport(*BT_void_ptr, >> - >> BT_void_ptr->getName(), N); >> - R->addRange(Receiver->getSourceRange()); >> - R->addVisitorCreator( >> - bugreporter::registerTrackNullOrUndefValue, >> Receiver); >> - C.EmitReport(R); >> - return; >> - } else >> - // Do not report implicit bug. >> - return; >> - } >> - } >> - else if (!StNotNull) { >> - // Handle the safe cases where the return value is 0 if the >> - // receiver is nil. >> - // >> - // FIXME: For now take the conservative approach that we only >> - // return null values if we *know* that the receiver is nil. >> - // This is because we can have surprises like: >> - // >> - // ... = [[NSScreens screens] objectAtIndex:0]; >> - // >> - // What can happen is that [... screens] could return nil, but >> - // it most likely isn't nil. We should assume the semantics >> - // of this case unless we have *a lot* more knowledge. >> - // >> - SVal V = C.getValueManager().makeZeroVal(ME->getType()); >> - C.GenerateNode(StNull->BindExpr(ME, V)); >> - return; >> - } >> - } >> - } >> - } >> + DefinedOrUnknownSVal receiverVal = >> + cast<DefinedOrUnknownSVal>(state->getSVal(Receiver)); >> + >> + const GRState *notNullState, *nullState; >> + llvm::tie(notNullState, nullState) = state->Assume(receiverVal); >> + >> + if (nullState && !notNullState) { >> + HandleNilReceiver(C, nullState, ME); >> + C.setDoneEvaluating(); // FIXME: eventually remove. >> + return; >> } >> - // Do not propagate null state. >> - if (StNotNull) >> - C.GenerateNode(StNotNull); >> + >> + assert(notNullState); >> + state = notNullState; >> } >> + >> + // Add a state transition if the state has changed. >> + C.addTransition(state); >> +} >> + >> +void CallAndMessageChecker::EmitNilReceiverBug(CheckerContext &C, >> + const ObjCMessageExpr *ME, >> + ExplodedNode *N) { >> + >> + if (!BT_msg_ret) >> + BT_msg_ret = >> + new BuiltinBug("Receiver in message expression is " >> + "'nil' and returns a garbage value"); >> + >> + llvm::SmallString<200> buf; >> + llvm::raw_svector_ostream os(buf); >> + os << "The receiver of message '" << ME->getSelector().getAsString() >> + << "' is nil and returns a value of type '" >> + << ME->getType().getAsString() << "' that will be garbage"; >> + >> + EnhancedBugReport *report = new EnhancedBugReport(*BT_msg_ret, os.str(), >> N); >> + const Expr *receiver = ME->getReceiver(); >> + report->addRange(receiver->getSourceRange()); >> + report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, >> + receiver); >> + C.EmitReport(report); >> +} >> + >> +void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C, >> + const GRState *state, >> + const ObjCMessageExpr *ME) { >> + >> + // Check the return type of the message expression. A message to nil will >> + // return different values depending on the return type and the >> architecture. >> + QualType RetTy = ME->getType(); >> + >> + if (RetTy->isStructureType()) { >> + // FIXME: At some point we shouldn't rely on isConsumedExpr(), but >> instead >> + // have the "use of undefined value" be smarter about where the >> + // undefined value came from. >> + if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { >> + if (ExplodedNode* N = C.GenerateSink(state)) >> + EmitNilReceiverBug(C, ME, N); >> + return; >> + } >> + >> + // The result is not consumed by a surrounding expression. Just >> propagate >> + // the current state. >> + C.addTransition(state); >> + return; >> + } >> + >> + // Other cases: check if the return type is smaller than void*. >> + ASTContext &Ctx = C.getASTContext(); >> + if (RetTy != Ctx.VoidTy && >> + C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { >> + // Compute: sizeof(void *) and sizeof(return type) >> + const uint64_t voidPtrSize = Ctx.getTypeSize(Ctx.VoidPtrTy); >> + const uint64_t returnTypeSize = Ctx.getTypeSize(ME->getType()); >> + >> + if (voidPtrSize < returnTypeSize) { >> + if (ExplodedNode* N = C.GenerateSink(state)) >> + EmitNilReceiverBug(C, ME, N); >> + return; >> + } >> + >> + // Handle the safe cases where the return value is 0 if the >> + // receiver is nil. >> + // >> + // FIXME: For now take the conservative approach that we only >> + // return null values if we *know* that the receiver is nil. >> + // This is because we can have surprises like: >> + // >> + // ... = [[NSScreens screens] objectAtIndex:0]; >> + // >> + // What can happen is that [... screens] could return nil, but >> + // it most likely isn't nil. We should assume the semantics >> + // of this case unless we have *a lot* more knowledge. >> + // >> + SVal V = C.getValueManager().makeZeroVal(ME->getType()); >> + C.GenerateNode(state->BindExpr(ME, V)); >> + return; >> + } >> + >> + C.addTransition(state); >> } >> >> Modified: cfe/trunk/lib/Analysis/GRExprEngine.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/GRExprEngine.cpp?rev=89804&r1=89803&r2=89804&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Analysis/GRExprEngine.cpp (original) >> +++ cfe/trunk/lib/Analysis/GRExprEngine.cpp Tue Nov 24 15:41:28 2009 >> @@ -108,12 +108,12 @@ >> // Checker worklist routines. >> >> //===----------------------------------------------------------------------===// >> >> -void GRExprEngine::CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, >> +bool GRExprEngine::CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, >> ExplodedNodeSet &Src, bool isPrevisit) { >> >> if (Checkers.empty()) { >> - Dst = Src; >> - return; >> + Dst.insert(Src); >> + return false; >> } >> >> ExplodedNodeSet Tmp; >> @@ -129,8 +129,16 @@ >> Checker *checker = I->second; >> >> for (ExplodedNodeSet::iterator NI = PrevSet->begin(), NE = >> PrevSet->end(); >> - NI != NE; ++NI) >> - checker->GR_Visit(*CurrSet, *Builder, *this, S, *NI, tag, isPrevisit); >> + NI != NE; ++NI) { >> + // FIXME: Halting evaluation of the checkers is something we may >> + // not support later. The design is still evolving. >> + if (checker->GR_Visit(*CurrSet, *Builder, *this, S, *NI, >> + tag, isPrevisit)) { >> + if (CurrSet != &Dst) >> + Dst.insert(*CurrSet); >> + return true; >> + } >> + } >> >> // Update which NodeSet is the current one. >> PrevSet = CurrSet; >> @@ -138,6 +146,7 @@ >> >> // Don't autotransition. The CheckerContext objects should do this >> // automatically. >> + return false; >> } >> >> // FIXME: This is largely copy-paste from CheckerVisit(). Need to >> @@ -1854,8 +1863,12 @@ >> >> // Handle previsits checks. >> ExplodedNodeSet Src, DstTmp; >> - Src.Add(Pred); >> - CheckerVisit(ME, DstTmp, Src, true); >> + Src.Add(Pred); >> + >> + if (CheckerVisit(ME, DstTmp, Src, true)) { >> + Dst.insert(DstTmp); >> + return; >> + } >> >> unsigned size = Dst.size(); >> >> >> Modified: >> cfe/trunk/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m?rev=89804&r1=89803&r2=89804&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m >> (original) >> +++ cfe/trunk/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m >> Tue Nov 24 15:41:28 2009 >> @@ -28,20 +28,20 @@ >> void createFoo2() { >> MyClass *obj = 0; >> >> - long double ld = [obj longDoubleM]; // expected-warning{{The receiver in >> the message expression is 'nil' and results in the returned value}} >> + long double ld = [obj longDoubleM]; // expected-warning{{The receiver of >> message 'longDoubleM' is nil and returns a value of type 'long double' that >> will be garbage}} >> } >> >> void createFoo3() { >> MyClass *obj; >> obj = 0; >> >> - long long ll = [obj longlongM]; // expected-warning{{The receiver in the >> message expression is 'nil' and results in the returned value}} >> + long long ll = [obj longlongM]; // expected-warning{{The receiver of >> message 'longlongM' is nil and returns a value of type 'long long' that will >> be garbage}} >> } >> >> void createFoo4() { >> MyClass *obj = 0; >> >> - double d = [obj doubleM]; // expected-warning{{The receiver in the >> message expression is 'nil' and results in the returned value}} >> + double d = [obj doubleM]; // expected-warning{{The receiver of message >> 'doubleM' is nil and returns a value of type 'double' that will be garbage}} >> } >> >> void createFoo5() { >> @@ -59,7 +59,7 @@ >> long long j = [obj longlongM]; // no-warning >> } >> >> - long long j = [obj longlongM]; // expected-warning{{The receiver in the >> message expression is 'nil' and results in the returned value}} >> + long long j = [obj longlongM]; // expected-warning{{The receiver of >> message 'longlongM' is nil and returns a value of type 'long long' that will >> be garbage}} >> } >> >> int handleVoidInComma() { >> >> Modified: >> cfe/trunk/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m?rev=89804&r1=89803&r2=89804&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m >> (original) >> +++ cfe/trunk/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m >> Tue Nov 24 15:41:28 2009 >> @@ -15,12 +15,12 @@ >> >> void createFoo() { >> MyClass *obj = 0; >> - Bar f = [obj foo]; // expected-warning{{The receiver in the message >> expression is 'nil' and results in the returned value (of type 'Bar') to be >> garbage or otherwise undefined.}} >> + Bar f = [obj foo]; // expected-warning{{The receiver of message 'foo' is >> nil and returns a value of type 'Bar' that will be garbage}} >> } >> >> void createFoo2() { >> MyClass *obj = 0; >> [obj foo]; // no-warning >> - Bar f = [obj foo]; // expected-warning{{The receiver in the message >> expression is 'nil' and results in the returned value (of type 'Bar') to be >> garbage or otherwise undefined.}} >> + Bar f = [obj foo]; // expected-warning{{The receiver of message 'foo' is >> nil and returns a value of type 'Bar' that will be garbage}} >> } >> >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
