[analyzer] Support returning objects by value.

[NoQ]

Summary

Now that we've added the necessary information into the CFG in D42875, we can use it to support returning objects by value.

Unfortunately, we still do not support the constructions into temporaries which are later copied via copy-constructors, which is common in statements like return Class(). But we can still support objects constructed in a different manner, say return getObject() or return { 1, 2 }. Also, we've always supported return Obj; when Obj has a trivial copy constructor, but now we support it even when the constructor is non-trivial.

This patch adds a facility to prevent the new approach from being activated when temporary destructors are disabled, because we have a mechanism for suppressing hilarious false positives that wouldn't work in this case - see D42779. Namely, i'm adding a new EvalCallOption that will indicate that we're constructing a temporary object.

[NoQ]

Destructor for the returned temporary is still evaluated conservatively:

class C {
public:
  int &x, &y;
  C(int &_x, int &_y) : x(_x), y(_y) { ++x; }
  C(const C &c) : x(c.x), y(c.y) { ++x; }
  ~C() { ++y; }
};

C make(int &x, int &y) {
  return { x, y };
}

void test() {
  int x = 0, y = 0;
  {
    C c = make(x, y);
  }
  clang_analyzer_dump(x); // 2 S32b
  clang_analyzer_dump(y); // 1 S32b
}

Also there's a weird rebound at the call site - from the callee-context-temporary to the caller-context-temporary - which is currently implemented as a trivial copy and not a full-featured constructor call. I guess there shouldn't be a copy here (because there is no constructor or assignment operator), so i suspect we should directly construct into the caller context's temporary for the call-expression.

These issues are known, i'm just pointing out that we'd need to tackle them to make at least this simple example work correctly.

[NoQ]

Well, it still seems to be a reasonable improvement given how all temporary materialization works now, and it's under the flag (-analyzer-config cfg-temporary-dtors=true), so i guess i'll mark it as TODO and commit, and i'll keep an eye on that when looking at real-world code analysis results. Destructor fixes and tests are also coming in D43104.

[NoQ]

Update the comment with some thoughts from http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html

[NoQ]

Committed this but put a wrong phabricator link in the commit message, sorry >_<
rC325202

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

