[analyzer] Improve modeling for returning an object from the top frame with RVO.

Static Analyzer processes the program function-by-function, sometimes diving
into other functions ("inlining" them). When an object is returned from an
inlined function, Return Value Optimization is modeled, and the returned object
is constructed at its return location directly.

When an object is returned from the function from which the analysis has started
(the top stack frame of the analysis), the return location is unknown. Model it
with a SymbolicRegion based on a conjured symbol that is specifically tagged for
that purpose, because this is generally the correct way to symbolicate
unknown locations in Static Analyzer.

Fixes leak false positives when an object is returned from top frame in C++17:
objects that are put into a SymbolicRegion-based memory region automatically
"escape" and no longer get reported as leaks. This only applies to C++17 return
values with destructors, because it produces a redundant CXXBindTemporaryExpr
in the call site, which confuses our liveness analysis. The actual fix
for liveness analysis is still pending, but it is no longer causing problems.

Additionally, re-enable temporary destructor tests in C++17.

Differential Revision: https://reviews.llvm.org/D55804

rdar://problem/46217550


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

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -113,7 +113,9 @@ SVal ExprEngine::makeZeroElementRegion(ProgramStateRef State, SVal LValue,
 std::pair<ProgramStateRef, SVal> ExprEngine::prepareForObjectConstruction(
     const Expr *E, ProgramStateRef State, const LocationContext *LCtx,
     const ConstructionContext *CC, EvalCallOptions &CallOpts) {
-  MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
+  SValBuilder &SVB = getSValBuilder();
+  MemRegionManager &MRMgr = SVB.getRegionManager();
+  ASTContext &ACtx = SVB.getContext();
 
   // See if we're constructing an existing region by looking at the
   // current construction context.
@@ -139,7 +141,7 @@ std::pair<ProgramStateRef, SVal> ExprEngine::prepareForObjectConstruction(
       assert(Init->isAnyMemberInitializer());
       const CXXMethodDecl *CurCtor = cast<CXXMethodDecl>(LCtx->getDecl());
       Loc ThisPtr =
-      getSValBuilder().getCXXThis(CurCtor, LCtx->getStackFrame());
+      SVB.getCXXThis(CurCtor, LCtx->getStackFrame());
       SVal ThisVal = State->getSVal(ThisPtr);
 
       const ValueDecl *Field;
@@ -199,12 +201,25 @@ std::pair<ProgramStateRef, SVal> ExprEngine::prepareForObjectConstruction(
             cast<Expr>(SFC->getCallSite()), State, CallerLCtx,
             RTC->getConstructionContext(), CallOpts);
       } else {
-        // We are on the top frame of the analysis.
-        // TODO: What exactly happens when we are? Does the temporary object
-        // live long enough in the region store in this case? Would checkers
-        // think that this object immediately goes out of scope?
-        CallOpts.IsTemporaryCtorOrDtor = true;
-        SVal V = loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx));
+        // We are on the top frame of the analysis. We do not know where is the
+        // object returned to. Conjure a symbolic region for the return value.
+        // TODO: We probably need a new MemRegion kind to represent the storage
+        // of that SymbolicRegion, so that we cound produce a fancy symbol
+        // instead of an anonymous conjured symbol.
+        // TODO: Do we need to track the region to avoid having it dead
+        // too early? It does die too early, at least in C++17, but because
+        // putting anything into a SymbolicRegion causes an immediate escape,
+        // it doesn't cause any leak false positives.
+        const auto *RCC = cast<ReturnedValueConstructionContext>(CC);
+        // Make sure that this doesn't coincide with any other symbol
+        // conjured for the returned expression.
+        static const int TopLevelSymRegionTag = 0;
+        const Expr *RetE = RCC->getReturnStmt()->getRetValue();
+        assert(RetE && "Void returns should not have a construction context");
+        QualType ReturnTy = RetE->getType();
+        QualType RegionTy = ACtx.getPointerType(ReturnTy);
+        SVal V = SVB.conjureSymbolVal(&TopLevelSymRegionTag, RetE, SFC,
+                                      RegionTy, currBldrCtx->blockCount());
         return std::make_pair(State, V);
       }
       llvm_unreachable("Unhandled return value construction context!");

test/Analysis/temporaries.cpp

