NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
This is assertion removal. The assertion was saying that a function's return
value is always a temporary object - which is not true when copy elision is
happening, C++17 mandatory copy elision in particular (we don't support other
forms of copy elision yet, but we have no choice but to support this one).
Repository:
rC Clang
https://reviews.llvm.org/D44755
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/lifetime-extension.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1,6 +1,9 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
+
+// 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();
Index: test/Analysis/lifetime-extension.cpp
===================================================================
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -1,7 +1,11 @@
// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify %s
// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++17 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES %s
// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -DMOVES -verify %s
// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -DMOVES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++17 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -DMOVES %s
+
+// Note: The C++17 run-lines don't -verify yet - it is a no-crash test.
void clang_analyzer_eval(bool);
void clang_analyzer_checkInlined(bool);
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -203,6 +203,10 @@
// 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?
+ // TODO: We assume that the call site has a temporary object construction
+ // context. This is no longer true in C++17 or when copy elision is
+ // performed. We may need to unwrap multiple stack frames here and we
+ // won't necessarily end up with a temporary at the end.
const LocationContext *TempLCtx = LCtx;
if (const LocationContext *CallerLCtx =
LCtx->getCurrentStackFrame()->getParent()) {
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -455,29 +455,48 @@
const LocationContext *LC, const MemRegion *R) {
const CXXBindTemporaryExpr *BTE = nullptr;
const MaterializeTemporaryExpr *MTE = nullptr;
- const LocationContext *TempLC = LC;
if (CC) {
- // In case of temporary object construction, extract data necessary for
- // destruction and lifetime extension.
- const auto *TCC = dyn_cast<TemporaryObjectConstructionContext>(CC);
-
// If the temporary is being returned from the function, it will be
// destroyed or lifetime-extended in the caller stack frame.
if (isa<ReturnedValueConstructionContext>(CC)) {
const StackFrameContext *SFC = LC->getCurrentStackFrame();
assert(SFC);
- if (SFC->getParent()) {
- TempLC = SFC->getParent();
- const CFGElement &CallElem =
- (*SFC->getCallSiteBlock())[SFC->getIndex()];
- if (auto RTCElem = CallElem.getAs<CFGCXXRecordTypedCall>()) {
- TCC = cast<TemporaryObjectConstructionContext>(
- RTCElem->getConstructionContext());
- }
+ LC = SFC->getParent();
+ if (!LC) {
+ // We are on the top frame. We won't ever need any info
+ // for this temporary, so don't set anything.
+ return State;
+ }
+ const CFGElement &CallElem =
+ (*SFC->getCallSiteBlock())[SFC->getIndex()];
+ auto RTCElem = CallElem.getAs<CFGCXXRecordTypedCall>();
+ if (!RTCElem) {
+ // We have a parent stack frame, but no construction context for the
+ // return value. Give up until we provide the construction context
+ // at the call site.
+ return State;
}
+ CC = dyn_cast<TemporaryObjectConstructionContext>(
+ RTCElem->getConstructionContext());
+ if (!CC) {
+ // TODO: We are not returning an object into a temporary. There must
+ // be copy elision happening at the call site. We still need to
+ // explicitly support the situation when the return value is put
+ // into another return statement, i.e.
+ // ReturnedValueConstructionContexts are chained through multiple
+ // stack frames before finally settling in a temporary.
+ // We don't seem to need to explicitly support construction into
+ // a variable after a return.
+ return State;
+ }
+ // Proceed to deal with the temporary we've found on the parent
+ // stack frame.
}
- if (TCC) {
+
+ // In case of temporary object construction, extract data necessary for
+ // destruction and lifetime extension.
+ if (const auto *TCC = dyn_cast<TemporaryObjectConstructionContext>(CC)) {
if (AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG()) {
BTE = TCC->getCXXBindTemporaryExpr();
MTE = TCC->getMaterializedTemporaryExpr();
@@ -496,12 +515,12 @@
}
if (BTE) {
- State = addInitializedTemporary(State, BTE, TempLC,
+ State = addInitializedTemporary(State, BTE, LC,
cast<CXXTempObjectRegion>(R));
}
if (MTE) {
- State = addTemporaryMaterialization(State, MTE, TempLC,
+ State = addTemporaryMaterialization(State, MTE, LC,
cast<CXXTempObjectRegion>(R));
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
