[analyzer] C++17: Fix leak false positives when an object with destructor is returned from the top frame.

[NoQ]

Summary

The return_from_top_frame test demonstrates what happens. The object bound to the sub-expression of the return statement is a lazy compound value in this case. Liveness of that expression ends slightly before the end of the analysis, leaving time for MallocChecker to diagnose a leak.

This is fixed by admitting that we do not know how to properly model the memory region in which the return value is constructed. Neither CXXTempObjectRegion nor SymbolicRegion seems suitable, because the region needs to be in an unknown memory space and must also be live after the last reference disappears. The fix is only applied to C++17 when the object has a destructor (which produces a CXX17ElidedCopyReturnedValueConstructionContext) because other cases seem to work due to liveness analysis behaving correctly.

We could (and probably should) teach liveness analysis that with C++17 AST with CXXBindTemporaryExpr it is still a good idea to keep the top-frame returned expression alive forever. But we still don't have the correct region to represent construction target. Either we should modify liveness analysis and use SymbolicRegion, or we need to come up with a new kind of region (either for the return-to object itself, or for an implicit parameter through which its address is passed into the callee, so that the return-to object would be a SymbolicRegion of its SymbolRegionValue, similarly to how CXXThisRegion works).

Before i forget: re-enable tests for temporaries on C++17. Notice that some still fail. The current patch doesn't regress any of the old tests in this file that are now FIXME'd out.

[xazax.hun]

Is there any downsides for using symbolic region for the construction target? For me that would make perfect sense, since this is often modelled by passing the address of the target into the callee. The programmer could do RVO like thing by hand, so modeling automatic and manual RVO the same way would be the least surprising in my opinion.

[NoQ]

In D55804#1334106, @xazax.hun wrote:

Is there any downsides for using symbolic region for the construction target? For me that would make perfect sense, since this is often modelled by passing the address of the target into the callee. The programmer could do RVO like thing by hand, so modeling automatic and manual RVO the same way would be the least surprising in my opinion.

Hmm, let me actually see how hard it is. The main reason why i don't like it is that we still need to come up with a symbol to stuff into it, so we're kinda just delaying the inevitable. If we had, for instance, a region for the "implicit variable" that stores the return address, we could announce that this region is live for as long as the stack frame is on the stack, and its SymbolRegionValue (together with the SymbolicRegion around it) would be automatically kept alive. But if we make an anonymous SymbolConjured instead, we would also need to introduce a separate liveness hack to keep it alive, which should ideally be removed later.

[NoQ]

Actually, LiveVariables analysis is not the right place to deal with this problem; these changes would go into SymbolReaper (or directly into ExprEngine), because we're dealing with something that depends on the stack frame, while LiveVariables analysis only depends on the CFG.

[NoQ]

Wait a minute.

In D55804#1334957, @NoQ wrote:

we would also need to introduce a separate liveness hack to keep it alive

No, we wouldn't! With a symbolic region it is fine if it dies too early, because putting anything into a symbolic region is an immediate escape anyways. It is still scary to keep it dying too early, but i don't see any actual problem arise from it, and in any case it's better than before, and the leaks do indeed go away.

Additionally, this is still a fairly safe change because there's nothing new in constructing into symbolic regions.

[dcoughlin]

This seems reasonable to me, although I have a question inline about why you are using makeZeroElementRegion().

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

234

I don't understand why you are using makeZeroElementRegion() here. Doesn't the assertion of !IsArray mean it must just return 'V'?

[NoQ]

Fxd!

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

234

Hmm. Right. I mis-remembered that it's also modeling a cast.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {
           // call in the parent stack frame. This is equivalent to not being
           // able to find construction context at all.
           break;
         }
         return 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;
+        // We are on the top frame of the analysis. We do not know where is the
+        // object returned to.
+        // In C++17, do not inline the constructor, because the object
+        // represented by the returned expression will not live long enough
+        // according to liveness analysis.
+        // TODO: Does it live long enough in all other cases?
+        // It definitely does in most cases.
+        if (CC->getKind() ==
+            ConstructionContext::CXX17ElidedCopyReturnedValueKind) {
+          CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+        }
+        // TODO: We probably need a new MemRegion kind to represent such object.
         SVal V = loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx));
+        CallOpts.IsTemporaryCtorOrDtor = true;
         return std::make_pair(State, V);
       }
       llvm_unreachable("Unhandled return value construction context!");
     }
     case ConstructionContext::ElidedTemporaryObjectKind: {
       assert(AMgr.getAnalyzerOptions().ShouldElideConstructors);
       const auto *TCC = cast<ElidedTemporaryObjectConstructionContext>(CC);
       const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();
       const MaterializeTemporaryExpr *MTE = TCC->getMaterializedTemporaryExpr();
       const CXXConstructExpr *CE = TCC->getConstructorAfterElision();
 
       // Support pre-C++17 copy elision. We'll have the elidable copy
       // constructor in the AST and in the CFG, but we'll skip it
       // and construct directly into the final object. This call
       // also sets the CallOpts flags for us.
       SVal V;
       // If the elided copy/move constructor is not supported, there's still
       // benefit in trying to model the non-elided constructor.
       // Stash our state before trying to elide, as it'll get overwritten.
       ProgramStateRef PreElideState = State;
       EvalCallOptions PreElideCallOpts = CallOpts;
 
       std::tie(State, V) = prepareForObjectConstruction(
           CE, State, LCtx, TCC->getConstructionContextAfterElision(), CallOpts);
 
       // FIXME: This definition of "copy elision has not failed" is unreliable.
       // It doesn't indicate that the constructor will actually be inlined

test/Analysis/temporaries.cpp

-// 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();
 void clang_analyzer_checkInlined(bool);
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 struct Trivial {

 void test_ternary_temporary_with_copy(int coin) {
   int x = 0, y = 0, z = 0, w = 0;
   {
     C c = coin ? C(x, y) : C(z, w);
   }
   // 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}}
 
   } else {
     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
 
 namespace destructors_for_return_values {
 
 class C {
 public:

 
 namespace dont_forget_destructor_around_logical_op {
 int glob;
 
 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
   }
 };
 
 C get();
 
 bool is(C);
 
 
 void test(int coin) {
   // Here temporaries are being cleaned up after && is evaluated. There are two
   // temporaries: the return value of get() and the elidable copy constructor
   // of that return value into is(). According to the CFG, we need to cleanup
   // both of them depending on whether the temporary corresponding to the
   // return value of get() was initialized. However, we didn't track
   // 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@-4{{UNKNOWN}}
+    // expected-warning@-5{{UNKNOWN}}
+#endif
+#else
+    // expected-warning@-8{{UNKNOWN}}
 #endif
   } else {
     // The destructor is not called on this branch.
     clang_analyzer_eval(glob); // expected-warning{{UNKNOWN}}
   }
 }
 } // namespace dont_forget_destructor_around_logical_op
 
void foo(void (*bar4)(S)) {
   clang_analyzer_eval(glob == 1);
 #ifdef TEMPORARY_DTORS
   // expected-warning@-2{{TRUE}}
 #else
   // expected-warning@-4{{UNKNOWN}}
 #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();
   c->bar3(S(3));
   // FIXME: Should be TRUE.
   clang_analyzer_eval(glob == 3); // expected-warning{{UNKNOWN}}
   delete c;
 
public:
   }
 };
 
 void test() {
   C<int> c;
   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 bar() {
+  return foo(); // no-warning
+}
+} // namespace return_from_top_frame