r349696 - [analyzer] Improve modeling for returning an object from the top frame with RVO.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 19 15:14:07 PST 2018
Author: dergachev
Date: Wed Dec 19 15:14:06 2018
New Revision: 349696
URL: http://llvm.org/viewvc/llvm-project?rev=349696&view=rev
Log:
[analyzer] Improve modeling for returning an object from the top frame with RVO.
Static Analyzer processes the program function-by-function, sometimes diving
into other functions ("inlining" them). When an object is returned from an
inlined function, Return Value Optimization is modeled, and the returned object
is constructed at its return location directly.
When an object is returned from the function from which the analysis has started
(the top stack frame of the analysis), the return location is unknown. Model it
with a SymbolicRegion based on a conjured symbol that is specifically tagged for
that purpose, because this is generally the correct way to symbolicate
unknown locations in Static Analyzer.
Fixes leak false positives when an object is returned from top frame in C++17:
objects that are put into a SymbolicRegion-based memory region automatically
"escape" and no longer get reported as leaks. This only applies to C++17 return
values with destructors, because it produces a redundant CXXBindTemporaryExpr
in the call site, which confuses our liveness analysis. The actual fix
for liveness analysis is still pending, but it is no longer causing problems.
Additionally, re-enable temporary destructor tests in C++17.
Differential Revision: https://reviews.llvm.org/D55804
rdar://problem/46217550
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
cfe/trunk/test/Analysis/temporaries.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp?rev=349696&r1=349695&r2=349696&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Wed Dec 19 15:14:06 2018
@@ -113,7 +113,9 @@ SVal ExprEngine::makeZeroElementRegion(P
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.
@@ -139,7 +141,7 @@ std::pair<ProgramStateRef, SVal> ExprEng
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;
@@ -199,12 +201,25 @@ std::pair<ProgramStateRef, SVal> ExprEng
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!");
Modified: cfe/trunk/test/Analysis/temporaries.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/temporaries.cpp?rev=349696&r1=349695&r2=349696&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/temporaries.cpp (original)
+++ cfe/trunk/test/Analysis/temporaries.cpp Wed Dec 19 15:14:06 2018
@@ -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 @@ namespace destructors {
}
#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 @@ void test_ternary_temporary_with_copy(in
// 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 @@ void test_ternary_temporary_with_copy(in
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 @@ class C {
public:
~C() {
glob = 1;
+ // FIXME: Why is destructor not inlined in C++17
clang_analyzer_checkInlined(true);
#ifdef TEMPORARY_DTORS
- // expected-warning at -2{{TRUE}}
+#if __cplusplus < 201703L
+ // expected-warning at -3{{TRUE}}
+#endif
#endif
}
};
@@ -886,11 +924,16 @@ void test(int coin) {
// 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 at -2{{TRUE}}
+#if __cplusplus < 201703L
+ // expected-warning at -3{{TRUE}}
#else
- // expected-warning at -4{{UNKNOWN}}
+ // expected-warning at -5{{UNKNOWN}}
+#endif
+#else
+ // expected-warning at -8{{UNKNOWN}}
#endif
} else {
// The destructor is not called on this branch.
@@ -1012,11 +1055,16 @@ void foo(void (*bar4)(S)) {
#endif
bar2(S(2));
+ // FIXME: Why are we losing information in C++17?
clang_analyzer_eval(glob == 2);
#ifdef TEMPORARY_DTORS
- // expected-warning at -2{{TRUE}}
+#if __cplusplus < 201703L
+ // expected-warning at -3{{TRUE}}
#else
- // expected-warning at -4{{UNKNOWN}}
+ // expected-warning at -5{{UNKNOWN}}
+#endif
+#else
+ // expected-warning at -8{{UNKNOWN}}
#endif
C *c = new D();
@@ -1172,3 +1220,29 @@ void test() {
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
More information about the cfe-commits
mailing list