On Jun 19, 2012, at 6:32 PM, Jordan Rose wrote: > Author: jrose > Date: Tue Jun 19 20:32:01 2012 > New Revision: 158784 > > URL: http://llvm.org/viewvc/llvm-project?rev=158784&view=rev > Log: > [analyzer] Invalidate placement args; return the pointer given to placement > new > > The default global placement new just returns the pointer it is given. > Note that other custom 'new' implementations with placement args are not > guaranteed to do this. > > In addition, we need to invalidate placement args, since they may be updated > by > the allocator function. (Also, right now we don't properly handle the > constructor inside a CXXNewExpr, so we need to invalidate the placement args > just so that callers know something changed!) > > This invalidation is not perfect because CallOrObjCMessage doesn't support > CXXNewExpr, and all of our invalidation callbacks expect that if there's no > CallOrObjCMessage, the invalidation is happening manually (e.g. by a direct > assignment) and shouldn't affect checker-specific metadata (like malloc > state); > hence the malloc test case in new-fail.cpp. But region values are now > properly invalidated, at least. >
Can we extend CallOrObjCMessage with the CXXNewExpr? I know you think CallOrObjCMessage should be rewritten, but it's a much better solution than copying a bunch of dense code including 4 original FIXMEs and still not having the complete invalidation as the result. Also, what do we win from adding the invalidation? All the false positives seemed to have moved into a test which is XFAILED. If we don't win anything and it's too hard to reuse CallOrObjCMessage, we could just not do any invalidation (you already have a fixit notes for it). Also, I think XFAIL stands for expected fail; I am not sure if it should be used for a TODO list (might be wrong though..). You could just leave the tests where they were (new.cpp) and add a note. Cheers, Anna. > The long-term solution to this problem is to rework CallOrObjCMessage into > something more general, rather than the morass of branches it is today. > > <rdar://problem/11679031> > > Added: > cfe/trunk/test/Analysis/new-fail.cpp > Modified: > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp > cfe/trunk/test/Analysis/new.cpp > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp?rev=158784&r1=158783&r2=158784&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Tue Jun 19 20:32:01 > 2012 > @@ -170,6 +170,16 @@ > 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) { > StmtNodeBuilder Bldr(Pred, Dst, *currentBuilderContext); > @@ -182,18 +192,108 @@ > QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType(); > const ElementRegion *EleReg = > getStoreManager().GetElementZeroRegion(NewReg, ObjTy); > + ProgramStateRef State = Pred->getState(); > > if (CNE->isArray()) { > // FIXME: allocating an array requires simulating the constructors. > // For now, just return a symbolicated region. > - ProgramStateRef state = Pred->getState(); > - state = state->BindExpr(CNE, Pred->getLocationContext(), > + State = State->BindExpr(CNE, Pred->getLocationContext(), > loc::MemRegionVal(EleReg)); > - Bldr.generateNode(CNE, Pred, state); > + Bldr.generateNode(CNE, Pred, State); > return; > } > > - // FIXME: Update for AST changes. > + 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.) > + } > + > + // 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; > > Added: cfe/trunk/test/Analysis/new-fail.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/new-fail.cpp?rev=158784&view=auto > ============================================================================== > --- cfe/trunk/test/Analysis/new-fail.cpp (added) > +++ cfe/trunk/test/Analysis/new-fail.cpp Tue Jun 19 20:32:01 2012 > @@ -0,0 +1,21 @@ > +// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc > -analyzer-store region -verify %s > +// XFAIL: * > + > +void f1() { > + int *n = new int; > + if (*n) { // expected-warning {{Branch condition evaluates to a garbage > value}} > + } > +} > + > +void f2() { > + int *n = new int(3); > + if (*n) { // no-warning > + } > +} > + > +void *operator new(size_t, void *, void *); > +void *testCustomNew() { > + int *x = (int *)malloc(sizeof(int)); > + void *y = new (0, x) int; > + return y; // no-warning (placement new could have freed x) > +} > > Modified: cfe/trunk/test/Analysis/new.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/new.cpp?rev=158784&r1=158783&r2=158784&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/new.cpp (original) > +++ cfe/trunk/test/Analysis/new.cpp Tue Jun 19 20:32:01 2012 > @@ -1,15 +1,36 @@ > -// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-store region > -verify %s > -// XFAIL: * > +// RUN: %clang_cc1 -analyze > -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-store > region -verify %s > > -void f1() { > - int *n = new int; > - if (*n) { // expected-warning {{Branch condition evaluates to a garbage > value}} > - } > +void clang_analyzer_eval(bool); > + > +typedef typeof(sizeof(int)) size_t; > +extern "C" void *malloc(size_t); > + > +// This is the standard placement new. > +inline void* operator new(size_t, void* __p) throw() > +{ > + return __p; > +} > + > +void *testPlacementNew() { > + int *x = (int *)malloc(sizeof(int)); > + *x = 1; > + clang_analyzer_eval(*x == 1); // expected-warning{{TRUE}}; > + > + void *y = new (x) int; > + clang_analyzer_eval(x == y); // expected-warning{{TRUE}}; > + clang_analyzer_eval(*x == 1); // expected-warning{{UNKNOWN}}; > + > + return y; > } > > -void f2() { > - int *n = new int(3); > - if (*n) { // no-warning > - } > +void *operator new(size_t, size_t, int *); > +void *testCustomNew() { > + int x[1] = {1}; > + clang_analyzer_eval(*x == 1); // expected-warning{{TRUE}}; > + > + void *y = new (0, x) int; > + clang_analyzer_eval(*x == 1); // expected-warning{{UNKNOWN}}; > + > + return y; // no-warning > } > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
