On Jul 2, 2012, at 3:21 PM, Jordan Rose wrote: > Author: jrose > Date: Mon Jul 2 17:21:47 2012 > New Revision: 159608 > > URL: http://llvm.org/viewvc/llvm-project?rev=159608&view=rev > Log: > [analyzer] Introduce CXXAllocatorCall to handle placement arg invalidation. > > This is NOT full-blown support for operator new, but removes some nasty > duplicated code introduced in r158784. > > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h > cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp > cfe/trunk/test/Analysis/new.cpp > > Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h?rev=159608&r1=159607&r2=159608&view=diff > ============================================================================== > --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h > (original) > +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Calls.h Mon Jul > 2 17:21:47 2012 > @@ -33,8 +33,9 @@ > CE_BEG_SIMPLE_CALLS = CE_Function, > CE_END_SIMPLE_CALLS = CE_Block, > CE_CXXConstructor, > + CE_CXXAllocator, > CE_BEG_FUNCTION_CALLS = CE_Function, > - CE_END_FUNCTION_CALLS = CE_CXXConstructor, > + CE_END_FUNCTION_CALLS = CE_CXXAllocator, > CE_ObjCMessage, > CE_ObjCPropertyAccess, > CE_BEG_OBJC_CALLS = CE_ObjCMessage, > @@ -337,6 +338,35 @@ > } > }; > > +class CXXAllocatorCall : public AnyFunctionCall { > + const CXXNewExpr *E; > + > +public: > + CXXAllocatorCall(const CXXNewExpr *e, ProgramStateRef St, > + const LocationContext *LCtx) > + : AnyFunctionCall(St, LCtx, CE_CXXAllocator), E(e) {} > + > + const CXXNewExpr *getOriginExpr() const { return E; } > + SourceRange getSourceRange() const { return E->getSourceRange(); } > + > + const FunctionDecl *getDecl() const { > + return E->getOperatorNew(); > + } > + > + unsigned getNumArgs() const { return E->getNumPlacementArgs() + 1; } > + > + const Expr *getArgExpr(unsigned Index) const { > + // The first argument of an allocator call is the size of the allocation. > + if (Index == 0) > + return 0; > + return E->getPlacementArg(Index - 1); > + } > + > + static bool classof(const CallEvent *CE) { > + return CE->getKind() == CE_CXXAllocator; > + } > +}; > + > /// \brief Represents any expression that calls an Objective-C method. > class ObjCMethodCall : public CallEvent { > const ObjCMessageExpr *Msg; > > Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp?rev=159608&r1=159607&r2=159608&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Mon Jul 2 > 17:21:47 2012 > @@ -947,6 +947,7 @@ > case CE_CXXMember: > case CE_Block: > case CE_CXXConstructor: > + case CE_CXXAllocator: > // FIXME: These calls are currently unsupported. > return getPersistentStopSummary(); > case CE_ObjCMessage: > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp?rev=159608&r1=159607&r2=159608&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Mon Jul 2 17:21:47 > 2012 > @@ -92,191 +92,51 @@ > Bldr.generateNode(PP, Pred, state); > } > > -static bool isPointerToConst(const ParmVarDecl *ParamDecl) { > - // FIXME: Copied from ExprEngineCallAndReturn.cpp > - QualType PointeeTy = ParamDecl->getOriginalType()->getPointeeType(); > - if (PointeeTy != QualType() && PointeeTy.isConstQualified() && > - !PointeeTy->isAnyPointerType() && !PointeeTy->isReferenceType()) { > - return true; > - } > - return false; > -} > - > void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred, > ExplodedNodeSet &Dst) { > + // FIXME: Much of this should eventually migrate to CXXAllocatorCall. > + // Also, we need to decide how allocators actually work -- they're not > + // really part of the CXXNewExpr because they happen BEFORE the > + // CXXConstructExpr subexpression. See PR12014 for some discussion. > StmtNodeBuilder Bldr(Pred, Dst, *currentBuilderContext); > > unsigned blockCount = currentBuilderContext->getCurrentBlockCount(); > const LocationContext *LCtx = Pred->getLocationContext(); > DefinedOrUnknownSVal symVal = > - svalBuilder.getConjuredSymbolVal(NULL, CNE, LCtx, CNE->getType(), > blockCount); > - const MemRegion *NewReg = cast<loc::MemRegionVal>(symVal).getRegion(); > - QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType(); > - const ElementRegion *EleReg = > - getStoreManager().GetElementZeroRegion(NewReg, ObjTy); > + svalBuilder.getConjuredSymbolVal(0, CNE, LCtx, CNE->getType(), > blockCount); > ProgramStateRef State = Pred->getState(); > > + // Invalidate placement args. > + CXXAllocatorCall Call(CNE, State, LCtx); > + State = Call.invalidateRegions(blockCount); > + > if (CNE->isArray()) { > // FIXME: allocating an array requires simulating the constructors. > // For now, just return a symbolicated region. > + const MemRegion *NewReg = cast<loc::MemRegionVal>(symVal).getRegion(); > + QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType(); > + const ElementRegion *EleReg = > + getStoreManager().GetElementZeroRegion(NewReg, ObjTy); > State = State->BindExpr(CNE, Pred->getLocationContext(), > loc::MemRegionVal(EleReg)); > Bldr.generateNode(CNE, Pred, State); > return; > } > > + // FIXME: Once we have proper support for CXXConstructExprs inside > + // CXXNewExpr, we need to make sure that the constructed object is not > + // immediately invalidated here. (The placement call should happen before > + // the constructor call anyway.) > FunctionDecl *FD = CNE->getOperatorNew(); > if (FD && FD->isReservedGlobalPlacementOperator()) { > // Non-array placement new should always return the placement location. > SVal PlacementLoc = State->getSVal(CNE->getPlacementArg(0), LCtx); > State = State->BindExpr(CNE, LCtx, PlacementLoc); > - // FIXME: Once we have proper support for CXXConstructExprs inside > - // CXXNewExpr, we need to make sure that the constructed object is not > - // immediately invalidated here. (The placement call should happen before > - // the constructor call anyway.) > + } else { > + State = State->BindExpr(CNE, LCtx, symVal); > } > > - // Invalidate placement args. > - > - // FIXME: This is largely copied from invalidateArguments, because > - // CallOrObjCMessage is not general enough to handle new-expressions yet. > - SmallVector<const MemRegion *, 4> RegionsToInvalidate; > - > - unsigned Index = 0; > - for (CXXNewExpr::const_arg_iterator I = CNE->placement_arg_begin(), > - E = CNE->placement_arg_end(); > - I != E; ++I) { > - // Pre-increment the argument index to skip over the implicit size arg. > - ++Index; > - if (FD && Index < FD->getNumParams()) > - if (isPointerToConst(FD->getParamDecl(Index))) > - continue; > - > - SVal V = State->getSVal(*I, LCtx); > - > - // If we are passing a location wrapped as an integer, unwrap it and > - // invalidate the values referred by the location. > - if (nonloc::LocAsInteger *Wrapped = dyn_cast<nonloc::LocAsInteger>(&V)) > - V = Wrapped->getLoc(); > - else if (!isa<Loc>(V)) > - continue; > - > - if (const MemRegion *R = V.getAsRegion()) { > - // Invalidate the value of the variable passed by reference. > - > - // Are we dealing with an ElementRegion? If the element type is > - // a basic integer type (e.g., char, int) and the underlying region > - // is a variable region then strip off the ElementRegion. > - // FIXME: We really need to think about this for the general case > - // as sometimes we are reasoning about arrays and other times > - // about (char*), etc., is just a form of passing raw bytes. > - // e.g., void *p = alloca(); foo((char*)p); > - if (const ElementRegion *ER = dyn_cast<ElementRegion>(R)) { > - // Checking for 'integral type' is probably too promiscuous, but > - // we'll leave it in for now until we have a systematic way of > - // handling all of these cases. Eventually we need to come up > - // with an interface to StoreManager so that this logic can be > - // appropriately delegated to the respective StoreManagers while > - // still allowing us to do checker-specific logic (e.g., > - // invalidating reference counts), probably via callbacks. > - if (ER->getElementType()->isIntegralOrEnumerationType()) { > - const MemRegion *superReg = ER->getSuperRegion(); > - if (isa<VarRegion>(superReg) || isa<FieldRegion>(superReg) || > - isa<ObjCIvarRegion>(superReg)) > - R = cast<TypedRegion>(superReg); > - } > - // FIXME: What about layers of ElementRegions? > - } > - > - // Mark this region for invalidation. We batch invalidate regions > - // below for efficiency. > - RegionsToInvalidate.push_back(R); > - } else { > - // Nuke all other arguments passed by reference. > - // FIXME: is this necessary or correct? This handles the non-Region > - // cases. Is it ever valid to store to these? > - State = State->unbindLoc(cast<Loc>(V)); > - } > - } > - > - // Invalidate designated regions using the batch invalidation API. > - > - // FIXME: We can have collisions on the conjured symbol if the > - // expression *I also creates conjured symbols. We probably want > - // to identify conjured symbols by an expression pair: the enclosing > - // expression (the context) and the expression itself. This should > - // disambiguate conjured symbols. > - unsigned Count = currentBuilderContext->getCurrentBlockCount(); > - > - // NOTE: Even if RegionsToInvalidate is empty, we may still invalidate > - // global variables. > - State = State->invalidateRegions(RegionsToInvalidate, CNE, Count, LCtx); > Bldr.generateNode(CNE, Pred, State); > - return; > - > - // FIXME: The below code is long-since dead. However, constructor handling > - // in new-expressions is far from complete. See PR12014 for more details. > -#if 0 > - // Evaluate constructor arguments. > - const FunctionProtoType *FnType = NULL; > - const CXXConstructorDecl *CD = CNE->getConstructor(); > - if (CD) > - FnType = CD->getType()->getAs<FunctionProtoType>(); > - ExplodedNodeSet argsEvaluated; > - Bldr.takeNodes(Pred); > - evalArguments(CNE->constructor_arg_begin(), CNE->constructor_arg_end(), > - FnType, Pred, argsEvaluated); > - Bldr.addNodes(argsEvaluated); > - > - // Initialize the object region and bind the 'new' expression. > - for (ExplodedNodeSet::iterator I = argsEvaluated.begin(), > - E = argsEvaluated.end(); I != E; ++I) { > - > - ProgramStateRef state = (*I)->getState(); > - > - // Accumulate list of regions that are invalidated. > - // FIXME: Eventually we should unify the logic for constructor > - // processing in one place. > - SmallVector<const MemRegion*, 10> regionsToInvalidate; > - for (CXXNewExpr::const_arg_iterator > - ai = CNE->constructor_arg_begin(), ae = CNE->constructor_arg_end(); > - ai != ae; ++ai) > - { > - SVal val = state->getSVal(*ai, (*I)->getLocationContext()); > - if (const MemRegion *region = val.getAsRegion()) > - regionsToInvalidate.push_back(region); > - } > - > - if (ObjTy->isRecordType()) { > - regionsToInvalidate.push_back(EleReg); > - // Invalidate the regions. > - // TODO: Pass the call to new information as the last argument, to > limit > - // the globals which will get invalidated. > - state = state->invalidateRegions(regionsToInvalidate, > - CNE, blockCount, 0, 0); > - > - } else { > - // Invalidate the regions. > - // TODO: Pass the call to new information as the last argument, to > limit > - // the globals which will get invalidated. > - state = state->invalidateRegions(regionsToInvalidate, > - CNE, blockCount, 0, 0); > - > - if (CNE->hasInitializer()) { > - SVal V = state->getSVal(*CNE->constructor_arg_begin(), > - (*I)->getLocationContext()); > - state = state->bindLoc(loc::MemRegionVal(EleReg), V); > - } else { > - // Explicitly set to undefined, because currently we retrieve > symbolic > - // value from symbolic region. > - state = state->bindLoc(loc::MemRegionVal(EleReg), UndefinedVal()); > - } > - } > - state = state->BindExpr(CNE, (*I)->getLocationContext(), > - loc::MemRegionVal(EleReg)); > - Bldr.generateNode(CNE, *I, state); > - } > -#endif > } > > void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE, > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp?rev=159608&r1=159607&r2=159608&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp Mon Jul 2 > 17:21:47 2012 > @@ -240,11 +240,6 @@ > return false; > } > > - // Do not inline constructors until we can model destructors. > - // This is unfortunate, but basically necessary for smart pointers and > such. > - if (isa<CXXConstructorDecl>(D)) > - return false; > - > // It is possible that the live variables analysis cannot be > // run. If so, bail out. > if (!CalleeADC->getAnalysis<RelaxedLiveVariables>()) > @@ -267,10 +262,17 @@ > > switch (Call.getKind()) { > case CE_Function: > - case CE_CXXConstructor: > case CE_CXXMember: > // These are always at least possible to inline. > break; > + case CE_CXXConstructor: > + // Do not inline constructors until we can model destructors. > + // This is unfortunate, but basically necessary for smart pointers and > such. > + return false; > + case CE_CXXAllocator: > + // Do not inline allocators until we model deallocators. > + // This is unfortunate, but basically necessary for smart pointers and > such. > + return false; > case CE_Block: { > const BlockDataRegion *BR = cast<BlockCall>(Call).getBlockRegion(); > if (!BR)
Not sure where this switch was added, but we seem to rely on it to determine if we should inline the call or not. We have at least 2 other methods that help us to decide that: CallEvent::mayBeInlined() and shouldInlineDecl() called below. Can we try and refactor this logic into one of those? > > Modified: cfe/trunk/test/Analysis/new.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/new.cpp?rev=159608&r1=159607&r2=159608&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/new.cpp (original) > +++ cfe/trunk/test/Analysis/new.cpp Mon Jul 2 17:21:47 2012 > @@ -49,6 +49,16 @@ > return y; // no-warning > } > > +void *operator new(size_t, void *, void *); > +void *testCustomNewMalloc() { > + int *x = (int *)malloc(sizeof(int)); > + > + // Should be no-warning (the custom allocator could have freed x). > + void *y = new (0, x) int; // no-warning > + > + return y; > +} > + > > //-------------------------------- > // Incorrectly-modelled behavior > @@ -69,14 +79,3 @@ > clang_analyzer_eval(*n == 3); // expected-warning{{UNKNOWN}} > } > > - > -void *operator new(size_t, void *, void *); > -void *testCustomNewMalloc() { > - int *x = (int *)malloc(sizeof(int)); > - > - // Should be no-warning (the custom allocator could have freed x). > - void *y = new (0, x) int; // expected-warning{{leak of memory pointed to > by 'x'}} > - > - return y; > -} > - > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
