Re-apply "[analyzer] Model trivial copy/move ctors with an aggregate bind."

With the optimization in the previous commit, this should be safe again.

Originally applied in r173951, then reverted in r174069.

llvm-svn: 174212

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

@@ -44,6 +44,7 @@ namespace ento {
 class AnalysisManager;
 class CallEvent;
 class SimpleCall;
+class CXXConstructorCall;
 
 class ExprEngine : public SubEngine {
 public:
@@ -546,6 +547,10 @@ private:
                      ExplodedNode *Pred);
 
   bool replayWithoutInlining(ExplodedNode *P, const LocationContext *CalleeLC);
+
+  /// Models a trivial copy or move constructor call with a simple bind.
+  void performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
+                          const CXXConstructorCall &Call);
 };
 
 /// Traits for storing the call processing policy inside GDM.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -49,6 +49,35 @@ void ExprEngine::CreateCXXTemporaryObject(const MaterializeTemporaryExpr *ME,
   Bldr.generateNode(ME, Pred, state->BindExpr(ME, LCtx, V));
 }
 
+void ExprEngine::performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
+                                    const CXXConstructorCall &Call) {
+  const CXXConstructExpr *CtorExpr = Call.getOriginExpr();
+  assert(CtorExpr->getConstructor()->isCopyOrMoveConstructor());
+  assert(CtorExpr->getConstructor()->isTrivial());
+
+  SVal ThisVal = Call.getCXXThisVal();
+  const LocationContext *LCtx = Pred->getLocationContext();
+
+  ExplodedNodeSet Dst;
+  Bldr.takeNodes(Pred);
+
+  SVal V = Call.getArgSVal(0);
+
+  // Make sure the value being copied is not unknown.
+  if (const Loc *L = dyn_cast<Loc>(&V))
+    V = Pred->getState()->getSVal(*L);
+
+  evalBind(Dst, CtorExpr, Pred, ThisVal, V, true);
+
+  PostStmt PS(CtorExpr, LCtx);
+  for (ExplodedNodeSet::iterator I = Dst.begin(), E = Dst.end();
+       I != E; ++I) {
+    ProgramStateRef State = (*I)->getState();
+    State = bindReturnValue(Call, LCtx, State);
+    Bldr.generateNode(PS, State, *I);
+  }
+}
+
 void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
                                        ExplodedNode *Pred,
                                        ExplodedNodeSet &destNodes) {
@@ -56,6 +85,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   ProgramStateRef State = Pred->getState();
 
   const MemRegion *Target = 0;
+  bool IsArray = false;
 
   switch (CE->getConstructionKind()) {
   case CXXConstructExpr::CK_Complete: {
@@ -79,6 +109,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
                 Target = State->getLValue(AT->getElementType(),
                                           getSValBuilder().makeZeroArrayIndex(),
                                           Base).getAsRegion();
+                IsArray = true;
               } else {
                 Target = State->getLValue(Var, LCtx).getAsRegion();
               }
@@ -148,14 +179,25 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   getCheckerManager().runCheckersForPreCall(DstPreCall, DstPreVisit,
                                             *Call, *this);
 
-  ExplodedNodeSet DstInvalidated;
-  StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx);
-  for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
-       I != E; ++I)
-    defaultEvalCall(Bldr, *I, *Call);
+  ExplodedNodeSet DstEvaluated;
+  StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx);
+
+  if (CE->getConstructor()->isTrivial() &&
+      CE->getConstructor()->isCopyOrMoveConstructor() &&
+      !IsArray) {
+    // FIXME: Handle other kinds of trivial constructors as well.
+    for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
+         I != E; ++I)
+      performTrivialCopy(Bldr, *I, *Call);
+
+  } else {
+    for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
+         I != E; ++I)
+      defaultEvalCall(Bldr, *I, *Call);
+  }
 
   ExplodedNodeSet DstPostCall;
-  getCheckerManager().runCheckersForPostCall(DstPostCall, DstInvalidated,
+  getCheckerManager().runCheckersForPostCall(DstPostCall, DstEvaluated,
                                              *Call, *this);
   getCheckerManager().runCheckersForPostStmt(destNodes, DstPostCall, CE, *this);
 }

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

@@ -187,6 +187,23 @@ static bool wasDifferentDeclUsedForInlining(CallEventRef<> Call,
   return RuntimeCallee->getCanonicalDecl() != StaticDecl->getCanonicalDecl();
 }
 
+/// Returns true if the CXXConstructExpr \p E was intended to construct a
+/// prvalue for the region in \p V.
+///
+/// Note that we can't just test for rvalue vs. glvalue because
+/// CXXConstructExprs embedded in DeclStmts and initializers are considered
+/// rvalues by the AST, and the analyzer would like to treat them as lvalues.
+static bool isTemporaryPRValue(const CXXConstructExpr *E, SVal V) {
+  if (E->isGLValue())
+    return false;
+
+  const MemRegion *MR = V.getAsRegion();
+  if (!MR)
+    return false;
+
+  return isa<CXXTempObjectRegion>(MR);
+}
+
 /// The call exit is simulated with a sequence of nodes, which occur between 
 /// CallExitBegin and CallExitEnd. The following operations occur between the 
 /// two program points:
