[cfe-commits] r158784 - in /cfe/trunk: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp test/Analysis/new-fail.cpp test/Analysis/new.cpp
Anna Zaks
ganna at apple.com
Tue Jun 19 22:07:08 PDT 2012
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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list