NoQ updated this revision to Diff 151279.
NoQ added a comment.
I added an option to disable copy elision on CFG side in
https://reviews.llvm.org/D47616. The analyzer makes use of it automagically:
elided constructors are replaced with temporary constructors in the CFG and the
old behavior is restored.
Tests now test both behaviors and demonstrate that C++17 mandatory copy elision
works exactly like pre-C++17 optional copy elision, despite underlying AST
being completely different. Well, most of the time: there's still a bug that
causes us to think that we need to call a destructor on the elided temporary.
Thankfully, such destructor would be evaluated conservatively.
https://reviews.llvm.org/D47671
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cxx17-mandatory-elision.cpp
test/Analysis/gtest.cpp
test/Analysis/inlining/temp-dtors-path-notes.cpp
test/Analysis/lifetime-extension.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -612,107 +612,44 @@
clang_analyzer_eval(c3.getY() == 2); // expected-warning{{TRUE}}
C c4 = returnTemporaryWithConstruction();
- clang_analyzer_eval(c4.getX() == 1);
- clang_analyzer_eval(c4.getY() == 2);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-3{{TRUE}}
- // expected-warning@-3{{TRUE}}
-#else
- // expected-warning@-6{{UNKNOWN}}
- // expected-warning@-6{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(c4.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c4.getY() == 2); // expected-warning{{TRUE}}
C c5 = returnTemporaryWithAnotherFunctionWithConstruction();
- clang_analyzer_eval(c5.getX() == 1);
- clang_analyzer_eval(c5.getY() == 2);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-3{{TRUE}}
- // expected-warning@-3{{TRUE}}
-#else
- // expected-warning@-6{{UNKNOWN}}
- // expected-warning@-6{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(c5.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c5.getY() == 2); // expected-warning{{TRUE}}
C c6 = returnTemporaryWithCopyConstructionWithConstruction();
- clang_analyzer_eval(c5.getX() == 1);
- clang_analyzer_eval(c5.getY() == 2);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-3{{TRUE}}
- // expected-warning@-3{{TRUE}}
-#else
- // expected-warning@-6{{UNKNOWN}}
- // expected-warning@-6{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(c5.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c5.getY() == 2); // expected-warning{{TRUE}}
#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
+ clang_analyzer_eval(c7.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c7.getY() == 2); // expected-warning{{TRUE}}
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
+ clang_analyzer_eval(c8.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c8.getY() == 2); // expected-warning{{TRUE}}
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
+ clang_analyzer_eval(c9.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c9.getY() == 2); // expected-warning{{TRUE}}
#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
+ clang_analyzer_eval(d1.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(d1.getY() == 2); // expected-warning{{TRUE}}
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
+ clang_analyzer_eval(d2.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(d2.getY() == 2); // expected-warning{{TRUE}}
D d3 = returnTemporaryWithCopyConstructionWithVariableAndNonTrivialCopy();
- clang_analyzer_eval(d3.getX() == 1);
- clang_analyzer_eval(d3.getY() == 2);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-3{{TRUE}}
- // expected-warning@-3{{TRUE}}
-#else
- // expected-warning@-6{{UNKNOWN}}
- // expected-warning@-6{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(d3.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(d3.getY() == 2); // expected-warning{{TRUE}}
}
} // namespace test_return_temporary
@@ -767,19 +704,9 @@
C c2 = coin ? C(1) : C(2);
if (coin) {
- clang_analyzer_eval(c2.getX() == 1);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-2{{TRUE}}
-#else
- // expected-warning@-4{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(c2.getX() == 1); // expected-warning{{TRUE}}
} else {
- clang_analyzer_eval(c2.getX() == 2);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-2{{TRUE}}
-#else
- // expected-warning@-4{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(c2.getX() == 2); // expected-warning{{TRUE}}
}
}
@@ -816,27 +743,20 @@
{
C c = C(x, y);
}
- // Two constructors (temporary object expr and copy) and two destructors.
- clang_analyzer_eval(x == 2);
- clang_analyzer_eval(y == 2);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-3{{TRUE}}
- // expected-warning@-3{{TRUE}}
-#else
- // expected-warning@-6{{UNKNOWN}}
- // expected-warning@-6{{UNKNOWN}}
-#endif
+ // Only one constructor directly into the variable, and one destructor.
+ clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(y == 1); // expected-warning{{TRUE}}
}
void test_ternary_temporary(int coin) {
int x = 0, y = 0, z = 0, w = 0;
{
const C &c = coin ? C(x, y) : C(z, w);
}
- // This time each branch contains an additional elidable copy constructor.
+ // Only one constructor on every branch, and one automatic destructor.
if (coin) {
- clang_analyzer_eval(x == 2);
- clang_analyzer_eval(y == 2);
+ clang_analyzer_eval(x == 1);
+ clang_analyzer_eval(y == 1);
#ifdef TEMPORARY_DTORS
// expected-warning@-3{{TRUE}}
// expected-warning@-3{{TRUE}}
@@ -850,8 +770,8 @@
} else {
clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(z == 2);
- clang_analyzer_eval(w == 2);
+ clang_analyzer_eval(z == 1);
+ clang_analyzer_eval(w == 1);
#ifdef TEMPORARY_DTORS
// expected-warning@-3{{TRUE}}
// expected-warning@-3{{TRUE}}
@@ -867,33 +787,18 @@
{
C c = coin ? C(x, y) : C(z, w);
}
- // Temporary expression, elidable copy within branch,
- // constructor for variable - 3 total.
+ // On each branch the variable is constructed directly.
if (coin) {
- clang_analyzer_eval(x == 3);
- clang_analyzer_eval(y == 3);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-3{{TRUE}}
- // expected-warning@-3{{TRUE}}
-#else
- // expected-warning@-6{{UNKNOWN}}
- // expected-warning@-6{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(y == 1); // expected-warning{{TRUE}}
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 == 3);
- clang_analyzer_eval(w == 3);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-3{{TRUE}}
- // expected-warning@-3{{TRUE}}
-#else
- // expected-warning@-6{{UNKNOWN}}
- // expected-warning@-6{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(z == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(w == 1); // expected-warning{{TRUE}}
}
}
} // namespace test_match_constructors_and_destructors
@@ -928,12 +833,13 @@
}
void testCopiedCall() {
- C c = make();
- // Should have divided by zero in the temporary destructor.
- clang_analyzer_warnIfReached();
-#ifndef TEMPORARY_DTORS
- // expected-warning@-2{{REACHABLE}}
-#endif
+ {
+ C c = make();
+ // Should have elided the constructor/destructor for the temporary
+ clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+ }
+ // Should have divided by zero in the destructor.
+ clang_analyzer_warnIfReached(); // no-warning
}
} // namespace destructors_for_return_values
@@ -1018,20 +924,10 @@
#endif
S s = 20;
- clang_analyzer_eval(s.x == 20);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-2{{TRUE}}
-#else
- // expected-warning@-4{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(s.x == 20); // expected-warning{{TRUE}}
C c2 = s;
- clang_analyzer_eval(c2.getX() == 20);
-#ifdef TEMPORARY_DTORS
- // expected-warning@-2{{TRUE}}
-#else
- // expected-warning@-4{{UNKNOWN}}
-#endif
+ clang_analyzer_eval(c2.getX() == 20); // expected-warning{{TRUE}}
}
} // end namespace implicit_constructor_conversion
Index: test/Analysis/lifetime-extension.cpp
===================================================================
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -97,13 +97,10 @@
void f2() {
C *after, *before;
- C c = C(1, &after, &before);
- clang_analyzer_eval(after == before);
-#ifdef TEMPORARIES
- // expected-warning@-2{{TRUE}}
-#else
- // expected-warning@-4{{UNKNOWN}}
-#endif
+ {
+ C c = C(1, &after, &before);
+ }
+ clang_analyzer_eval(after == before); // expected-warning{{TRUE}}
}
void f3(bool coin) {
@@ -129,20 +126,20 @@
// operator. Ideally also add support for the binary conditional operator in
// C++. Because for now it calls the constructor for the condition twice.
if (coin) {
+ // FIXME: Should not warn.
clang_analyzer_eval(after == before);
#ifdef TEMPORARIES
// expected-warning@-2{{The left operand of '==' is a garbage value}}
#else
// expected-warning@-4{{UNKNOWN}}
#endif
} else {
+ // FIXME: Should be TRUE.
clang_analyzer_eval(after == before);
#ifdef TEMPORARIES
- // Seems to work at the moment, but also seems accidental.
- // Feel free to break.
- // expected-warning@-4{{TRUE}}
+ // expected-warning@-2{{FALSE}}
#else
- // expected-warning@-6{{UNKNOWN}}
+ // expected-warning@-4{{UNKNOWN}}
#endif
}
}
@@ -234,24 +231,25 @@
} // end namespace maintain_original_object_address_on_move
namespace maintain_address_of_copies {
+class C;
-template <typename T> struct AddressVector {
- const T *buf[10];
+struct AddressVector {
+ C *buf[10];
int len;
AddressVector() : len(0) {}
- void push(const T *t) {
- buf[len] = t;
+ void push(C *c) {
+ buf[len] = c;
++len;
}
};
class C {
- AddressVector<C> &v;
+ AddressVector &v;
public:
- C(AddressVector<C> &v) : v(v) { v.push(this); }
+ C(AddressVector &v) : v(v) { v.push(this); }
~C() { v.push(this); }
#ifdef MOVES
@@ -267,132 +265,70 @@
#endif
} // no-warning
- static C make(AddressVector<C> &v) { return C(v); }
+ static C make(AddressVector &v) { return C(v); }
};
void f1() {
- AddressVector<C> v;
+ AddressVector v;
{
C c = C(v);
}
- // 0. Create the original temporary and lifetime-extend it into variable 'c'
- // construction argument.
- // 1. Construct variable 'c' (elidable copy/move).
- // 2. Destroy the temporary.
- // 3. Destroy variable 'c'.
- clang_analyzer_eval(v.len == 4);
- clang_analyzer_eval(v.buf[0] == v.buf[2]);
- clang_analyzer_eval(v.buf[1] == v.buf[3]);
-#ifdef TEMPORARIES
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
-#else
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
-#endif
+ // 0. Construct variable 'c' (copy/move elided).
+ // 1. Destroy variable 'c'.
+ clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
}
void f2() {
- AddressVector<C> v;
+ AddressVector v;
{
const C &c = C::make(v);
}
- // 0. Construct the original temporary within make(),
- // 1. Construct the return value of make() (elidable copy/move) and
- // lifetime-extend it via reference 'c',
- // 2. Destroy the temporary within make(),
- // 3. Destroy the temporary lifetime-extended by 'c'.
- clang_analyzer_eval(v.len == 4);
- clang_analyzer_eval(v.buf[0] == v.buf[2]);
- clang_analyzer_eval(v.buf[1] == v.buf[3]);
+ // 0. Construct the return value of make() (copy/move elided) and
+ // lifetime-extend it directly via reference 'c',
+ // 1. Destroy the temporary lifetime-extended by 'c'.
+ clang_analyzer_eval(v.len == 2);
+ clang_analyzer_eval(v.buf[0] == v.buf[1]);
#ifdef TEMPORARIES
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
+ // expected-warning@-3{{TRUE}}
+ // expected-warning@-3{{TRUE}}
#else
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
#endif
}
void f3() {
- AddressVector<C> v;
+ AddressVector v;
{
C &&c = C::make(v);
}
- // 0. Construct the original temporary within make(),
- // 1. Construct the return value of make() (elidable copy/move) and
- // lifetime-extend it via reference 'c',
- // 2. Destroy the temporary within make(),
- // 3. Destroy the temporary lifetime-extended by 'c'.
- clang_analyzer_eval(v.len == 4);
- clang_analyzer_eval(v.buf[0] == v.buf[2]);
- clang_analyzer_eval(v.buf[1] == v.buf[3]);
+ // 0. Construct the return value of make() (copy/move elided) and
+ // lifetime-extend it directly via reference 'c',
+ // 1. Destroy the temporary lifetime-extended by 'c'.
+ clang_analyzer_eval(v.len == 2);
+ clang_analyzer_eval(v.buf[0] == v.buf[1]);
#ifdef TEMPORARIES
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
+ // expected-warning@-3{{TRUE}}
+ // expected-warning@-3{{TRUE}}
#else
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
#endif
}
-C doubleMake(AddressVector<C> &v) {
+C doubleMake(AddressVector &v) {
return C::make(v);
}
void f4() {
- AddressVector<C> v;
+ AddressVector v;
{
C c = doubleMake(v);
}
- // 0. Construct the original temporary within make(),
- // 1. Construct the return value of make() (elidable copy/move) and
- // lifetime-extend it into the return value constructor argument within
- // doubleMake(),
- // 2. Destroy the temporary within make(),
- // 3. Construct the return value of doubleMake() (elidable copy/move) and
- // lifetime-extend it into the variable 'c' constructor argument,
- // 4. Destroy the return value of make(),
- // 5. Construct variable 'c' (elidable copy/move),
- // 6. Destroy the return value of doubleMake(),
- // 7. Destroy variable 'c'.
- clang_analyzer_eval(v.len == 8);
- clang_analyzer_eval(v.buf[0] == v.buf[2]);
- clang_analyzer_eval(v.buf[1] == v.buf[4]);
- clang_analyzer_eval(v.buf[3] == v.buf[6]);
- clang_analyzer_eval(v.buf[5] == v.buf[7]);
-#ifdef TEMPORARIES
- // expected-warning@-6{{TRUE}}
- // expected-warning@-6{{TRUE}}
- // expected-warning@-6{{TRUE}}
- // expected-warning@-6{{TRUE}}
- // expected-warning@-6{{TRUE}}
-#else
- // expected-warning@-12{{UNKNOWN}}
- // expected-warning@-12{{UNKNOWN}}
- // expected-warning@-12{{UNKNOWN}}
- // expected-warning@-12{{UNKNOWN}}
- // expected-warning@-12{{UNKNOWN}}
-#endif
+ // 0. Construct variable 'c' (all copies/moves elided),
+ // 1. Destroy variable 'c'.
+ clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
}
-
-class NoDtor {
- AddressVector<NoDtor> &v;
-
-public:
- NoDtor(AddressVector<NoDtor> &v) : v(v) { v.push(this); }
-};
-
-void f5() {
- AddressVector<NoDtor> v;
- const NoDtor &N = NoDtor(v);
- clang_analyzer_eval(v.buf[0] == &N); // expected-warning{{TRUE}}
-}
-
} // end namespace maintain_address_of_copies
Index: test/Analysis/inlining/temp-dtors-path-notes.cpp
===================================================================
--- test/Analysis/inlining/temp-dtors-path-notes.cpp
+++ test/Analysis/inlining/temp-dtors-path-notes.cpp
@@ -30,19 +30,14 @@
};
void test(int coin) {
- // We'd divide by zero in the temporary destructor that goes after the
- // elidable copy-constructor from C(0) to the lifetime-extended temporary.
- // So in fact this example has nothing to do with lifetime extension.
- // Actually, it would probably be better to elide the constructor, and
- // put the note for the destructor call at the closing brace after nop.
+ // We'd divide by zero in the automatic destructor for variable 'c'.
const C &c = coin ? C(1) : C(0); // expected-note {{Assuming 'coin' is 0}}
// expected-note@-1{{'?' condition is false}}
// expected-note@-2{{Passing the value 0 via 1st parameter 'x'}}
// expected-note@-3{{Calling constructor for 'C'}}
// expected-note@-4{{Returning from constructor for 'C'}}
- // expected-note@-5{{Calling '~C'}}
c.nop();
-}
+} // expected-note{{Calling '~C'}}
} // end namespace test_lifetime_extended_temporary
namespace test_bug_after_dtor {
Index: test/Analysis/gtest.cpp
===================================================================
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -154,13 +154,11 @@
void testAssertSymbolicPtr(const bool *b) {
ASSERT_TRUE(*b); // no-crash
- // FIXME: Our solver doesn't handle this well yet.
- clang_analyzer_eval(*b); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(*b); // expected-warning{{TRUE}}
}
void testAssertSymbolicRef(const bool &b) {
ASSERT_TRUE(b); // no-crash
- // FIXME: Our solver doesn't handle this well yet.
- clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(b); // expected-warning{{TRUE}}
}
Index: test/Analysis/cxx17-mandatory-elision.cpp
===================================================================
--- test/Analysis/cxx17-mandatory-elision.cpp
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -1,5 +1,17 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -analyzer-config elide-constructors=false -DNO_ELIDE_FLAG -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -analyzer-config elide-constructors=false -DNO_ELIDE_FLAG -verify %s
+
+// Copy elision always occurs in C++17, otherwise it's under
+// an on-by-default flag.
+#if __cplusplus >= 201703L
+ #define ELIDE 1
+#else
+ #ifndef NO_ELIDE_FLAG
+ #define ELIDE 1
+ #endif
+#endif
void clang_analyzer_eval(bool);
@@ -39,9 +51,9 @@
C() : t(T(4)) {
S s = {1, 2, 3};
t.s = s;
- // FIXME: Should be TRUE in C++11 as well.
+ // FIXME: Should be TRUE regardless of copy elision.
clang_analyzer_eval(t.w == 4);
-#if __cplusplus >= 201703L
+#ifdef ELIDE
// expected-warning@-2{{TRUE}}
#else
// expected-warning@-4{{UNKNOWN}}
@@ -149,7 +161,7 @@
AddressVector<ClassWithoutDestructor> v;
ClassWithoutDestructor c = make3(v);
-#if __cplusplus >= 201703L
+#if ELIDE
clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
clang_analyzer_eval(v.buf[0] == &c); // expected-warning{{TRUE}}
#else
@@ -184,13 +196,13 @@
ClassWithDestructor c = ClassWithDestructor(v);
// Check if the last destructor is an automatic destructor.
// A temporary destructor would have fired by now.
-#if __cplusplus >= 201703L
+#if ELIDE
clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
#else
clang_analyzer_eval(v.len == 3); // expected-warning{{TRUE}}
#endif
}
-#if __cplusplus >= 201703L
+#if ELIDE
// 0. Construct the variable.
// 1. Destroy the variable.
clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
@@ -218,13 +230,13 @@
TestCtorInitializer t(v);
// Check if the last destructor is an automatic destructor.
// A temporary destructor would have fired by now.
-#if __cplusplus >= 201703L
+#if ELIDE
clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
#else
clang_analyzer_eval(v.len == 3); // expected-warning{{TRUE}}
#endif
}
-#if __cplusplus >= 201703L
+#if ELIDE
// 0. Construct the member variable.
// 1. Destroy the member variable.
clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
@@ -257,14 +269,14 @@
ClassWithDestructor c = make3(v);
// Check if the last destructor is an automatic destructor.
// A temporary destructor would have fired by now.
-#if __cplusplus >= 201703L
+#if ELIDE
clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
#else
clang_analyzer_eval(v.len == 9); // expected-warning{{TRUE}}
#endif
}
-#if __cplusplus >= 201703L
+#if ELIDE
// 0. Construct the variable. Yes, constructor in make1() constructs
// the variable 'c'.
// 1. Destroy the variable.
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -209,12 +209,37 @@
}
llvm_unreachable("Unhandled return value construction context!");
}
- case ConstructionContext::ElidedTemporaryObjectKind:
+ case ConstructionContext::ElidedTemporaryObjectKind: {
assert(AMgr.getAnalyzerOptions().shouldElideConstructors());
- // FALL-THROUGH
+ 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;
+ std::tie(State, V) = prepareForObjectConstruction(
+ CE, State, LCtx, TCC->getConstructionContextAfterElision(), CallOpts);
+
+ // Remember that we've elided the constructor.
+ State = addObjectUnderConstruction(State, CE, LCtx, V);
+
+ // Remember that we've elided the destructor.
+ if (BTE)
+ State = elideDestructor(State, BTE, LCtx);
+
+ // Instead of materialization, shamelessly return
+ // the final object destination.
+ if (MTE)
+ State = addObjectUnderConstruction(State, MTE, LCtx, V);
+
+ return std::make_pair(State, V);
+ }
case ConstructionContext::SimpleTemporaryObjectKind: {
- // TODO: Copy elision implementation goes here.
- const auto *TCC = cast<TemporaryObjectConstructionContext>(CC);
+ const auto *TCC = cast<SimpleTemporaryObjectConstructionContext>(CC);
const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();
const MaterializeTemporaryExpr *MTE = TCC->getMaterializedTemporaryExpr();
SVal V = UnknownVal();
@@ -266,6 +291,20 @@
SVal Target = UnknownVal();
+ if (Optional<SVal> ElidedTarget =
+ getObjectUnderConstruction(State, CE, LCtx)) {
+ // We've previously modeled an elidable constructor by pretending that it in
+ // fact constructs into the correct target. This constructor can therefore
+ // be skipped.
+ Target = *ElidedTarget;
+ StmtNodeBuilder Bldr(Pred, destNodes, *currBldrCtx);
+ State = finishObjectConstruction(State, CE, LCtx);
+ if (auto L = Target.getAs<Loc>())
+ State = State->BindExpr(CE, LCtx, State->getSVal(*L, CE->getType()));
+ Bldr.generateNode(CE, Pred, State);
+ return;
+ }
+
// FIXME: Handle arrays, which run the same constructor for every element.
// For now, we just run the first constructor (which should still invalidate
// the entire array).
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -139,7 +139,8 @@
// encountered. This list may be expanded when new actions are implemented.
assert(getCXXCtorInitializer() || isa<DeclStmt>(getStmt()) ||
isa<CXXNewExpr>(getStmt()) || isa<CXXBindTemporaryExpr>(getStmt()) ||
- isa<MaterializeTemporaryExpr>(getStmt()));
+ isa<MaterializeTemporaryExpr>(getStmt()) ||
+ isa<CXXConstructExpr>(getStmt()));
}
const Stmt *getStmt() const {
@@ -183,6 +184,14 @@
REGISTER_TRAIT_WITH_PROGRAMSTATE(ObjectsUnderConstruction,
ObjectsUnderConstructionMap)
+// Additionally, track a set of destructors that correspond to elided
+// constructors when copy elision occurs.
+typedef std::pair<const CXXBindTemporaryExpr *, const LocationContext *>
+ ElidedDestructorItem;
+typedef llvm::ImmutableSet<ElidedDestructorItem>
+ ElidedDestructorSet;
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ElidedDestructors,
+ ElidedDestructorSet);
//===----------------------------------------------------------------------===//
// Engine construction and deletion.
@@ -358,14 +367,12 @@
// a new temporary region out of thin air and copy the contents of the object
// (which are currently present in the Environment, because Init is an rvalue)
// into that region. This is not correct, but it is better than nothing.
- bool FoundOriginalMaterializationRegion = false;
const TypedValueRegion *TR = nullptr;
if (const auto *MT = dyn_cast<MaterializeTemporaryExpr>(Result)) {
if (Optional<SVal> V = getObjectUnderConstruction(State, MT, LC)) {
- FoundOriginalMaterializationRegion = true;
- TR = cast<CXXTempObjectRegion>(V->getAsRegion());
- assert(TR);
State = finishObjectConstruction(State, MT, LC);
+ State = State->BindExpr(Result, LC, *V);
+ return State;
} else {
StorageDuration SD = MT->getStorageDuration();
// If this object is bound to a reference with static storage duration, we
@@ -402,46 +409,42 @@
}
}
- if (!FoundOriginalMaterializationRegion) {
- // What remains is to copy the value of the object to the new region.
- // FIXME: In other words, what we should always do is copy value of the
- // Init expression (which corresponds to the bigger object) to the whole
- // temporary region TR. However, this value is often no longer present
- // in the Environment. If it has disappeared, we instead invalidate TR.
- // Still, what we can do is assign the value of expression Ex (which
- // corresponds to the sub-object) to the TR's sub-region Reg. At least,
- // values inside Reg would be correct.
- SVal InitVal = State->getSVal(Init, LC);
- if (InitVal.isUnknown()) {
- InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
- currBldrCtx->blockCount());
- State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
-
- // Then we'd need to take the value that certainly exists and bind it
- // over.
- if (InitValWithAdjustments.isUnknown()) {
- // Try to recover some path sensitivity in case we couldn't
- // compute the value.
- InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
- Result, LC, InitWithAdjustments->getType(),
- currBldrCtx->blockCount());
- }
- State =
- State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
- } else {
- State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
+ // What remains is to copy the value of the object to the new region.
+ // FIXME: In other words, what we should always do is copy value of the
+ // Init expression (which corresponds to the bigger object) to the whole
+ // temporary region TR. However, this value is often no longer present
+ // in the Environment. If it has disappeared, we instead invalidate TR.
+ // Still, what we can do is assign the value of expression Ex (which
+ // corresponds to the sub-object) to the TR's sub-region Reg. At least,
+ // values inside Reg would be correct.
+ SVal InitVal = State->getSVal(Init, LC);
+ if (InitVal.isUnknown()) {
+ InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
+ currBldrCtx->blockCount());
+ State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
+
+ // Then we'd need to take the value that certainly exists and bind it
+ // over.
+ if (InitValWithAdjustments.isUnknown()) {
+ // Try to recover some path sensitivity in case we couldn't
+ // compute the value.
+ InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
+ Result, LC, InitWithAdjustments->getType(),
+ currBldrCtx->blockCount());
}
+ State =
+ State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
+ } else {
+ State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
}
// The result expression would now point to the correct sub-region of the
// newly created temporary region. Do this last in order to getSVal of Init
// correctly in case (Result == Init).
State = State->BindExpr(Result, LC, Reg);
- if (!FoundOriginalMaterializationRegion) {
- // Notify checkers once for two bindLoc()s.
- State = processRegionChange(State, TR, LC);
- }
+ // Notify checkers once for two bindLoc()s.
+ State = processRegionChange(State, TR, LC);
return State;
}
@@ -475,6 +478,30 @@
return State->remove<ObjectsUnderConstruction>(Key);
}
+ProgramStateRef ExprEngine::elideDestructor(ProgramStateRef State,
+ const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC) {
+ ElidedDestructorItem I(BTE, LC);
+ assert(!State->contains<ElidedDestructors>(I));
+ return State->add<ElidedDestructors>(I);
+}
+
+ProgramStateRef
+ExprEngine::cleanupElidedDestructor(ProgramStateRef State,
+ const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC) {
+ ElidedDestructorItem I(BTE, LC);
+ assert(State->contains<ElidedDestructors>(I));
+ return State->remove<ElidedDestructors>(I);
+}
+
+bool ExprEngine::isDestructorElided(ProgramStateRef State,
+ const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC) {
+ ElidedDestructorItem I(BTE, LC);
+ return State->contains<ElidedDestructors>(I);
+}
+
bool ExprEngine::areAllObjectsFullyConstructed(ProgramStateRef State,
const LocationContext *FromLC,
const LocationContext *ToLC) {
@@ -485,6 +512,10 @@
if (I.first.getLocationContext() == LC)
return false;
+ for (auto I: State->get<ElidedDestructors>())
+ if (I.second == LC)
+ return false;
+
LC = LC->getParent();
}
return true;
@@ -529,6 +560,14 @@
Key.print(Out, nullptr, PP);
Out << " : " << Value << NL;
}
+
+ for (auto I : State->get<ElidedDestructors>()) {
+ if (I.second != LC)
+ continue;
+ Out << '(' << I.second << ',' << (const void *)I.first << ") ";
+ I.first->printPretty(Out, nullptr, PP);
+ Out << " : (constructor elided)" << NL;
+ }
}
void ExprEngine::printState(raw_ostream &Out, ProgramStateRef State,
@@ -1003,10 +1042,11 @@
void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
ExplodedNode *Pred,
ExplodedNodeSet &Dst) {
- ExplodedNodeSet CleanDtorState;
- StmtNodeBuilder StmtBldr(Pred, CleanDtorState, *currBldrCtx);
+ const CXXBindTemporaryExpr *BTE = D.getBindTemporaryExpr();
ProgramStateRef State = Pred->getState();
+ const LocationContext *LC = Pred->getLocationContext();
const MemRegion *MR = nullptr;
+
if (Optional<SVal> V =
getObjectUnderConstruction(State, D.getBindTemporaryExpr(),
Pred->getLocationContext())) {
@@ -1017,6 +1057,21 @@
Pred->getLocationContext());
MR = V->getAsRegion();
}
+
+ // If copy elision has occured, and the constructor corresponding to the
+ // destructor was elided, we need to skip the destructor as well.
+ if (isDestructorElided(State, BTE, LC)) {
+ State = cleanupElidedDestructor(State, BTE, LC);
+ NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+ PostImplicitCall PP(D.getDestructorDecl(getContext()),
+ D.getBindTemporaryExpr()->getLocStart(),
+ Pred->getLocationContext());
+ Bldr.generateNode(PP, State, Pred);
+ return;
+ }
+
+ ExplodedNodeSet CleanDtorState;
+ StmtNodeBuilder StmtBldr(Pred, CleanDtorState, *currBldrCtx);
StmtBldr.generateNode(D.getBindTemporaryExpr(), Pred, State);
QualType T = D.getBindTemporaryExpr()->getSubExpr()->getType();
@@ -2200,8 +2255,21 @@
// it must be a separate problem.
assert(isa<CXXBindTemporaryExpr>(I.first.getStmt()));
State = State->remove<ObjectsUnderConstruction>(I.first);
+ // Also cleanup the elided destructor info.
+ ElidedDestructorItem Item(
+ cast<CXXBindTemporaryExpr>(I.first.getStmt()),
+ I.first.getLocationContext());
+ State = State->remove<ElidedDestructors>(Item);
}
+ // Also suppress the assertion for elided destructors when temporary
+ // destructors are not provided at all by the CFG, because there's no
+ // good place to clean them up.
+ if (!AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG())
+ for (auto I : State->get<ElidedDestructors>())
+ if (I.second == LC)
+ State = State->remove<ElidedDestructors>(I);
+
LC = LC->getParent();
}
if (State != Pred->getState()) {
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -780,9 +780,30 @@
llvm::PointerUnion<const Stmt *, const CXXCtorInitializer *> P,
const LocationContext *LC);
+ /// If the given expression corresponds to a temporary that was used for
+ /// passing into an elidable copy/move constructor and that constructor
+ /// was actually elided, track that we also need to elide the destructor.
+ static ProgramStateRef elideDestructor(ProgramStateRef State,
+ const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC);
+
+ /// Stop tracking the destructor that corresponds to an elided constructor.
+ static ProgramStateRef
+ cleanupElidedDestructor(ProgramStateRef State,
+ const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC);
+
+ /// Returns true if the given expression corresponds to a temporary that
+ /// was constructed for passing into an elidable copy/move constructor
+ /// and that constructor was actually elided.
+ static bool isDestructorElided(ProgramStateRef State,
+ const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC);
+
/// Check if all objects under construction have been fully constructed
/// for the given context range (including FromLC, not including ToLC).
- /// This is useful for assertions.
+ /// This is useful for assertions. Also checks if elided destructors
+ /// were cleaned up.
static bool areAllObjectsFullyConstructed(ProgramStateRef State,
const LocationContext *FromLC,
const LocationContext *ToLC);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
