[analyzer] operator new: Model the cast of returned pointer into object type.

According to [basic.stc.dynamic.allocation], the return type of any C++
overloaded operator new() is "void *". However, type of the new-expression
"new T()" and the type of "this" during construction of "T" are both "T *".

Hence an implicit cast, which is not present in the AST, needs to be performed
before the construction. This patch adds such cast in the case when the
allocator was indeed inlined. For now, in the case where the allocator was *not*
inlined we still use the same symbolic value (which is a pure SymbolicRegion of
type "T *") because it is consistent with how we represent the casts and causes
less surprise in the checkers after switching to the new behavior.

The better approach would be to represent that value as a cast over a
SymbolicRegion of type "void *", however we have technical difficulties
conjuring such region without any actual expression of type "void *" present in
the AST.

Differential Revision: https://reviews.llvm.org/D41250
rdar://problem/12180598

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@322777 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index ec2e2e42c6f6ca5a1294b6ee1e24530e071b2c08..2c15607c7966cd337d2b2ea87250a5e3cab53659 100644 (file)
--- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -119,9 +119,17 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
         if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
           // TODO: Detect when the allocator returns a null pointer.
           // Constructor shall not be called in this case.
-          if (const MemRegion *MR =
-                  getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())
+          if (const SubRegion *MR = dyn_cast_or_null<SubRegion>(
+                  getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())) {
+            if (CNE->isArray()) {
+              // TODO: This code exists only to trigger the suppression for
+              // array constructors. In fact, we need to call the constructor
+              // for every allocated element, not just the first one!
+              return getStoreManager().GetElementZeroRegion(
+                  MR, CNE->getType()->getPointeeType());
+            }
             return MR;
+          }
         }
       } else if (auto *DS = dyn_cast<DeclStmt>(StmtElem->getStmt())) {
         if (const auto *Var = dyn_cast<VarDecl>(DS->getSingleDecl())) {
@@ -473,12 +481,22 @@ void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
   for (auto I : DstPreCall)
     defaultEvalCall(CallBldr, I, *Call);
+  // If the call is inlined, DstPostCall will be empty and we bail out now.
 
   // Store return value of operator new() for future use, until the actual
   // CXXNewExpr gets processed.
   ExplodedNodeSet DstPostValue;
   StmtNodeBuilder ValueBldr(DstPostCall, DstPostValue, *currBldrCtx);
   for (auto I : DstPostCall) {
+    // FIXME: Because CNE serves as the "call site" for the allocator (due to
+    // lack of a better expression in the AST), the conjured return value symbol
+    // is going to be of the same type (C++ object pointer type). Technically
+    // this is not correct because the operator new's prototype always says that
+    // it returns a 'void *'. So we should change the type of the symbol,
+    // and then evaluate the cast over the symbolic pointer from 'void *' to
+    // the object pointer type. But without changing the symbol's type it
+    // is breaking too much to evaluate the no-op symbolic cast over it, so we
+    // skip it for now.
     ProgramStateRef State = I->getState();
     ValueBldr.generateNode(
         CNE, I,
@@ -564,16 +582,20 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
 
   StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
 
+  SVal Result = symVal;
+
   if (CNE->isArray()) {
     // FIXME: allocating an array requires simulating the constructors.
     // For now, just return a symbolicated region.
-    const SubRegion *NewReg =
-        symVal.castAs<loc::MemRegionVal>().getRegionAs<SubRegion>();
-    QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType();
-    const ElementRegion *EleReg =
-      getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
-    State = State->BindExpr(CNE, Pred->getLocationContext(),
-                            loc::MemRegionVal(EleReg));
+    if (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
+      const SubRegion *NewReg =
+          symVal.castAs<loc::MemRegionVal>().getRegionAs<SubRegion>();
+      QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType();
+      const ElementRegion *EleReg =
+          getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
+      Result = loc::MemRegionVal(EleReg);
+    }
+    State = State->BindExpr(CNE, Pred->getLocationContext(), Result);
     Bldr.generateNode(CNE, Pred, State);
     return;
   }
@@ -582,7 +604,6 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
   // 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.)
-  SVal Result = symVal;
   if (FD && FD->isReservedGlobalPlacementOperator()) {
     // Non-array placement new should always return the placement location.
     SVal PlacementLoc = State->getSVal(CNE->getPlacementArg(0), LCtx);

diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
index 2cd169ee6dc821914cde7c9118d25a69c127e148..bb05acd1f416ba03cc41cf7e8781b15650315cdc 100644 (file)
--- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -277,12 +277,19 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
       state = state->BindExpr(CCE, callerCtx, ThisV);
     }
 
-    if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(CE)) {
+    if (const auto *CNE = dyn_cast<CXXNewExpr>(CE)) {
       // We are currently evaluating a CXXNewAllocator CFGElement. It takes a
       // while to reach the actual CXXNewExpr element from here, so keep the
       // region for later use.
-      state = setCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(),
-                                      state->getSVal(CE, callerCtx));
+      // Additionally cast the return value of the inlined operator new
+      // (which is of type 'void *') to the correct object type.
+      SVal AllocV = state->getSVal(CNE, callerCtx);
+      AllocV = svalBuilder.evalCast(
+          AllocV, CNE->getType(),
+          getContext().getPointerType(getContext().VoidTy));
+
+      state =
+          setCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(), AllocV);
     }
   }
 

diff --git a/test/Analysis/new-ctor-conservative.cpp b/test/Analysis/new-ctor-conservative.cpp
index ce0ad464656f10e3b34a6542a05f7af029bd1136..4500e3a253d12020a60f173cab6e82efef26dab7 100644 (file)
--- a/test/Analysis/new-ctor-conservative.cpp
+++ b/test/Analysis/new-ctor-conservative.cpp
@@ -12,3 +12,18 @@ void checkConstructorInlining() {
   S *s = new S;
   clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}}
 }
+
+void checkNewPOD() {
+  int *i = new int;
+  clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}}
+  int *j = new int();
+  clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}}
+  int *k = new int(5);
+  clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
+}
+
+void checkNewArray() {
+  S *s = new S[10];
+  // FIXME: Should be true once we inline array constructors.
+  clang_analyzer_eval(s[0].x == 1); // expected-warning{{UNKNOWN}}
+}

diff --git a/test/Analysis/new-ctor-inlined.cpp b/test/Analysis/new-ctor-inlined.cpp
index d16b9b1d287ab53b6f98265cda658bca10ea99cd..1506bf27f353a55cddb40c0605af38074bd7239b 100644 (file)
--- a/test/Analysis/new-ctor-inlined.cpp
+++ b/test/Analysis/new-ctor-inlined.cpp
@@ -38,3 +38,18 @@ void checkNestedNew() {
   Sp *p = new Sp(new Sp(0));
   clang_analyzer_eval(p->p->p == 0); // expected-warning{{TRUE}}
 }
+
+void checkNewPOD() {
+  int *i = new int;
+  clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}}
+  int *j = new int();
+  clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}}
+  int *k = new int(5);
+  clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
+}
+
+void checkTrivialCopy() {
+  S s;
+  S *t = new S(s); // no-crash
+  clang_analyzer_eval(t->x == 1); // expected-warning{{TRUE}}
+}