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

2018-12-19 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL349696: [analyzer] Improve modeling for returning an object 
from the top frame with RVO. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55804?vs=178936=178975#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  cfe/trunk/test/Analysis/temporaries.cpp

Index: cfe/trunk/test/Analysis/temporaries.cpp
===
--- cfe/trunk/test/Analysis/temporaries.cpp
+++ cfe/trunk/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 @@
   }
 
 #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 @@
   // 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 @@
 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 @@
 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 @@
   // 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

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

2018-12-19 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:224
+bool IsArray = false;
+V = makeZeroElementRegion(State, V, ReturnTy, IsArray);
+assert(!IsArray && "Returning array from a function!");

dcoughlin wrote:
> I don't understand why you are using makeZeroElementRegion() here. Doesn't 
> the assertion of !IsArray mean it must just return 'V'?
Hmm. Right. I mis-remembered that it's also modeling a cast.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-19 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ updated this revision to Diff 178936.
NoQ marked 2 inline comments as done.
NoQ added a comment.

Fxd!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ 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 @@
   }
 
 #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 @@
   // 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 @@
 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 @@
 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 @@
   // 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.

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

2018-12-18 Thread Devin Coughlin via Phabricator via cfe-commits 
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

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




Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:224
+bool IsArray = false;
+V = makeZeroElementRegion(State, V, ReturnTy, IsArray);
+assert(!IsArray && "Returning array from a function!");

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-18 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ updated this revision to Diff 178802.
NoQ added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ 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 @@
   }
 
 #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 @@
   // 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 @@
 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 @@
 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

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

2018-12-18 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-18 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-18 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55804/new/

https://reviews.llvm.org/D55804



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-17 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, 
rnkovacs, Szelethus, mikhail.ramalho, baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D55804

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ 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();
@@ -805,7 +821,12 @@
   // 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 +834,12 @@
 clang_analyzer_eval(x == 0); //