@@ -1,9 +1,25 @@
-// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -analyzer-config eagerly-assume=false %s -std=c++11
-// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -analyzer-config eagerly-assume=false %s -std=c++17
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus\
+// RUN:     -analyzer-checker debug.ExprInspection -Wno-non-pod-varargs\
+// RUN:     -analyzer-config eagerly-assume=false -verify %s\
+// RUN:     -std=c++03 -analyzer-config cfg-temporary-dtors=false
+
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus\
+// RUN:     -analyzer-checker debug.ExprInspection -Wno-non-pod-varargs\
+// RUN:     -analyzer-config eagerly-assume=false -verify %s\
+// RUN:     -std=c++11 -analyzer-config cfg-temporary-dtors=false
+
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus\
+// RUN:     -analyzer-checker debug.ExprInspection -Wno-non-pod-varargs\
+// RUN:     -analyzer-config eagerly-assume=false -verify %s\
+// RUN:     -std=c++11 -analyzer-config cfg-temporary-dtors=true\
+// RUN:     -DTEMPORARY_DTORS
+
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus\
+// RUN:     -analyzer-checker debug.ExprInspection -Wno-non-pod-varargs\
+// RUN:     -analyzer-config eagerly-assume=false -verify %s\
+// RUN:     -std=c++17 -analyzer-config cfg-temporary-dtors=true\
+// RUN:     -DTEMPORARY_DTORS
 
-// Note: The C++17 run-line doesn't -verify yet - it is a no-crash test.
 
 extern bool clang_analyzer_eval(bool);
 extern bool clang_analyzer_warnIfReached();
@@ -450,7 +466,16 @@ namespace destructors {
   }
 
 #if __cplusplus >= 201103L
-  CtorWithNoReturnDtor returnNoReturnDtor() {
+  struct CtorWithNoReturnDtor2 {
+    CtorWithNoReturnDtor2() = default;
+
+    CtorWithNoReturnDtor2(int x) {
+      clang_analyzer_checkInlined(true); // expected-warning{{TRUE}}
+    }
+
+    ~CtorWithNoReturnDtor2() __attribute__((noreturn));
+  };
+  CtorWithNoReturnDtor2 returnNoReturnDtor() {
     return {1}; // no-crash
   }
 #endif
@@ -805,7 +830,12 @@ void test_ternary_temporary_with_copy(int coin) {
   // On each branch the variable is constructed directly.
   if (coin) {
     clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
+#if __cplusplus < 201703L
     clang_analyzer_eval(y == 1); // expected-warning{{TRUE}}
+#else
+    // FIXME: Destructor called twice in C++17?
+    clang_analyzer_eval(y == 2); // expected-warning{{TRUE}}
+#endif
     clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
     clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
 
@@ -813,7 +843,12 @@ void test_ternary_temporary_with_copy(int coin) {
     clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
     clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
     clang_analyzer_eval(z == 1); // expected-warning{{TRUE}}
+#if __cplusplus < 201703L
     clang_analyzer_eval(w == 1); // expected-warning{{TRUE}}
+#else
+    // FIXME: Destructor called twice in C++17?
+    clang_analyzer_eval(w == 2); // expected-warning{{TRUE}}
+#endif
   }
 }
 } // namespace test_match_constructors_and_destructors
@@ -865,9 +900,12 @@ class C {
 public:
   ~C() {
     glob = 1;
+    // FIXME: Why is destructor not inlined in C++17
     clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-    // expected-warning@-2{{TRUE}}
+#if __cplusplus < 201703L
+    // expected-warning@-3{{TRUE}}
+#endif
 #endif
   }
 };
@@ -886,11 +924,16 @@ void test(int coin) {
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
+    // FIXME: Why is destructor not inlined in C++17
     clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-    // expected-warning@-2{{TRUE}}
+#if __cplusplus < 201703L
+    // expected-warning@-3{{TRUE}}
+#else
+    // expected-warning@-5{{UNKNOWN}}
+#endif
 #else
-    // expected-warning@-4{{UNKNOWN}}
+    // expected-warning@-8{{UNKNOWN}}
 #endif
   } else {
     // The destructor is not called on this branch.
@@ -1012,11 +1055,16 @@ void foo(void (*bar4)(S)) {
 #endif
 
   bar2(S(2));
+  // FIXME: Why are we losing information in C++17?
   clang_analyzer_eval(glob == 2);
 #ifdef TEMPORARY_DTORS
-  // expected-warning@-2{{TRUE}}
+#if __cplusplus < 201703L
+  // expected-warning@-3{{TRUE}}
 #else
-  // expected-warning@-4{{UNKNOWN}}
+  // expected-warning@-5{{UNKNOWN}}
+#endif
+#else
+  // expected-warning@-8{{UNKNOWN}}
 #endif
 
   C *c = new D();
@@ -1172,3 +1220,29 @@ void test() {
   c.foo();
 }
 } // namespace union_indirect_field_crash
+
+namespace return_from_top_frame {
+struct S {
+  int *p;
+  S() { p = new int; }
+  S(S &&s) : p(s.p) { s.p = 0; }
+  ~S();  // Presumably releases 'p'.
+};
+
+S foo() {
+  S s;
+  return s;
+}
+
+S bar1() {
+  return foo(); // no-warning
+}
+
+S bar2() {
+  return S();
+}
+
+S bar3(int coin) {
+  return coin ? S() : foo(); // no-warning
+}
+} // namespace return_from_top_frame