Index: cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h =================================================================== --- cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h +++ cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h @@ -131,6 +131,21 @@ } }; +class ObjCMessageNil { + template + static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg, + CheckerContext &C) { + ((const CHECKER *)checker)->checkObjCMessageNil(msg, C); + } + +public: + template + static void _register(CHECKER *checker, CheckerManager &mgr) { + mgr._registerForObjCMessageNil( + CheckerManager::CheckObjCMessageFunc(checker, _checkObjCMessage)); + } +}; + class PostObjCMessage { template static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg, Index: cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h =================================================================== --- cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h +++ cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h @@ -93,6 +93,12 @@ StringRef getName() const { return Name; } }; +enum class ObjCMessageVisitKind { + Pre, + Post, + MessageNil +}; + class CheckerManager { const LangOptions LangOpts; AnalyzerOptionsRef AOptions; @@ -211,7 +217,7 @@ const ExplodedNodeSet &Src, const ObjCMethodCall &msg, ExprEngine &Eng) { - runCheckersForObjCMessage(/*isPreVisit=*/true, Dst, Src, msg, Eng); + runCheckersForObjCMessage(ObjCMessageVisitKind::Pre, Dst, Src, msg, Eng); } /// \brief Run checkers for post-visiting obj-c messages. @@ -220,12 +226,22 @@ const ObjCMethodCall &msg, ExprEngine &Eng, bool wasInlined = false) { - runCheckersForObjCMessage(/*isPreVisit=*/false, Dst, Src, msg, Eng, + runCheckersForObjCMessage(ObjCMessageVisitKind::Post, Dst, Src, msg, Eng, wasInlined); } + /// \brief Run checkers for visiting an obj-c message to nil. + void runCheckersForObjCMessageNil(ExplodedNodeSet &Dst, + const ExplodedNodeSet &Src, + const ObjCMethodCall &msg, + ExprEngine &Eng) { + runCheckersForObjCMessage(ObjCMessageVisitKind::MessageNil, Dst, Src, msg, + Eng); + } + + /// \brief Run checkers for visiting obj-c messages. - void runCheckersForObjCMessage(bool isPreVisit, + void runCheckersForObjCMessage(ObjCMessageVisitKind visitKind, ExplodedNodeSet &Dst, const ExplodedNodeSet &Src, const ObjCMethodCall &msg, ExprEngine &Eng, @@ -457,6 +473,8 @@ void _registerForPreObjCMessage(CheckObjCMessageFunc checkfn); void _registerForPostObjCMessage(CheckObjCMessageFunc checkfn); + void _registerForObjCMessageNil(CheckObjCMessageFunc checkfn); + void _registerForPreCall(CheckCallFunc checkfn); void _registerForPostCall(CheckCallFunc checkfn); @@ -557,8 +575,14 @@ const CachedStmtCheckers &getCachedStmtCheckersFor(const Stmt *S, bool isPreVisit); + /// Returns the checkers that have registered for callbacks of the + /// given \p Kind. + const std::vector & + getObjCMessageCheckers(ObjCMessageVisitKind Kind); + std::vector PreObjCMessageCheckers; std::vector PostObjCMessageCheckers; + std::vector ObjCMessageNilCheckers; std::vector PreCallCheckers; std::vector PostCallCheckers; Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -40,6 +40,7 @@ : public Checker< check::PreStmt, check::PreStmt, check::PreObjCMessage, + check::ObjCMessageNil, check::PreCall > { mutable std::unique_ptr BT_call_null; mutable std::unique_ptr BT_call_undef; @@ -60,6 +61,12 @@ void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const; void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C) const; + + /// Fill in the return value that results from messaging nil based on the + /// return type and architecture and diagnose if the return value will be + /// garbage. + void checkObjCMessageNil(const ObjCMethodCall &msg, CheckerContext &C) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; private: @@ -471,22 +478,14 @@ C.emitReport(std::move(R)); } return; - } else { - // Bifurcate the state into nil and non-nil ones. - DefinedOrUnknownSVal receiverVal = recVal.castAs(); - - ProgramStateRef state = C.getState(); - ProgramStateRef notNilState, nilState; - std::tie(notNilState, nilState) = state->assume(receiverVal); - - // Handle receiver must be nil. - if (nilState && !notNilState) { - HandleNilReceiver(C, state, msg); - return; - } } } +void CallAndMessageChecker::checkObjCMessageNil(const ObjCMethodCall &msg, + CheckerContext &C) const { + HandleNilReceiver(C, C.getState(), msg); +} + void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg, ExplodedNode *N) const { Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp @@ -38,6 +38,7 @@ check::PostStmt, check::PreObjCMessage, check::PostObjCMessage, + check::ObjCMessageNil, check::PreCall, check::PostCall, check::BranchCondition, @@ -95,6 +96,15 @@ /// check::PostObjCMessage void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const {} + /// \brief Visit an Objective-C message whose receiver is nil. + /// + /// This will be called when the analyzer core processes a method call whose + /// receiver is definitely nil. In this case, check{Pre/Post}ObjCMessage and + /// check{Pre/Post}Call will not be called. + /// + /// check::ObjCMessageNil + void checkObjCMessageNil(const ObjCMethodCall &M, CheckerContext &C) const {} + /// \brief Pre-visit an abstract "call" event. /// /// This is used for checkers that want to check arguments or attributed Index: cfe/trunk/lib/StaticAnalyzer/Core/CheckerManager.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ cfe/trunk/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -177,7 +177,9 @@ namespace { struct CheckObjCMessageContext { typedef std::vector CheckersTy; - bool IsPreVisit, WasInlined; + + ObjCMessageVisitKind Kind; + bool WasInlined; const CheckersTy &Checkers; const ObjCMethodCall &Msg; ExprEngine &Eng; @@ -185,14 +187,28 @@ CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); } CheckersTy::const_iterator checkers_end() { return Checkers.end(); } - CheckObjCMessageContext(bool isPreVisit, const CheckersTy &checkers, + CheckObjCMessageContext(ObjCMessageVisitKind visitKind, + const CheckersTy &checkers, const ObjCMethodCall &msg, ExprEngine &eng, bool wasInlined) - : IsPreVisit(isPreVisit), WasInlined(wasInlined), Checkers(checkers), + : Kind(visitKind), WasInlined(wasInlined), Checkers(checkers), Msg(msg), Eng(eng) { } void runChecker(CheckerManager::CheckObjCMessageFunc checkFn, NodeBuilder &Bldr, ExplodedNode *Pred) { + + bool IsPreVisit; + + switch (Kind) { + case ObjCMessageVisitKind::Pre: + IsPreVisit = true; + break; + case ObjCMessageVisitKind::MessageNil: + case ObjCMessageVisitKind::Post: + IsPreVisit = false; + break; + } + const ProgramPoint &L = Msg.getProgramPoint(IsPreVisit,checkFn.Checker); CheckerContext C(Bldr, Eng, Pred, L, WasInlined); @@ -202,19 +218,29 @@ } /// \brief Run checkers for visiting obj-c messages. -void CheckerManager::runCheckersForObjCMessage(bool isPreVisit, +void CheckerManager::runCheckersForObjCMessage(ObjCMessageVisitKind visitKind, ExplodedNodeSet &Dst, const ExplodedNodeSet &Src, const ObjCMethodCall &msg, ExprEngine &Eng, bool WasInlined) { - CheckObjCMessageContext C(isPreVisit, - isPreVisit ? PreObjCMessageCheckers - : PostObjCMessageCheckers, - msg, Eng, WasInlined); + auto &checkers = getObjCMessageCheckers(visitKind); + CheckObjCMessageContext C(visitKind, checkers, msg, Eng, WasInlined); expandGraphWithCheckers(C, Dst, Src); } +const std::vector & +CheckerManager::getObjCMessageCheckers(ObjCMessageVisitKind Kind) { + switch (Kind) { + case ObjCMessageVisitKind::Pre: + return PreObjCMessageCheckers; + break; + case ObjCMessageVisitKind::Post: + return PostObjCMessageCheckers; + case ObjCMessageVisitKind::MessageNil: + return ObjCMessageNilCheckers; + } +} namespace { // FIXME: This has all the same signatures as CheckObjCMessageContext. // Is there a way we can merge the two? @@ -616,6 +642,11 @@ void CheckerManager::_registerForPreObjCMessage(CheckObjCMessageFunc checkfn) { PreObjCMessageCheckers.push_back(checkfn); } + +void CheckerManager::_registerForObjCMessageNil(CheckObjCMessageFunc checkfn) { + ObjCMessageNilCheckers.push_back(checkfn); +} + void CheckerManager::_registerForPostObjCMessage(CheckObjCMessageFunc checkfn) { PostObjCMessageCheckers.push_back(checkfn); } Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -139,6 +139,74 @@ CallEventRef Msg = CEMgr.getObjCMethodCall(ME, Pred->getState(), Pred->getLocationContext()); + // There are three cases for the receiver: + // (1) it is definitely nil, + // (2) it is definitely non-nil, and + // (3) we don't know. + // + // If the receiver is definitely nil, we skip the pre/post callbacks and + // instead call the ObjCMessageNil callbacks and return. + // + // If the receiver is definitely non-nil, we call the pre- callbacks, + // evaluate the call, and call the post- callbacks. + // + // If we don't know, we drop the potential nil flow and instead + // continue from the assumed non-nil state as in (2). This approach + // intentionally drops coverage in order to prevent false alarms + // in the following scenario: + // + // id result = [o someMethod] + // if (result) { + // if (!o) { + // // <-- This program point should be unreachable because if o is nil + // // it must the case that result is nil as well. + // } + // } + // + // We could avoid dropping coverage by performing an explicit case split + // on each method call -- but this would get very expensive. An alternative + // would be to introduce lazy constraints. + // FIXME: This ignores many potential bugs (). + // Revisit once we have lazier constraints. + if (Msg->isInstanceMessage()) { + SVal recVal = Msg->getReceiverSVal(); + if (!recVal.isUndef()) { + // Bifurcate the state into nil and non-nil ones. + DefinedOrUnknownSVal receiverVal = + recVal.castAs(); + ProgramStateRef State = Pred->getState(); + + ProgramStateRef notNilState, nilState; + std::tie(notNilState, nilState) = State->assume(receiverVal); + + // Receiver is definitely nil, so run ObjCMessageNil callbacks and return. + if (nilState && !notNilState) { + StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx); + bool HasTag = Pred->getLocation().getTag(); + Pred = Bldr.generateNode(ME, Pred, nilState, nullptr, + ProgramPoint::PreStmtKind); + assert((Pred || HasTag) && "Should have cached out already!"); + if (!Pred) + return; + getCheckerManager().runCheckersForObjCMessageNil(Dst, Pred, + *Msg, *this); + return; + } + + ExplodedNodeSet dstNonNil; + StmtNodeBuilder Bldr(Pred, dstNonNil, *currBldrCtx); + // Generate a transition to the non-nil state, dropping any potential + // nil flow. + if (notNilState != State) { + bool HasTag = Pred->getLocation().getTag(); + Pred = Bldr.generateNode(ME, Pred, notNilState); + assert((Pred || HasTag) && "Should have cached out already!"); + if (!Pred) + return; + } + } + } + // Handle the previsits checks. ExplodedNodeSet dstPrevisit; getCheckerManager().runCheckersForPreObjCMessage(dstPrevisit, Pred, @@ -160,38 +228,12 @@ if (UpdatedMsg->isInstanceMessage()) { SVal recVal = UpdatedMsg->getReceiverSVal(); if (!recVal.isUndef()) { - // Bifurcate the state into nil and non-nil ones. - DefinedOrUnknownSVal receiverVal = - recVal.castAs(); - - ProgramStateRef notNilState, nilState; - std::tie(notNilState, nilState) = State->assume(receiverVal); - - // There are three cases: can be nil or non-nil, must be nil, must be - // non-nil. We ignore must be nil, and merge the rest two into non-nil. - // FIXME: This ignores many potential bugs (). - // Revisit once we have lazier constraints. - if (nilState && !notNilState) { - continue; - } - - // Check if the "raise" message was sent. - assert(notNilState); if (ObjCNoRet.isImplicitNoReturn(ME)) { // If we raise an exception, for now treat it as a sink. // Eventually we will want to handle exceptions properly. Bldr.generateSink(ME, Pred, State); continue; } - - // Generate a transition to non-Nil state. - if (notNilState != State) { - bool HasTag = Pred->getLocation().getTag(); - Pred = Bldr.generateNode(ME, Pred, notNilState); - assert((Pred || HasTag) && "Should have cached out already!"); - if (!Pred) - continue; - } } } else { // Check for special class methods that are known to not return Index: cfe/trunk/test/Analysis/NSContainers.m =================================================================== --- cfe/trunk/test/Analysis/NSContainers.m +++ cfe/trunk/test/Analysis/NSContainers.m @@ -24,6 +24,8 @@ @interface NSObject {} - (id)init; + (id)alloc; + +- (id)mutableCopy; @end typedef struct { @@ -292,3 +294,20 @@ [arr addObject:0 safe:1]; // no-warning } +@interface MyView : NSObject +-(NSArray *)subviews; +@end + +void testNoReportWhenReceiverNil(NSMutableArray *array, int b) { + // Don't warn about adding nil to a container when the receiver is also + // definitely nil. + if (array == 0) { + [array addObject:0]; // no-warning + } + + MyView *view = b ? [[MyView alloc] init] : 0; + NSMutableArray *subviews = [[view subviews] mutableCopy]; + // When view is nil, subviews is also nil so there should be no warning + // here either. + [subviews addObject:view]; // no-warning +} Index: cfe/trunk/test/Analysis/objc-message.m =================================================================== --- cfe/trunk/test/Analysis/objc-message.m +++ cfe/trunk/test/Analysis/objc-message.m @@ -0,0 +1,40 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class %s + +extern void clang_analyzer_warnIfReached(); +void clang_analyzer_eval(int); + +@interface SomeClass +-(id)someMethodWithReturn; +-(void)someMethod; +@end + +void consistencyOfReturnWithNilReceiver(SomeClass *o) { + id result = [o someMethodWithReturn]; + if (result) { + if (!o) { + // It is impossible for both o to be nil and result to be non-nil, + // so this should not be reached. + clang_analyzer_warnIfReached(); // no-warning + } + } +} + +void maybeNilReceiverIsNotNilAfterMessage(SomeClass *o) { + [o someMethod]; + + // We intentionally drop the nil flow (losing coverage) after a method + // call when the receiver may be nil in order to avoid inconsistencies of + // the kind tested for in consistencyOfReturnWithNilReceiver(). + clang_analyzer_eval(o != 0); // expected-warning{{TRUE}} +} + +void nilReceiverIsStillNilAfterMessage(SomeClass *o) { + if (o == 0) { + id result = [o someMethodWithReturn]; + + // Both the receiver and the result should be nil after a message + // sent to a nil receiver returning a value of type id. + clang_analyzer_eval(o == 0); // expected-warning{{TRUE}} + clang_analyzer_eval(result == 0); // expected-warning{{TRUE}} + } +}