[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

224 ↗

(On Diff #178802)

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

224 ↗

(On Diff #178802)

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

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

SVal ExprEngine::makeZeroElementRegion(ProgramStateRef State, SVal LValue,
   }
 
   return 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.
   if (CC) {
     switch (CC->getKind()) {
     case ConstructionContext::CXX17ElidedCopyVariableKind:
     case ConstructionContext::SimpleVariableKind: {
       const auto *DSCC = cast<VariableConstructionContext>(CC);
if (CC) {
     }
     case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
     case ConstructionContext::SimpleConstructorInitializerKind: {
       const auto *ICC = cast<ConstructorInitializerConstructionContext>(CC);
       const auto *Init = ICC->getCXXCtorInitializer();
       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;
       SVal FieldVal;
       if (Init->isIndirectMemberInitializer()) {
         Field = Init->getIndirectMember();
         FieldVal = State->getLValue(Init->getIndirectMember(), ThisVal);
       } else {
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;
-        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!");
     }
     case ConstructionContext::ElidedTemporaryObjectKind: {
       assert(AMgr.getAnalyzerOptions().ShouldElideConstructors);
       const auto *TCC = cast<ElidedTemporaryObjectConstructionContext>(CC);
       const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();

cfe/trunk/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 testLifeExtensionWithNoReturnDtor() {
 
     // 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() {
+  struct CtorWithNoReturnDtor2 {
+    CtorWithNoReturnDtor2() = default;
+
+    CtorWithNoReturnDtor2(int x) {
+      clang_analyzer_checkInlined(true); // expected-warning{{TRUE}}
+    }
+
+    ~CtorWithNoReturnDtor2() __attribute__((noreturn));
+  };
+  CtorWithNoReturnDtor2 returnNoReturnDtor() {
     return {1}; // no-crash
   }
 #endif
 
 #endif // TEMPORARY_DTORS
 }
 
 namespace default_param_elided_destructors {

 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 bar1() {
+  return foo(); // no-warning
+}
+
+S bar2() {
+  return S();
+}
+
+S bar3(int coin) {
+  return coin ? S() : foo(); // no-warning
+}
+} // namespace return_from_top_frame