[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

void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
   // 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());
+  } else {
+    Result =
+        svalBuilder.evalCast(Result, CNE->getType(),
+                             getContext().getPointerType(getContext().VoidTy));
   }
 
   // Bind the address of the object, then check to see if we cached out.
   State = State->BindExpr(CNE, LCtx, Result);
   ExplodedNode *NewN = Bldr.generateNode(CNE, Pred, State);
   if (!NewN)
     return;
 

lib/StaticAnalyzer/Core/Store.cpp

const MemRegion *StoreManager::castRegion(const MemRegion *R, QualType CastToTy) {
   QualType PointeeTy = CastToTy->getPointeeType();
   QualType CanonPointeeTy = Ctx.getCanonicalType(PointeeTy);
 
   // Handle casts to void*.  We just pass the region through.
   if (CanonPointeeTy.getLocalUnqualifiedType() == Ctx.VoidTy)
     return R;
 
   // Handle casts from compatible types.
-  if (R->isBoundable())
+  if (R->isBoundable()) {
     if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(R)) {
       QualType ObjTy = Ctx.getCanonicalType(TR->getValueType());
       if (CanonPointeeTy == ObjTy)
         return R;
     }
 
+    if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R)) {
+      QualType SymTy =
+          Ctx.getCanonicalType(SR->getSymbol()->getType()->getPointeeType());
+      if (CanonPointeeTy == SymTy)
+        return R;
+    }
+  }
+
   // Process region cast according to the kind of the region being cast.
   switch (R->getKind()) {
     case MemRegion::CXXThisRegionKind:
     case MemRegion::CodeSpaceRegionKind:
     case MemRegion::StackLocalsSpaceRegionKind:
     case MemRegion::StackArgumentsSpaceRegionKind:
     case MemRegion::HeapSpaceRegionKind:
     case MemRegion::UnknownSpaceRegionKind:

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}}
+}

test/Analysis/new-ctor-inlined.cpp

 
 void checkNewAndConstructorInlining() {
   S *s = new S;
   // Check that the symbol for 's' is not dying.
   clang_analyzer_eval(s != 0); // expected-warning{{TRUE}}
   // Check that bindings are correct (and also not dying).
   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}}
+}