public:
   /// call origin expression.
   ProgramStateRef bindReturnValue(const CallEvent &Call,
                                   const LocationContext *LCtx,
                                   ProgramStateRef State);
 
   struct EvalCallOptions {
     bool IsConstructorWithImproperlyModeledTargetRegion = false;
     bool IsArrayConstructorOrDestructor = false;
+    bool IsConstructorIntoTemporary = false;
 
     EvalCallOptions() {}
   };
 
   /// Evaluate a call, running pre- and post-call checks and allowing checkers
   /// to be responsible for handling the evaluation of the call itself.
   void evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
                 const CallEvent &Call);

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

 
 
 const MemRegion *
 ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
                                           ExplodedNode *Pred,
                                           EvalCallOptions &CallOpts) {
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
+  MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
 
   // See if we're constructing an existing region by looking at the
   // current construction context.
   if (auto CC = getCurrentCFGElement().getAs<CFGConstructor>()) {
     if (const Stmt *TriggerStmt = CC->getTriggerStmt()) {
       if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(TriggerStmt)) {
         if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
           // TODO: Detect when the allocator returns a null pointer.
if (const Stmt *TriggerStmt = CC->getTriggerStmt()) {
         }
       } else if (auto *DS = dyn_cast<DeclStmt>(TriggerStmt)) {
         const auto *Var = cast<VarDecl>(DS->getSingleDecl());
         SVal LValue = State->getLValue(Var, LCtx);
         QualType Ty = Var->getType();
         LValue = makeZeroElementRegion(State, LValue, Ty,
                                        CallOpts.IsArrayConstructorOrDestructor);
         return LValue.getAsRegion();
+      } else if (auto *RS = dyn_cast<ReturnStmt>(TriggerStmt)) {
+        CallOpts.IsConstructorIntoTemporary = true;
+        return MRMgr.getCXXTempObjectRegion(CE, LCtx);
       }
       // TODO: Consider other directly initialized elements.
     } else if (const CXXCtorInitializer *Init = CC->getTriggerInit()) {
       assert(Init->isAnyMemberInitializer());
       const CXXMethodDecl *CurCtor = cast<CXXMethodDecl>(LCtx->getDecl());
       Loc ThisPtr =
       getSValBuilder().getCXXThis(CurCtor, LCtx->getCurrentStackFrame());
       SVal ThisVal = State->getSVal(ThisPtr);
if (auto CC = getCurrentCFGElement().getAs<CFGConstructor>()) {
 
     // FIXME: This will eventually need to handle new-expressions as well.
     // Don't forget to update the pre-constructor initialization code in
     // ExprEngine::VisitCXXConstructExpr.
   }
   // If we couldn't find an existing region to construct into, assume we're
   // constructing a temporary. Notify the caller of our failure.
   CallOpts.IsConstructorWithImproperlyModeledTargetRegion = true;
-  MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
   return MRMgr.getCXXTempObjectRegion(CE, LCtx);
 }
 
 const CXXConstructExpr *
 ExprEngine::findDirectConstructorForCurrentCFGElement() {
   // Go backward in the CFG to see if the previous element (ignoring
   // destructors) was a CXXConstructExpr. If so, that constructor
   // was constructed directly into an existing region.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

case CE_CXXConstructor: {
     if (Ctor.getDecl()->getParent()->hasTrivialDestructor())
       break;
 
     // For other types, only inline constructors if destructor inlining is
     // also enabled.
     if (!Opts.mayInlineCXXMemberFunction(CIMK_Destructors))
       return CIP_DisallowedAlways;
 
-    // FIXME: This is a hack. We don't handle temporary destructors
-    // right now, so we shouldn't inline their constructors.
-    if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete)
+    if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete) {
+      // If we don't handle temporary destructors, we shouldn't inline
+      // their constructors.
+      if (CallOpts.IsConstructorIntoTemporary &&
+          !Opts.includeTemporaryDtorsInCFG())
+        return CIP_DisallowedOnce;
+
+      // If we did not construct the correct this-region, it would be pointless
+      // to inline the constructor. Instead we will simply invalidate
+      // the fake temporary target.
       if (CallOpts.IsConstructorWithImproperlyModeledTargetRegion)
         return CIP_DisallowedOnce;
+    }
 
     break;
   }
   case CE_CXXDestructor: {
     if (!Opts.mayInlineCXXMemberFunction(CIMK_Destructors))
       return CIP_DisallowedAlways;
 
     // Inlining destructors requires building the CFG correctly.

test/Analysis/temporaries.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++03 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true %s -std=c++11
 
 extern bool clang_analyzer_eval(bool);
 extern bool clang_analyzer_warnIfReached();
+void clang_analyzer_checkInlined(bool);
 
 struct Trivial {
   Trivial(int x) : value(x) {}
   int value;
 };
 
 struct NonTrivial : public Trivial {
   NonTrivial(int x) : Trivial(x) {}

 
 // Test for fallback logic that conservatively stops exploration after
 // executing a temporary constructor for a class with a no-return destructor
 // when temporary destructors are not enabled in the CFG.
 
   struct CtorWithNoReturnDtor {
     CtorWithNoReturnDtor() = default;
 
+    CtorWithNoReturnDtor(int x) {
+      clang_analyzer_checkInlined(false); // no-warning
+    }
+
     ~CtorWithNoReturnDtor() __attribute__((noreturn));
   };
 
   void testDefaultContructorWithNoReturnDtor() {
     CtorWithNoReturnDtor();
     clang_analyzer_warnIfReached();  // no-warning
   }
 
   void testLifeExtensionWithNoReturnDtor() {
     const CtorWithNoReturnDtor &c = CtorWithNoReturnDtor();
 
     // This represents an (expected) loss of coverage, since the destructor
     // of the lifetime-exended temporary is executed at the end of
     // scope.
     clang_analyzer_warnIfReached();  // no-warning
   }
 
+#if __cplusplus >= 201103L
+  CtorWithNoReturnDtor returnNoReturnDtor() {
+    return {1}; // no-crash
+  }
+#endif
+
 #endif // TEMPORARY_DTORS
 }
 
 void testStaticMaterializeTemporaryExpr() {
   static const Trivial &ref = getTrivial();
   clang_analyzer_eval(ref.value == 42); // expected-warning{{TRUE}}
 
   static const Trivial &directRef = Trivial(42);
public:
   }
   const int &j;
 };
 void run() {
   int i = 22;
   Sub(i).m();
 }
 }
