[PATCH] D44239: [analyzer] Re-enable constructor inlining when lifetime extension through fields occurs.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
NoQ added a dependency: D44238: [CFG] Fix automatic destructors when a member
is bound to a reference..
In code like
const int &x = C().x;
destructors were missing in the CFG, so it was unsafe to inline the
constructors, so constructor inlining was disabled in
https://reviews.llvm.org/D43689. But i think i found a way to fix the CFG in
https://reviews.llvm.org/D44238, so it should be safe to re-enable it now.
Repository:
rC Clang
https://reviews.llvm.org/D44239
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/lifetime-extension.cpp
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -42,10 +42,18 @@
const int &y = A().j[1]; // no-crash
const int &z = (A().j[1], A().j[0]); // no-crash
- // FIXME: All of these should be TRUE, but constructors aren't inlined.
- clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(x == 1);
+ clang_analyzer_eval(y == 3);
+ clang_analyzer_eval(z == 2);
+#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
}
} // end namespace pr19539_crash_on_destroying_an_integer
@@ -140,8 +148,12 @@
{
const bool &x = C(true, &after, &before).x; // no-crash
}
- // FIXME: Should be TRUE. Should not warn about garbage value.
- clang_analyzer_eval(after == before); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning@-2{{TRUE}}
+#else
+ // expected-warning@-4{{UNKNOWN}}
+#endif
}
} // end namespace maintain_original_object_address_on_lifetime_extension
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -694,11 +694,6 @@
// the fake temporary target.
if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
return CIP_DisallowedOnce;
-
- // If the temporary is lifetime-extended by binding a smaller object
- // within it to a reference, automatic destructors don't work properly.
- if (CallOpts.IsTemporaryLifetimeExtendedViaSubobject)
-return CIP_DisallowedOnce;
}
break;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -179,18 +179,6 @@
break;
}
case ConstructionContext::TemporaryObjectKind: {
- const auto *TOCC = cast(CC);
- // See if we're lifetime-extended via our field. If so, take a note.
- // Because automatic destructors aren't quite working in this case.
- if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
-if (const ValueDecl *VD = MTE->getExtendingDecl()) {
- assert(VD->getType()->isReferenceType());
- if (VD->getType()->getPointeeType().getCanonicalType() !=
- MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
-CallOpts.IsTemporaryLifetimeExtendedViaSubobject = true;
- }
-}
- }
// TODO: Support temporaries lifetime-extended via static references.
// They'd need a getCXXStaticTempObjectRegion().
CallOpts.IsTemporaryCtorOrDtor = true;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -105,11 +105,6 @@
/// This call is a constructor or a destructor of a temporary value.
bool IsTemporaryCtorOrDtor = false;
-/// This call is a constructor for a temporary that is lifetime-extended
-/// by binding a smaller object within it to a reference, for example
-/// 'const int &x = C().x;'.
-bool IsTemporaryLifetimeExtendedViaSubobject = false;
-
EvalCallOptions() {}
};
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -42,10 +42,18 @@
const int &y = A().j[1]; // no-crash
[PATCH] D44239: [analyzer] Re-enable constructor inlining when lifetime extension through fields occurs.
NoQ updated this revision to Diff 147637.
NoQ added a comment.
Herald added a subscriber: baloghadamsoftware.
Rebase.
https://reviews.llvm.org/D44239
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/lifetime-extension.cpp
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -46,10 +46,18 @@
const int &y = A().j[1]; // no-crash
const int &z = (A().j[1], A().j[0]); // no-crash
- // FIXME: All of these should be TRUE, but constructors aren't inlined.
- clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(x == 1);
+ clang_analyzer_eval(y == 3);
+ clang_analyzer_eval(z == 2);
+#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
}
} // end namespace pr19539_crash_on_destroying_an_integer
@@ -144,8 +152,12 @@
{
const bool &x = C(true, &after, &before).x; // no-crash
}
- // FIXME: Should be TRUE. Should not warn about garbage value.
- clang_analyzer_eval(after == before); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning@-2{{TRUE}}
+#else
+ // expected-warning@-4{{UNKNOWN}}
+#endif
}
struct A { // A is an aggregate.
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -695,12 +695,7 @@
if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
return CIP_DisallowedOnce;
- // If the temporary is lifetime-extended by binding a smaller object
- // within it to a reference, automatic destructors don't work properly.
- if (CallOpts.IsTemporaryLifetimeExtendedViaSubobject)
-return CIP_DisallowedOnce;
-
- // If the temporary is lifetime-extended by binding it to a reference-typ
+ // If the temporary is lifetime-extended by binding it to a reference-type
// field within an aggregate, automatic destructors don't work properly.
if (CallOpts.IsTemporaryLifetimeExtendedViaAggregate)
return CIP_DisallowedOnce;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -182,22 +182,13 @@
const auto *TOCC = cast(CC);
if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
if (const ValueDecl *VD = MTE->getExtendingDecl()) {
- // Pattern-match various forms of lifetime extension that aren't
- // currently supported by the CFG.
- // FIXME: Is there a better way to retrieve this information from
- // the MaterializeTemporaryExpr?
assert(MTE->getStorageDuration() != SD_FullExpression);
- if (VD->getType()->isReferenceType()) {
-assert(VD->getType()->isReferenceType());
-if (VD->getType()->getPointeeType().getCanonicalType() !=
-MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
- // We're lifetime-extended via our field. Automatic destructors
- // aren't quite working in this case.
- CallOpts.IsTemporaryLifetimeExtendedViaSubobject = true;
-}
- } else {
+ if (!VD->getType()->isReferenceType()) {
// We're lifetime-extended by a surrounding aggregate.
-// Automatic destructors aren't quite working in this case.
+// Automatic destructors aren't quite working in this case
+// on the CFG side. We should warn the caller about that.
+// FIXME: Is there a better way to retrieve this information from
+// the MaterializeTemporaryExpr?
CallOpts.IsTemporaryLifetimeExtendedViaAggregate = true;
}
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -106,11 +106,6 @@
bool IsTemporaryCtorOrDtor = false;
/// This call is a constructor for a temporary that is lifetime-extended
-/// by binding a smaller object within it to a reference, for example
-/// 'const int &x = C().x;'.
-bool Is
[PATCH] D44239: [analyzer] Re-enable constructor inlining when lifetime extension through fields occurs.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC333946: [analyzer] Re-enable constructors when lifetime
extension through fields occurs. (authored by dergachev, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D44239
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/lifetime-extension.cpp
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -46,10 +46,18 @@
const int &y = A().j[1]; // no-crash
const int &z = (A().j[1], A().j[0]); // no-crash
- // FIXME: All of these should be TRUE, but constructors aren't inlined.
- clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(x == 1);
+ clang_analyzer_eval(y == 3);
+ clang_analyzer_eval(z == 2);
+#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
}
} // end namespace pr19539_crash_on_destroying_an_integer
@@ -144,8 +152,12 @@
{
const bool &x = C(true, &after, &before).x; // no-crash
}
- // FIXME: Should be TRUE. Should not warn about garbage value.
- clang_analyzer_eval(after == before); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning@-2{{TRUE}}
+#else
+ // expected-warning@-4{{UNKNOWN}}
+#endif
}
struct A { // A is an aggregate.
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -181,22 +181,13 @@
const auto *TOCC = cast(CC);
if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
if (const ValueDecl *VD = MTE->getExtendingDecl()) {
- // Pattern-match various forms of lifetime extension that aren't
- // currently supported by the CFG.
- // FIXME: Is there a better way to retrieve this information from
- // the MaterializeTemporaryExpr?
assert(MTE->getStorageDuration() != SD_FullExpression);
- if (VD->getType()->isReferenceType()) {
-assert(VD->getType()->isReferenceType());
-if (VD->getType()->getPointeeType().getCanonicalType() !=
-MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
- // We're lifetime-extended via our field. Automatic destructors
- // aren't quite working in this case.
- CallOpts.IsTemporaryLifetimeExtendedViaSubobject = true;
-}
- } else {
+ if (!VD->getType()->isReferenceType()) {
// We're lifetime-extended by a surrounding aggregate.
-// Automatic destructors aren't quite working in this case.
+// Automatic destructors aren't quite working in this case
+// on the CFG side. We should warn the caller about that.
+// FIXME: Is there a better way to retrieve this information from
+// the MaterializeTemporaryExpr?
CallOpts.IsTemporaryLifetimeExtendedViaAggregate = true;
}
}
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -696,12 +696,7 @@
if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
return CIP_DisallowedOnce;
- // If the temporary is lifetime-extended by binding a smaller object
- // within it to a reference, automatic destructors don't work properly.
- if (CallOpts.IsTemporaryLifetimeExtendedViaSubobject)
-return CIP_DisallowedOnce;
-
- // If the temporary is lifetime-extended by binding it to a reference-typ
+ // If the temporary is lifetime-extended by binding it to a reference-type
// field within an aggregate, automatic destructors don't work properly.
if (CallOpts.IsTemporaryLifetimeExtendedViaAggregate)
return CIP_DisallowedOnce;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -106,11 +106,6 @@
bool IsTemporaryCtorOrDtor = false;
/// This call is a constructor for a temporary that is lifetime-ext
