[cfe-commits] r89804 - in /cfe/trunk: include/clang/Analysis/PathSensitive/Checker.h include/clang/Analysis/PathSensitive/ExplodedGraph.h include/clang/Analysis/PathSensitive/GRExprEngine.h lib/Analysis/CallAndMessageChecker.cpp lib/Analysis/GREx
Zhongxing Xu
xuzhongxing at gmail.com
Wed Nov 25 07:15:42 PST 2009
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 <kremenek at apple.com>:
> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list