From b673a41c92aa276f2e37164d0747be1cfb0c402b Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Sat, 18 Feb 2012 20:53:30 +0000 Subject: [PATCH] Adopt ExprEngine and checkers to ObjC property refactoring. Everything was working, but now diagnostics are aware of message expressions implied by uses of properties. Fixes . git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@150888 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Core/PathSensitive/ObjCMessage.h | 167 +++++++----------- .../Checkers/CallAndMessageChecker.cpp | 25 ++- .../Checkers/ObjCSelfInitChecker.cpp | 2 +- lib/StaticAnalyzer/Core/CheckerManager.cpp | 6 +- lib/StaticAnalyzer/Core/ExprEngine.cpp | 34 +++- lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 2 +- lib/StaticAnalyzer/Core/ObjCMessage.cpp | 93 ---------- test/Analysis/properties.m | 23 +++ 8 files changed, 132 insertions(+), 220 deletions(-) diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h index c74d07c63f..e87e08fe82 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h @@ -28,70 +28,56 @@ namespace ento { /// \brief Represents both explicit ObjC message expressions and implicit /// messages that are sent for handling properties in dot syntax. class ObjCMessage { - const Expr *MsgOrPropE; - const Expr *OriginE; - bool IsPropSetter; - SVal SetterArgV; - -protected: - ObjCMessage(const Expr *E, const Expr *origE, bool isSetter, SVal setArgV) - : MsgOrPropE(E), OriginE(origE), - IsPropSetter(isSetter), SetterArgV(setArgV) { } - + const ObjCMessageExpr *Msg; + const ObjCPropertyRefExpr *PE; + const bool IsPropSetter; public: - ObjCMessage() : MsgOrPropE(0), OriginE(0) { } + ObjCMessage() : Msg(0), PE(0), IsPropSetter(false) {} - ObjCMessage(const ObjCMessageExpr *E) - : MsgOrPropE(E), OriginE(E) { + ObjCMessage(const ObjCMessageExpr *E, const ObjCPropertyRefExpr *pe = 0, + bool isSetter = false) + : Msg(E), PE(pe), IsPropSetter(isSetter) { assert(E && "should not be initialized with null expression"); } - bool isValid() const { return MsgOrPropE != 0; } - bool isInvalid() const { return !isValid(); } + bool isValid() const { return Msg; } + + bool isPureMessageExpr() const { return !PE; } - bool isMessageExpr() const { - return isValid() && isa(MsgOrPropE); - } + bool isPropertyGetter() const { return PE && !IsPropSetter; } - bool isPropertyGetter() const { - return isValid() && - isa(MsgOrPropE) && !IsPropSetter; + bool isPropertySetter() const { + return IsPropSetter; } - bool isPropertySetter() const { - return isValid() && - isa(MsgOrPropE) && IsPropSetter; + const Expr *getMessageExpr() const { + return Msg; } - - const Expr *getOriginExpr() const { return OriginE; } - QualType getType(ASTContext &ctx) const; + QualType getType(ASTContext &ctx) const { + return Msg->getType(); + } QualType getResultType(ASTContext &ctx) const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - if (const ObjCMethodDecl *MD = msgE->getMethodDecl()) - return MD->getResultType(); + if (const ObjCMethodDecl *MD = Msg->getMethodDecl()) + return MD->getResultType(); return getType(ctx); } - ObjCMethodFamily getMethodFamily() const; + ObjCMethodFamily getMethodFamily() const { + return Msg->getMethodFamily(); + } - Selector getSelector() const; + Selector getSelector() const { + return Msg->getSelector(); + } const Expr *getInstanceReceiver() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getInstanceReceiver(); - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - if (propE->isObjectReceiver()) - return propE->getBase(); - return 0; + return Msg->getInstanceReceiver(); } SVal getInstanceReceiverSVal(ProgramStateRef State, const LocationContext *LC) const { - assert(isValid() && "This ObjCMessage is uninitialized!"); if (!isInstanceMessage()) return UndefinedVal(); if (const Expr *Ex = getInstanceReceiver()) @@ -105,101 +91,66 @@ public: } bool isInstanceMessage() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->isInstanceMessage(); - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - // FIXME: 'super' may be super class. - return propE->isObjectReceiver() || propE->isSuperReceiver(); + return Msg->isInstanceMessage(); } - const ObjCMethodDecl *getMethodDecl() const; - - const ObjCInterfaceDecl *getReceiverInterface() const; - - SourceLocation getSuperLoc() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getSuperLoc(); - return cast(MsgOrPropE)->getReceiverLocation(); + const ObjCMethodDecl *getMethodDecl() const { + return Msg->getMethodDecl(); } - const Expr *getMsgOrPropExpr() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - return MsgOrPropE; + const ObjCInterfaceDecl *getReceiverInterface() const { + return Msg->getReceiverInterface(); } + SourceLocation getSuperLoc() const { + if (PE) + return PE->getReceiverLocation(); + return Msg->getSuperLoc(); + } + SourceRange getSourceRange() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - return MsgOrPropE->getSourceRange(); + if (PE) + return PE->getSourceRange(); + return Msg->getSourceRange(); } unsigned getNumArgs() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getNumArgs(); - return isPropertySetter() ? 1 : 0; + return Msg->getNumArgs(); } SVal getArgSVal(unsigned i, const LocationContext *LCtx, ProgramStateRef state) const { - assert(isValid() && "This ObjCMessage is uninitialized!"); assert(i < getNumArgs() && "Invalid index for argument"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return state->getSVal(msgE->getArg(i), LCtx); - assert(isPropertySetter()); - return SetterArgV; + return state->getSVal(Msg->getArg(i), LCtx); } QualType getArgType(unsigned i) const { - assert(isValid() && "This ObjCMessage is uninitialized!"); assert(i < getNumArgs() && "Invalid index for argument"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getArg(i)->getType(); - assert(isPropertySetter()); - return cast(MsgOrPropE)->getType(); + return Msg->getArg(i)->getType(); } - const Expr *getArgExpr(unsigned i) const; + const Expr *getArgExpr(unsigned i) const { + assert(i < getNumArgs() && "Invalid index for argument"); + return Msg->getArg(i); + } SourceRange getArgSourceRange(unsigned i) const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - assert(i < getNumArgs() && "Invalid index for argument"); - if (const Expr *argE = getArgExpr(i)) - return argE->getSourceRange(); - return OriginE->getSourceRange(); + const Expr *argE = getArgExpr(i); + return argE->getSourceRange(); } SourceRange getReceiverSourceRange() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getReceiverRange(); - - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - if (propE->isObjectReceiver()) - return propE->getBase()->getSourceRange(); + if (PE) { + if (PE->isObjectReceiver()) + return PE->getBase()->getSourceRange(); + } + else { + return Msg->getReceiverRange(); + } // FIXME: This isn't a range. - return propE->getReceiverLocation(); - } -}; - -class ObjCPropertyGetter : public ObjCMessage { -public: - ObjCPropertyGetter(const ObjCPropertyRefExpr *propE, const Expr *originE) - : ObjCMessage(propE, originE, false, SVal()) { - assert(propE && originE && - "should not be initialized with null expressions"); - } -}; - -class ObjCPropertySetter : public ObjCMessage { -public: - ObjCPropertySetter(const ObjCPropertyRefExpr *propE, const Expr *storeE, - SVal argV) - : ObjCMessage(propE, storeE, true, argV) { - assert(propE && storeE &&"should not be initialized with null expressions"); + return PE->getReceiverLocation(); } }; @@ -252,7 +203,7 @@ public: const Expr *getOriginExpr() const { if (!CallE) - return Msg.getOriginExpr(); + return Msg.getMessageExpr(); if (const CXXConstructExpr *Ctor = CallE.dyn_cast()) return Ctor; diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 88a3be4139..3a4c15f0d2 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -32,6 +32,7 @@ class CallAndMessageChecker mutable OwningPtr BT_call_undef; mutable OwningPtr BT_call_arg; mutable OwningPtr BT_msg_undef; + mutable OwningPtr BT_objc_prop_undef; mutable OwningPtr BT_msg_arg; mutable OwningPtr BT_msg_ret; public: @@ -228,11 +229,21 @@ void CallAndMessageChecker::checkPreObjCMessage(ObjCMessage msg, SVal recVal = state->getSVal(receiver, LCtx); if (recVal.isUndef()) { if (ExplodedNode *N = C.generateSink()) { - if (!BT_msg_undef) - BT_msg_undef.reset(new BuiltinBug("Receiver in message expression is " - "an uninitialized value")); + BugType *BT = 0; + if (msg.isPureMessageExpr()) { + if (!BT_msg_undef) + BT_msg_undef.reset(new BuiltinBug("Receiver in message expression " + "is an uninitialized value")); + BT = BT_msg_undef.get(); + } + else { + if (!BT_objc_prop_undef) + BT_objc_prop_undef.reset(new BuiltinBug("Property access on an " + "uninitialized object pointer")); + BT = BT_objc_prop_undef.get(); + } BugReport *R = - new BugReport(*BT_msg_undef, BT_msg_undef->getName(), N); + new BugReport(*BT, BT->getName(), N); R->addRange(receiver->getSourceRange()); R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, receiver)); @@ -306,13 +317,13 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C, if (CanRetTy->isStructureOrClassType()) { // Structure returns are safe since the compiler zeroes them out. SVal V = C.getSValBuilder().makeZeroVal(msg.getType(Ctx)); - C.addTransition(state->BindExpr(msg.getOriginExpr(), LCtx, V)); + C.addTransition(state->BindExpr(msg.getMessageExpr(), LCtx, V)); return; } // Other cases: check if sizeof(return type) > sizeof(void*) if (CanRetTy != Ctx.VoidTy && C.getLocationContext()->getParentMap() - .isConsumedExpr(msg.getOriginExpr())) { + .isConsumedExpr(msg.getMessageExpr())) { // Compute: sizeof(void *) and sizeof(return type) const uint64_t voidPtrSize = Ctx.getTypeSize(Ctx.VoidPtrTy); const uint64_t returnTypeSize = Ctx.getTypeSize(CanRetTy); @@ -343,7 +354,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C, // of this case unless we have *a lot* more knowledge. // SVal V = C.getSValBuilder().makeZeroVal(msg.getType(Ctx)); - C.addTransition(state->BindExpr(msg.getOriginExpr(), LCtx, V)); + C.addTransition(state->BindExpr(msg.getMessageExpr(), LCtx, V)); return; } diff --git a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp index dbbab4891c..165dff5b10 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp @@ -196,7 +196,7 @@ void ObjCSelfInitChecker::checkPostObjCMessage(ObjCMessage msg, // value out when we return from this method. state = state->set(true); - SVal V = state->getSVal(msg.getOriginExpr(), C.getLocationContext()); + SVal V = state->getSVal(msg.getMessageExpr(), C.getLocationContext()); addSelfFlag(state, V, SelfFlag_InitRes, C); return; } diff --git a/lib/StaticAnalyzer/Core/CheckerManager.cpp b/lib/StaticAnalyzer/Core/CheckerManager.cpp index cb4da7cf4c..6f7a47fb8f 100644 --- a/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -190,8 +190,10 @@ namespace { NodeBuilder &Bldr, ExplodedNode *Pred) { ProgramPoint::Kind K = IsPreVisit ? ProgramPoint::PreStmtKind : ProgramPoint::PostStmtKind; - const ProgramPoint &L = ProgramPoint::getProgramPoint(Msg.getOriginExpr(), - K, Pred->getLocationContext(), checkFn.Checker); + const ProgramPoint &L = + ProgramPoint::getProgramPoint(Msg.getMessageExpr(), + K, Pred->getLocationContext(), + checkFn.Checker); CheckerContext C(Bldr, Eng, Pred, L); checkFn(Msg, C); diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp index 6f588c5ae1..34ad23a429 100644 --- a/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -808,11 +808,33 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, Bldr.addNodes(Dst); break; - case Stmt::ObjCMessageExprClass: + case Stmt::ObjCMessageExprClass: { Bldr.takeNodes(Pred); - VisitObjCMessage(cast(S), Pred, Dst); + // Is this a property access? + const ParentMap &PM = Pred->getLocationContext()->getParentMap(); + const ObjCMessageExpr *ME = cast(S); + bool evaluated = false; + + if (const PseudoObjectExpr *PO = + dyn_cast_or_null(PM.getParent(S))) { + const Expr *syntactic = PO->getSyntacticForm(); + if (const ObjCPropertyRefExpr *PR = + dyn_cast(syntactic)) { + bool isSetter = ME->getNumArgs() > 0; + VisitObjCMessage(ObjCMessage(ME, PR, isSetter), Pred, Dst); + evaluated = true; + } + else if (isa(syntactic)) { + VisitObjCMessage(ObjCMessage(ME, 0, true), Pred, Dst); + } + } + + if (!evaluated) + VisitObjCMessage(ME, Pred, Dst); + Bldr.addNodes(Dst); break; + } case Stmt::ObjCAtThrowStmtClass: { // FIXME: This is not complete. We basically treat @throw as @@ -1439,9 +1461,7 @@ void ExprEngine::evalStore(ExplodedNodeSet &Dst, const Expr *AssignE, const Expr *StoreE = AssignE ? AssignE : LocationE; if (isa(location)) { - loc::ObjCPropRef prop = cast(location); - return VisitObjCMessage(ObjCPropertySetter(prop.getPropRefExpr(), - StoreE, Val), Pred, Dst); + assert(false); } // Evaluate the location (checks for bad dereferences). @@ -1466,9 +1486,7 @@ void ExprEngine::evalLoad(ExplodedNodeSet &Dst, const Expr *Ex, assert(!isa(location) && "location cannot be a NonLoc."); if (isa(location)) { - loc::ObjCPropRef prop = cast(location); - return VisitObjCMessage(ObjCPropertyGetter(prop.getPropRefExpr(), Ex), - Pred, Dst); + assert(false); } // Are we loading from a region? This actually results in two loads; one diff --git a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index b5ceb64fab..72d03a1585 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -267,7 +267,7 @@ void ExprEngine::evalObjCMessage(StmtNodeBuilder &Bldr, state = invalidateArguments(state, CallOrObjCMessage(msg, state, LCtx), LCtx); // And create the new node. - Bldr.generateNode(msg.getOriginExpr(), Pred, state, GenSink); + Bldr.generateNode(msg.getMessageExpr(), Pred, state, GenSink); assert(Bldr.hasGeneratedNodes()); } diff --git a/lib/StaticAnalyzer/Core/ObjCMessage.cpp b/lib/StaticAnalyzer/Core/ObjCMessage.cpp index ac571a5ef6..65cdcd9d99 100644 --- a/lib/StaticAnalyzer/Core/ObjCMessage.cpp +++ b/lib/StaticAnalyzer/Core/ObjCMessage.cpp @@ -18,104 +18,11 @@ using namespace clang; using namespace ento; -QualType ObjCMessage::getType(ASTContext &ctx) const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getType(); - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - if (isPropertySetter()) - return ctx.VoidTy; - return propE->getType(); -} - -Selector ObjCMessage::getSelector() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getSelector(); - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - if (isPropertySetter()) - return propE->getSetterSelector(); - return propE->getGetterSelector(); -} - -ObjCMethodFamily ObjCMessage::getMethodFamily() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - // Case 1. Explicit message send. - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getMethodFamily(); - - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - - // Case 2. Reference to implicit property. - if (propE->isImplicitProperty()) { - if (isPropertySetter()) - return propE->getImplicitPropertySetter()->getMethodFamily(); - else - return propE->getImplicitPropertyGetter()->getMethodFamily(); - } - - // Case 3. Reference to explicit property. - const ObjCPropertyDecl *prop = propE->getExplicitProperty(); - if (isPropertySetter()) { - if (prop->getSetterMethodDecl()) - return prop->getSetterMethodDecl()->getMethodFamily(); - return prop->getSetterName().getMethodFamily(); - } else { - if (prop->getGetterMethodDecl()) - return prop->getGetterMethodDecl()->getMethodFamily(); - return prop->getGetterName().getMethodFamily(); - } -} - -const ObjCMethodDecl *ObjCMessage::getMethodDecl() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getMethodDecl(); - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - if (propE->isImplicitProperty()) - return isPropertySetter() ? propE->getImplicitPropertySetter() - : propE->getImplicitPropertyGetter(); - return 0; -} - -const ObjCInterfaceDecl *ObjCMessage::getReceiverInterface() const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getReceiverInterface(); - const ObjCPropertyRefExpr *propE = cast(MsgOrPropE); - if (propE->isClassReceiver()) - return propE->getClassReceiver(); - QualType recT; - if (const Expr *recE = getInstanceReceiver()) - recT = recE->getType(); - else { - assert(propE->isSuperReceiver()); - recT = propE->getSuperReceiverType(); - } - if (const ObjCObjectPointerType *Ptr = recT->getAs()) - return Ptr->getInterfaceDecl(); - return 0; -} - -const Expr *ObjCMessage::getArgExpr(unsigned i) const { - assert(isValid() && "This ObjCMessage is uninitialized!"); - assert(i < getNumArgs() && "Invalid index for argument"); - if (const ObjCMessageExpr *msgE = dyn_cast(MsgOrPropE)) - return msgE->getArg(i); - assert(isPropertySetter()); - if (const BinaryOperator *bop = dyn_cast(OriginE)) - if (bop->isAssignmentOp()) - return bop->getRHS(); - return 0; -} - QualType CallOrObjCMessage::getResultType(ASTContext &ctx) const { QualType resultTy; bool isLVal = false; if (isObjCMessage()) { - isLVal = isa(Msg.getOriginExpr()) && - Msg.getOriginExpr()->isLValue(); resultTy = Msg.getResultType(ctx); } else if (const CXXConstructExpr *Ctor = CallE.dyn_cast()) { diff --git a/test/Analysis/properties.m b/test/Analysis/properties.m index 6d04a4ab4e..231923b9ea 100644 --- a/test/Analysis/properties.m +++ b/test/Analysis/properties.m @@ -143,3 +143,26 @@ void rdar6611873() { return super.name; } @end + +// Static analyzer doesn't detect uninitialized variable issues for property accesses +@interface RDar9241180 +@property (readwrite,assign) id x; +-(id)testAnalyzer1:(int) y; +-(void)testAnalyzer2; +@end + +@implementation RDar9241180 +@synthesize x; +-(id)testAnalyzer1:(int)y { + RDar9241180 *o; + if (y && o.x) // expected-warning {{Property access on an uninitialized object pointer}} + return o; + return o; // expected-warning {{Undefined or garbage value returned to caller}} +} +-(void)testAnalyzer2 { + id y; + self.x = y; // expected-warning {{Argument for property setter is an uninitialized value}} +} +@end + + -- 2.40.0