@@ -247,13 +264,9 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
         svalBuilder.getCXXThis(CCE->getConstructor()->getParent(), calleeCtx);
       SVal ThisV = state->getSVal(This);
 
-      // If the constructed object is a prvalue, get its bindings.
-      // Note that we have to be careful here because constructors embedded
-      // in DeclStmts are not marked as lvalues.
-      if (!CCE->isGLValue())
-        if (const MemRegion *MR = ThisV.getAsRegion())
-          if (isa<CXXTempObjectRegion>(MR))
-            ThisV = state->getSVal(cast<Loc>(ThisV));
+      // If the constructed object is a temporary prvalue, get its bindings.
+      if (isTemporaryPRValue(CCE, ThisV))
+        ThisV = state->getSVal(cast<Loc>(ThisV));
 
       state = state->BindExpr(CCE, callerCtx, ThisV);
     }
@@ -692,7 +705,13 @@ ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call,
     }
     }
   } else if (const CXXConstructorCall *C = dyn_cast<CXXConstructorCall>(&Call)){
-    return State->BindExpr(E, LCtx, C->getCXXThisVal());
+    SVal ThisV = C->getCXXThisVal();
+
+    // If the constructed object is a temporary prvalue, get its bindings.
+    if (isTemporaryPRValue(cast<CXXConstructExpr>(E), ThisV))
+      ThisV = State->getSVal(cast<Loc>(ThisV));
+
+    return State->BindExpr(E, LCtx, ThisV);
   }
 
   // Conjure a symbol if the return value is unknown.

clang/test/Analysis/ctor-inlining.mm

@@ -1,8 +1,15 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_checkInlined(bool);
 
+// A simplified version of std::move.
+template <typename T>
+T &&move(T &obj) {
+  return static_cast<T &&>(obj);
+}
+
+
 struct Wrapper {
   __strong id obj;
 };
@@ -117,3 +124,96 @@ namespace ConstructorUsedAsRValue {
     clang_analyzer_eval(result); // expected-warning{{TRUE}}
   }
 }
+
+namespace PODUninitialized {
+  class POD {
+  public:
+    int x, y;
+  };
+
+  class PODWrapper {
+  public:
+    POD p;
+  };
+
+  class NonPOD {
+  public:
+    int x, y;
+
+    NonPOD() {}
+    NonPOD(const NonPOD &Other)
+      : x(Other.x), y(Other.y) // expected-warning {{undefined}}
+    {
+    }
+    NonPOD(NonPOD &&Other)
+    : x(Other.x), y(Other.y) // expected-warning {{undefined}}
+    {
+    }
+  };
+
+  class NonPODWrapper {
+  public:
+    class Inner {
+    public:
+      int x, y;
+
+      Inner() {}
+      Inner(const Inner &Other)
+        : x(Other.x), y(Other.y) // expected-warning {{undefined}}
+      {
+      }
+      Inner(Inner &&Other)
+      : x(Other.x), y(Other.y) // expected-warning {{undefined}}
+      {
+      }
+    };
+
+    Inner p;
+  };
+
+  void testPOD() {
+    POD p;
+    p.x = 1;
+    POD p2 = p; // no-warning
+    clang_analyzer_eval(p2.x == 1); // expected-warning{{TRUE}}
+    POD p3 = move(p); // no-warning
+    clang_analyzer_eval(p3.x == 1); // expected-warning{{TRUE}}
+
+    // Use rvalues as well.
+    clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
+
+    PODWrapper w;
+    w.p.y = 1;
+    PODWrapper w2 = w; // no-warning
+    clang_analyzer_eval(w2.p.y == 1); // expected-warning{{TRUE}}
+    PODWrapper w3 = move(w); // no-warning
+    clang_analyzer_eval(w3.p.y == 1); // expected-warning{{TRUE}}
+
+    // Use rvalues as well.
+    clang_analyzer_eval(PODWrapper(w3).p.y == 1); // expected-warning{{TRUE}}
+  }
+
+  void testNonPOD() {
+    NonPOD p;
+    p.x = 1;
+    NonPOD p2 = p;
+  }
+
+  void testNonPODMove() {
+    NonPOD p;
+    p.x = 1;
+    NonPOD p2 = move(p);
+  }
+
+  void testNonPODWrapper() {
+    NonPODWrapper w;
+    w.p.y = 1;
+    NonPODWrapper w2 = w;
+  }
+
+  void testNonPODWrapperMove() {
+    NonPODWrapper w;
+    w.p.y = 1;
+    NonPODWrapper w2 = move(w);
+  }
+}

clang/test/Analysis/temporaries.cpp

@@ -16,7 +16,7 @@ Trivial getTrivial() {
 }
 
 const Trivial &getTrivialRef() {
-  return Trivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'struct Trivial' returned to caller}}
+  return Trivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'const struct Trivial' returned to caller}}
 }
 
 
@@ -25,6 +25,6 @@ NonTrivial getNonTrivial() {
 }
 
 const NonTrivial &getNonTrivialRef() {
-  return NonTrivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'struct NonTrivial' returned to caller}}
+  return NonTrivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'const struct NonTrivial' returned to caller}}
 }