dcoughlin updated this revision to Diff 32864.
dcoughlin added a comment.
Update comments to correct nil/non-nil mistakes.
http://reviews.llvm.org/D12123
Files:
include/clang/StaticAnalyzer/Core/Checker.h
include/clang/StaticAnalyzer/Core/CheckerManager.h
lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
lib/StaticAnalyzer/Core/CheckerManager.cpp
lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
test/Analysis/NSContainers.m
test/Analysis/objc-message.m
Index: test/Analysis/objc-message.m
===================================================================
--- /dev/null
+++ 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}}
+ }
+}
Index: test/Analysis/NSContainers.m
===================================================================
--- test/Analysis/NSContainers.m
+++ test/Analysis/NSContainers.m
@@ -24,6 +24,8 @@
@interface NSObject <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: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -139,6 +139,69 @@
CallEventRef<ObjCMethodCall> 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 (<rdar://problem/11733396>).
+ // 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<DefinedOrUnknownSVal>();
+ 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) {
+ ExplodedNodeSet dstNil;
+ StmtNodeBuilder Bldr(Pred, dstNil, *currBldrCtx);
+
+ Pred = Bldr.generateNode(ME, Pred, nilState);
+ assert(Pred && "Should have cached out already!");
+ getCheckerManager().runCheckersForObjCMessageNil(Dst, dstNil,
+ *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) {
+ Pred = Bldr.generateNode(ME, Pred, notNilState);
+ assert(Pred && "Should have cached out already!");
+ }
+ }
+ }
+
// Handle the previsits checks.
ExplodedNodeSet dstPrevisit;
getCheckerManager().runCheckersForPreObjCMessage(dstPrevisit, Pred,
@@ -160,35 +223,12 @@
if (UpdatedMsg->isInstanceMessage()) {
SVal recVal = UpdatedMsg->getReceiverSVal();
if (!recVal.isUndef()) {
- // Bifurcate the state into nil and non-nil ones.
- DefinedOrUnknownSVal receiverVal =
- recVal.castAs<DefinedOrUnknownSVal>();
-
- 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 (<rdar://problem/11733396>).
- // 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) {
- Pred = Bldr.generateNode(ME, Pred, notNilState);
- assert(Pred && "Should have cached out already!");
- }
}
} else {
// Check for special class methods that are known to not return
Index: lib/StaticAnalyzer/Core/CheckerManager.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -177,22 +177,38 @@
namespace {
struct CheckObjCMessageContext {
typedef std::vector<CheckerManager::CheckObjCMessageFunc> CheckersTy;
- bool IsPreVisit, WasInlined;
+
+ ObjCMessageVisitKind Kind;
+ bool WasInlined;
const CheckersTy &Checkers;
const ObjCMethodCall &Msg;
ExprEngine &Eng;
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:
+ case ObjCMessageVisitKind::MessageNil:
+ IsPreVisit = true;
+ break;
+ 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::CheckObjCMessageFunc> &
+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: lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
+++ lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
@@ -38,6 +38,7 @@
check::PostStmt<DeclStmt>,
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: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -40,6 +40,7 @@
: public Checker< check::PreStmt<CallExpr>,
check::PreStmt<CXXDeleteExpr>,
check::PreObjCMessage,
+ check::ObjCMessageNil,
check::PreCall > {
mutable std::unique_ptr<BugType> BT_call_null;
mutable std::unique_ptr<BugType> 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<DefinedOrUnknownSVal>();
-
- 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: include/clang/StaticAnalyzer/Core/CheckerManager.h
===================================================================
--- include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ 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,21 +217,31 @@
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.
void runCheckersForPostObjCMessage(ExplodedNodeSet &Dst,
const ExplodedNodeSet &Src,
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<CheckObjCMessageFunc> &
+ getObjCMessageCheckers(ObjCMessageVisitKind Kind);
+
std::vector<CheckObjCMessageFunc> PreObjCMessageCheckers;
std::vector<CheckObjCMessageFunc> PostObjCMessageCheckers;
+ std::vector<CheckObjCMessageFunc> ObjCMessageNilCheckers;
std::vector<CheckCallFunc> PreCallCheckers;
std::vector<CheckCallFunc> PostCallCheckers;
Index: include/clang/StaticAnalyzer/Core/Checker.h
===================================================================
--- include/clang/StaticAnalyzer/Core/Checker.h
+++ include/clang/StaticAnalyzer/Core/Checker.h
@@ -131,6 +131,21 @@
}
};
+class ObjCMessageNil {
+ template <typename CHECKER>
+ static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg,
+ CheckerContext &C) {
+ ((const CHECKER *)checker)->checkObjCMessageNil(msg, C);
+ }
+
+public:
+ template <typename CHECKER>
+ static void _register(CHECKER *checker, CheckerManager &mgr) {
+ mgr._registerForObjCMessageNil(
+ CheckerManager::CheckObjCMessageFunc(checker, _checkObjCMessage<CHECKER>));
+ }
+};
+
class PostObjCMessage {
template <typename CHECKER>
static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg,
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
