[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
Ted Kremenek
kremenek at apple.com
Wed Nov 25 13:41:04 PST 2009
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 <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