[analyzer] Inline constructors for any object with a trivial destructor.

This allows us to better reason about status objects, like Clang's own
llvm::Optional (when its contents are trivially destructible), which are
often intended to be passed around by value.

We still don't inline constructors for temporaries in the general case.

<rdar://problem/11986434>

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

8 files changed, 64 insertions, 37 deletions

diff --git a/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 54cf5690c9..b1f4f623e2 100644
--- a/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -118,8 +118,9 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
   const Expr *RetE = RS->getRetValue();
   if (!RetE)
     return;
- 
-  SVal V = C.getState()->getSVal(RetE, C.getLocationContext());
+
+  const LocationContext *LCtx = C.getLocationContext();
+  SVal V = C.getState()->getSVal(RetE, LCtx);
   const MemRegion *R = V.getAsRegion();
 
   if (!R)
@@ -132,8 +133,9 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
     return;
 
   // Return stack memory in an ancestor stack frame is fine.
-  const StackFrameContext *SFC = SS->getStackFrame();
-  if (SFC != C.getLocationContext()->getCurrentStackFrame())
+  const StackFrameContext *CurFrame = LCtx->getCurrentStackFrame();
+  const StackFrameContext *MemFrame = SS->getStackFrame();
+  if (MemFrame != CurFrame)
     return;
 
   // Automatic reference counting automatically copies blocks.
@@ -141,6 +143,11 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
       isa<BlockDataRegion>(R))
     return;
 
+  // Returning a record by value is fine. (In this case, the returned
+  // expression will be a copy-constructor.)
+  if (isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType())
+    return;
+
   EmitStackError(C, R, RetE);
 }
 
diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp
index d6bcfe4eb2..15379d6b91 100644
--- a/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -839,12 +839,8 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
 
     case Expr::MaterializeTemporaryExprClass: {
       Bldr.takeNodes(Pred);
-      const MaterializeTemporaryExpr *Materialize
-                                            = cast<MaterializeTemporaryExpr>(S);
-      if (Materialize->getType()->isRecordType())
-        Dst.Add(Pred);
-      else
-        CreateCXXTemporaryObject(Materialize, Pred, Dst);
+      const MaterializeTemporaryExpr *MTE = cast<MaterializeTemporaryExpr>(S);
+      CreateCXXTemporaryObject(MTE, Pred, Dst);
       Bldr.addNodes(Dst);
       break;
     }
diff --git a/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/lib/StaticAnalyzer/Core/ExprEngineC.cpp
index 8608f993be..56858f4883 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -264,6 +264,7 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
       case CK_NonAtomicToAtomic:
         // True no-ops.
       case CK_NoOp:
+      case CK_ConstructorConversion:
       case CK_UserDefinedConversion:
       case CK_FunctionToPointerDecay: {
         // Copy the SVal of Ex to CastE.
@@ -375,7 +376,6 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
       case CK_BaseToDerivedMemberPointer:
       case CK_DerivedToBaseMemberPointer:
       case CK_ReinterpretMemberPointer:
-      case CK_ConstructorConversion:
       case CK_VectorSplat:
       case CK_LValueBitCast: {
         // Recover some path-sensitivty by conjuring a new value.
diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index f597ebf03f..84001d3a0c 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -32,13 +32,20 @@ void ExprEngine::CreateCXXTemporaryObject(const MaterializeTemporaryExpr *ME,
 
   // Bind the temporary object to the value of the expression. Then bind
   // the expression to the location of the object.
-  SVal V = state->getSVal(tempExpr, Pred->getLocationContext());
+  SVal V = state->getSVal(tempExpr, LCtx);
 
-  const MemRegion *R =
-    svalBuilder.getRegionManager().getCXXTempObjectRegion(ME, LCtx);
+  // If the object is a record, the constructor will have already created
+  // a temporary object region. If it is not, we need to copy the value over.
+  if (!ME->getType()->isRecordType()) {
+    const MemRegion *R =
+      svalBuilder.getRegionManager().getCXXTempObjectRegion(ME, LCtx);
+
+    SVal L = loc::MemRegionVal(R);
+    state = state->bindLoc(L, V);
+    V = L;
+  }
 
-  state = state->bindLoc(loc::MemRegionVal(R), V);
-  Bldr.generateNode(ME, Pred, state->BindExpr(ME, LCtx, loc::MemRegionVal(R)));
+  Bldr.generateNode(ME, Pred, state->BindExpr(ME, LCtx, V));
 }
 
 void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
@@ -101,8 +108,12 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
       // FIXME: This will eventually need to handle new-expressions as well.
     }
 
-    // If we couldn't find an existing region to construct into, we'll just
-    // generate a symbolic region, which is fine.
+    // If we couldn't find an existing region to construct into, assume we're
+    // constructing a temporary.
+    if (!Target) {
+      MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
+      Target = MRMgr.getCXXTempObjectRegion(CE, LCtx);
+    }
 
     break;
   }
diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
index 1739feb7aa..6529b84c4b 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -340,13 +340,6 @@ bool ExprEngine::inlineCall(const CallEvent &Call, const Decl *D,
     if (!shouldInlineCXX(getAnalysisManager()))
       return false;
 
-    // Only inline constructors and destructors if we built the CFGs for them
-    // properly.
-    const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
-    if (!ADC->getCFGBuildOptions().AddImplicitDtors ||
-        !ADC->getCFGBuildOptions().AddInitializers)
-      return false;
-
     const CXXConstructorCall &Ctor = cast<CXXConstructorCall>(Call);
 
     // FIXME: We don't handle constructors or destructors for arrays properly.
@@ -354,6 +347,17 @@ bool ExprEngine::inlineCall(const CallEvent &Call, const Decl *D,
     if (Target && isa<ElementRegion>(Target))
       return false;
 
+    // If the destructor is trivial, it's always safe to inline the constructor.
+    if (Ctor.getDecl()->getParent()->hasTrivialDestructor())
+      break;
+    
+    // For other types, only inline constructors if we built the CFGs for the
+    // destructor properly.
+    const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
+    assert(ADC->getCFGBuildOptions().AddInitializers && "No CFG initializers");
+    if (!ADC->getCFGBuildOptions().AddImplicitDtors)
+      return false;
+
     // FIXME: This is a hack. We don't handle temporary destructors
     // right now, so we shouldn't inline their constructors.
     const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr();
@@ -370,8 +374,7 @@ bool ExprEngine::inlineCall(const CallEvent &Call, const Decl *D,
     // Only inline constructors and destructors if we built the CFGs for them
     // properly.
     const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
-    if (!ADC->getCFGBuildOptions().AddImplicitDtors ||
-        !ADC->getCFGBuildOptions().AddInitializers)
+    if (!ADC->getCFGBuildOptions().AddImplicitDtors)
       return false;
 
     const CXXDestructorCall &Dtor = cast<CXXDestructorCall>(Call);
diff --git a/test/Analysis/base-init.cpp b/test/Analysis/base-init.cpp
index e63d50855e..b8d0689713 100644
--- a/test/Analysis/base-init.cpp
+++ b/test/Analysis/base-init.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-store region -analyzer-ipa=inlining -verify %s
-// XFAIL: *
 
 void clang_analyzer_eval(bool);
 
diff --git a/test/Analysis/method-call.cpp b/test/Analysis/method-call.cpp
index 912062739c..65bd5155dd 100644
--- a/test/Analysis/method-call.cpp
+++ b/test/Analysis/method-call.cpp
@@ -15,23 +15,22 @@ void testNullObject(A *a) {
   clang_analyzer_eval(a); // expected-warning{{TRUE}}
 }
 
-
-// FIXME: These require constructor inlining to be enabled.
-
 void f1() {
   A x(3);
-  // should be TRUE
-  clang_analyzer_eval(x.getx() == 3); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(x.getx() == 3); // expected-warning{{TRUE}}
 }
 
 void f2() {
   const A &x = A(3);
-  // should be TRUE
-  clang_analyzer_eval(x.getx() == 3); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(x.getx() == 3); // expected-warning{{TRUE}}
 }
 
 void f3() {
   const A &x = (A)3;
-  // should be TRUE
-  clang_analyzer_eval(x.getx() == 3); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(x.getx() == 3); // expected-warning{{TRUE}}
+}
+
+void f4() {
+  A x = 3;
+  clang_analyzer_eval(x.getx() == 3); // expected-warning{{TRUE}}
 }
diff --git a/test/Analysis/new.cpp b/test/Analysis/new.cpp
index fb77de22f1..fdd16da3dc 100644
--- a/test/Analysis/new.cpp
+++ b/test/Analysis/new.cpp
@@ -74,6 +74,18 @@ void testScalarInitialization() {
 }
 
 
+struct PtrWrapper {
+  int *x;
+
+  PtrWrapper(int *input) : x(input) {}
+};
+
+PtrWrapper *testNewInvalidation() {
+  // Ensure that we don't consider this a leak.
+  return new PtrWrapper(static_cast<int *>(malloc(4)));
+}
+
+
 //--------------------------------
 // Incorrectly-modelled behavior
 //--------------------------------