[analyzer] Model implied cast around operator new().

[NoQ]

Summary

C++ overridable operator new() has the following prototype:

void *operator new(size_t size, user-defined arguments...);

The return value is void *. However, before passing it to constructor, we need to make a cast to the respective object pointer type. Hence an implicit cast is present here, which is not represented in the current AST or CFG. Modeling this cast is straightforward though. This is the change i mentioned in D40939.

I also noticed that evalCast from void * to T * is uncomfortable to use because sometimes it transforms &SymRegion{$x} into &element{T, 0S32b, SymRegion{$x}} even when $x is already of type T *. The form &SymRegion{$x} seems to be the canonical form of this symbolic pointer value in the rest of the analyzer, so i decided to change evalCast to preserve it.

The problem of how to represent memregion value casts better still stands - it wouldn't add much to the analyzer's quality, but we just keep running into it over and over again.

[NoQ]

VisitCXXNewExpr is too late. We need to perform cast before calling the constructor. Otherwise bad things happen, for instance performTrivialCopy would construct another void region :)

Move the cast to pushCXXNewAllocatorValue(). This way we perform the cast before putting this-value into our temporary storage (the top of CXXNewAllocatorValueStack, or _this in terms of http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html), which seems correct. And this affects all two code paths on which we exit the allocator call - both the conservativeEvalCall path and the processCallExit path (and ideally the future evalCall path).

[xazax.hun]

The code looks good to me. But the best way to verify these kinds of changes to see how the results change on large projects after applying the patch.

[NoQ]

Rebase.

I also noticed that evalCast from void * to T * is uncomfortable to use because sometimes it transforms &SymRegion{$x} into &element{T, 0S32b, SymRegion{$x}} even when $x is already of type T *. The form &SymRegion{$x} seems to be the canonical form of this symbolic pointer value in the rest of the analyzer, so i decided to change evalCast to preserve it.

Suddenly it turns out that this is not needed anymore. I'm struggling quite a bit to get the casts right, and still failing to identify the actual system we're trying to follow when representing pointer casts. I'd love to get to the bottom of it eventually.

[NoQ]

In D41250#959755, @NoQ wrote:

I also noticed that evalCast from void * to T * is uncomfortable to use because sometimes it transforms &SymRegion{$x} into &element{T, 0S32b, SymRegion{$x}} even when $x is already of type T *. The form &SymRegion{$x} seems to be the canonical form of this symbolic pointer value in the rest of the analyzer, so i decided to change evalCast to preserve it.

Suddenly it turns out that this is not needed anymore. I'm struggling quite a bit to get the casts right, and still failing to identify the actual system we're trying to follow when representing pointer casts. I'd love to get to the bottom of it eventually.

Model the cast only around allocators that were inlined. Additionally, produce array element for array new[] allocator calls. This completely reverts the rather-accidental-than-intended change in behavior in the conservative case which i described above: we no longer get this

&element{T, 0S32b, SymRegion{$x}} even when $x is already of type T *

thing in the conservative case, while still behaving reasonably in the inlined case, without touching any other behavior of the analyzer (i.e. not touching the whole evalCast thing). So i believe this is the right thing to do, at least for this patch.

Eventually it might be better to return &element{T, 0S32b, SymRegion{$x}} where $x is a conjured void pointer - this would express the semantics and the origins of that SVal better. However, we cannot do that, because we cannot conjure a symbol without a void-pointer-type expression, and we don't have the expression that represents the call site for the allocator call.

[NoQ]

Rebase.

Add a FIXME to bring back the cast in the conservative case.

[dcoughlin]

This looks good to me, as well.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
   // element in the CFG.
 
   if (auto Elem = findElementDirectlyInitializedByCurrentConstructor()) {
     if (Optional<CFGStmt> StmtElem = Elem->getAs<CFGStmt>()) {
       if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(StmtElem->getStmt())) {
         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())) {
           if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) {
             SVal LValue = State->getLValue(Var, LCtx);
             QualType Ty = Var->getType();
             LValue = makeZeroElementRegion(State, LValue, Ty);
             return LValue.getAsRegion();
           }
void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
   ExplodedNodeSet DstPreCall;
   getCheckerManager().runCheckersForPreCall(DstPreCall, Pred,
                                             *Call, *this);
 
   ExplodedNodeSet DstPostCall;
   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,
         setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
   }
 
   getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
                                              *Call, *this);
if (FD) {
     if (const FunctionProtoType *ProtoType = Ty->getAs<FunctionProtoType>())
       if (!ProtoType->isNothrow(getContext()))
         if (auto dSymVal = symVal.getAs<DefinedOrUnknownSVal>())
           State = State->assume(*dSymVal, true);
   }
 
   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.
+    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);
-    State = State->BindExpr(CNE, Pred->getLocationContext(),
-                            loc::MemRegionVal(EleReg));
+      Result = loc::MemRegionVal(EleReg);
+    }
+    State = State->BindExpr(CNE, Pred->getLocationContext(), Result);
     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.)
-  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);
     Result = svalBuilder.evalCast(PlacementLoc, CNE->getType(),
                                   CNE->getPlacementArg(0)->getType());
   }
 
   // Bind the address of the object, then check to see if we cached out.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (const CXXConstructExpr *CCE = dyn_cast<CXXConstructExpr>(CE)) {
 
       // If the constructed object is a temporary prvalue, get its bindings.
       if (isTemporaryPRValue(CCE, ThisV))
         ThisV = state->getSVal(ThisV.castAs<Loc>());
 
       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);
     }
   }
 
   // Step 3: BindedRetNode -> CleanedNodes
   // If we can find a statement and a block in the inlined function, run remove
   // dead bindings before returning from the call. This is important to ensure
   // that we report the issues such as leaks in the stack contexts in which
   // they occurred.

test/Analysis/new-ctor-conservative.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
 };
 
 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}}
+}

test/Analysis/new-ctor-inlined.cpp

struct Sp {
   Sp(Sp *p): p(p) {}
   ~Sp() {}
 };
 
 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}}
+}