+
+namespace test_return_temporary {
+class C {
+  int x, y;
+
+public:
+  C(int x, int y) : x(x), y(y) {}
+  int getX() const { return x; }
+  int getY() const { return y; }
+  ~C() {}
+};
+
+class D: public C {
+public:
+  D() : C(1, 2) {}
+  D(const D &d): C(d.getX(), d.getY()) {}
+};
+
+C returnTemporaryWithVariable() { C c(1, 2); return c; }
+C returnTemporaryWithAnotherFunctionWithVariable() {
+  return returnTemporaryWithVariable();
+}
+C returnTemporaryWithCopyConstructionWithVariable() {
+  return C(returnTemporaryWithVariable());
+}
+
+C returnTemporaryWithConstruction() { return C(1, 2); }
+C returnTemporaryWithAnotherFunctionWithConstruction() {
+  return returnTemporaryWithConstruction();
+}
+C returnTemporaryWithCopyConstructionWithConstruction() {
+  return C(returnTemporaryWithConstruction());
+}
+
+D returnTemporaryWithVariableAndNonTrivialCopy() { D d; return d; }
+D returnTemporaryWithAnotherFunctionWithVariableAndNonTrivialCopy() {
+  return returnTemporaryWithVariableAndNonTrivialCopy();
+}
+D returnTemporaryWithCopyConstructionWithVariableAndNonTrivialCopy() {
+  return D(returnTemporaryWithVariableAndNonTrivialCopy());
+}
+
+#if __cplusplus >= 201103L
+C returnTemporaryWithBraces() { return {1, 2}; }
+C returnTemporaryWithAnotherFunctionWithBraces() {
+  return returnTemporaryWithBraces();
+}
+C returnTemporaryWithCopyConstructionWithBraces() {
+  return C(returnTemporaryWithBraces());
+}
+#endif // C++11
+
+void test() {
+  C c1 = returnTemporaryWithVariable();
+  clang_analyzer_eval(c1.getX() == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(c1.getY() == 2); // expected-warning{{TRUE}}
+
+  C c2 = returnTemporaryWithAnotherFunctionWithVariable();
+  clang_analyzer_eval(c2.getX() == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(c2.getY() == 2); // expected-warning{{TRUE}}
+
+  C c3 = returnTemporaryWithCopyConstructionWithVariable();
+  clang_analyzer_eval(c3.getX() == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(c3.getY() == 2); // expected-warning{{TRUE}}
+
+  C c4 = returnTemporaryWithConstruction();
+  // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+  // in the inlined function is supported.
+  clang_analyzer_eval(c4.getX() == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(c4.getY() == 2); // expected-warning{{UNKNOWN}}
+
+  C c5 = returnTemporaryWithAnotherFunctionWithConstruction();
+  // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+  // in the inlined function is supported.
+  clang_analyzer_eval(c5.getX() == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(c5.getY() == 2); // expected-warning{{UNKNOWN}}
+
+  C c6 = returnTemporaryWithCopyConstructionWithConstruction();
+  // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+  // in the inlined function is supported.
+  clang_analyzer_eval(c5.getX() == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(c5.getY() == 2); // expected-warning{{UNKNOWN}}
+
+#if __cplusplus >= 201103L
+
+  C c7 = returnTemporaryWithBraces();
+  clang_analyzer_eval(c7.getX() == 1);
+  clang_analyzer_eval(c7.getY() == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+
+  C c8 = returnTemporaryWithAnotherFunctionWithBraces();
+  clang_analyzer_eval(c8.getX() == 1);
+  clang_analyzer_eval(c8.getY() == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+
+  C c9 = returnTemporaryWithCopyConstructionWithBraces();
+  clang_analyzer_eval(c9.getX() == 1);
+  clang_analyzer_eval(c9.getY() == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+
+#endif // C++11
+
+  D d1 = returnTemporaryWithVariableAndNonTrivialCopy();
+  clang_analyzer_eval(d1.getX() == 1);
+  clang_analyzer_eval(d1.getY() == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+
+  D d2 = returnTemporaryWithAnotherFunctionWithVariableAndNonTrivialCopy();
+  clang_analyzer_eval(d2.getX() == 1);
+  clang_analyzer_eval(d2.getY() == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+
+  // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+  // in the inlined function is supported.
+  D d3 = returnTemporaryWithCopyConstructionWithVariableAndNonTrivialCopy();
+  clang_analyzer_eval(d3.getX() == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(d3.getY() == 2); // expected-warning{{UNKNOWN}}
+}
+} // namespace test_return_temporary