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); // 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 +891,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 +915,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@-4{{UNKNOWN}}
+ // expected-warning@-5{{UNKNOWN}}
+#endif
+#else
+ // expected-warning@-8{{UNKNOWN}}
#endif
} else {
// The destructor is not called on this branch.
@@ -1012,11 +1046,16 @@
#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 +1211,21 @@
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
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -199,12 +199,20 @@
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!");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
