[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members
This revision was automatically updated to reflect the committed changes.
Closed by commit rL288263: [analyzer] Construct temporary objects of correct
types, destroy them properly. (authored by dergachev).
Changed prior to commit:
https://reviews.llvm.org/D26839?vs=79540=79783#toc
Repository:
rL LLVM
https://reviews.llvm.org/D26839
Files:
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/test/Analysis/lifetime-extension.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -202,51 +202,70 @@
MemRegionManager = StateMgr.getRegionManager();
StoreManager = StateMgr.getStoreManager();
- // We need to be careful about treating a derived type's value as
- // bindings for a base type. Unless we're creating a temporary pointer region,
- // start by stripping and recording base casts.
- SmallVector Casts;
- const Expr *Inner = Ex->IgnoreParens();
- if (!Loc::isLocType(Result->getType())) {
-while (const CastExpr *CE = dyn_cast(Inner)) {
- if (CE->getCastKind() == CK_DerivedToBase ||
- CE->getCastKind() == CK_UncheckedDerivedToBase)
-Casts.push_back(CE);
- else if (CE->getCastKind() != CK_NoOp)
-break;
+ // MaterializeTemporaryExpr may appear out of place, after a few field and
+ // base-class accesses have been made to the object, even though semantically
+ // it is the whole object that gets materialized and lifetime-extended.
+ //
+ // For example:
+ //
+ // `-MaterializeTemporaryExpr
+ // `-MemberExpr
+ // `-CXXTemporaryObjectExpr
+ //
+ // instead of the more natural
+ //
+ // `-MemberExpr
+ // `-MaterializeTemporaryExpr
+ // `-CXXTemporaryObjectExpr
+ //
+ // Use the usual methods for obtaining the expression of the base object,
+ // and record the adjustments that we need to make to obtain the sub-object
+ // that the whole expression 'Ex' refers to. This trick is usual,
+ // in the sense that CodeGen takes a similar route.
- Inner = CE->getSubExpr()->IgnoreParens();
-}
- }
+ SmallVector CommaLHSs;
+ SmallVector Adjustments;
+
+ const Expr *Init = Ex->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
- // Create a temporary object region for the inner expression (which may have
- // a more derived type) and bind the value into it.
const TypedValueRegion *TR = nullptr;
if (const MaterializeTemporaryExpr *MT =
dyn_cast(Result)) {
StorageDuration SD = MT->getStorageDuration();
// If this object is bound to a reference with static storage duration, we
// put it in a different region to prevent "address leakage" warnings.
if (SD == SD_Static || SD == SD_Thread)
-TR = MRMgr.getCXXStaticTempObjectRegion(Inner);
+ TR = MRMgr.getCXXStaticTempObjectRegion(Init);
}
if (!TR)
-TR = MRMgr.getCXXTempObjectRegion(Inner, LC);
+TR = MRMgr.getCXXTempObjectRegion(Init, LC);
SVal Reg = loc::MemRegionVal(TR);
+ // Make the necessary adjustments to obtain the sub-object.
+ for (auto I = Adjustments.rbegin(), E = Adjustments.rend(); I != E; ++I) {
+const SubobjectAdjustment = *I;
+switch (Adj.Kind) {
+case SubobjectAdjustment::DerivedToBaseAdjustment:
+ Reg = StoreMgr.evalDerivedToBase(Reg, Adj.DerivedToBase.BasePath);
+ break;
+case SubobjectAdjustment::FieldAdjustment:
+ Reg = StoreMgr.getLValueField(Adj.Field, Reg);
+ break;
+case SubobjectAdjustment::MemberPointerAdjustment:
+ // FIXME: Unimplemented.
+ State->bindDefault(Reg, UnknownVal());
+ return State;
+}
+ }
+
+ // Try to recover some path sensitivity in case we couldn't compute the value.
if (V.isUnknown())
V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
currBldrCtx->blockCount());
+ // Bind the value of the expression to the sub-object region, and then bind
+ // the sub-object region to our expression.
State = State->bindLoc(Reg, V);
-
- // Re-apply the casts (from innermost to outermost) for type sanity.
- for (SmallVectorImpl::reverse_iterator I = Casts.rbegin(),
- E = Casts.rend();
- I != E; ++I) {
-Reg = StoreMgr.evalDerivedToBase(Reg, *I);
- }
-
State = State->BindExpr(Result, LC, Reg);
return State;
}
@@ -592,9 +611,9 @@
SVal dest = state->getLValue(varDecl, Pred->getLocationContext());
const MemRegion *Region = dest.castAs().getRegion();
- if (const ReferenceType *refType = varType->getAs()) {
-varType = refType->getPointeeType();
-Region = state->getSVal(Region).getAsRegion();
+ if (varType->isReferenceType()) {
+Region = state->getSVal(Region).getAsRegion()->getBaseRegion();
+varType =
[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members
NoQ updated this revision to Diff 79540.
NoQ marked an inline comment as done.
NoQ added a comment.
Update the comment.
https://reviews.llvm.org/D26839
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/lifetime-extension.cpp
Index: test/Analysis/lifetime-extension.cpp
===
--- /dev/null
+++ test/Analysis/lifetime-extension.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -Wno-unused -std=c++11 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
+struct A {
+ int i;
+ int j[2];
+ A() : i(1) {
+j[0] = 2;
+j[1] = 3;
+ }
+ ~A() {}
+};
+
+void clang_analyzer_eval(bool);
+
+void f() {
+ const int = A().i; // no-crash
+ const int = A().j[1]; // no-crash
+ const int = (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}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -202,51 +202,70 @@
MemRegionManager = StateMgr.getRegionManager();
StoreManager = StateMgr.getStoreManager();
- // We need to be careful about treating a derived type's value as
- // bindings for a base type. Unless we're creating a temporary pointer region,
- // start by stripping and recording base casts.
- SmallVector Casts;
- const Expr *Inner = Ex->IgnoreParens();
- if (!Loc::isLocType(Result->getType())) {
-while (const CastExpr *CE = dyn_cast(Inner)) {
- if (CE->getCastKind() == CK_DerivedToBase ||
- CE->getCastKind() == CK_UncheckedDerivedToBase)
-Casts.push_back(CE);
- else if (CE->getCastKind() != CK_NoOp)
-break;
+ // MaterializeTemporaryExpr may appear out of place, after a few field and
+ // base-class accesses have been made to the object, even though semantically
+ // it is the whole object that gets materialized and lifetime-extended.
+ //
+ // For example:
+ //
+ // `-MaterializeTemporaryExpr
+ // `-MemberExpr
+ // `-CXXTemporaryObjectExpr
+ //
+ // instead of the more natural
+ //
+ // `-MemberExpr
+ // `-MaterializeTemporaryExpr
+ // `-CXXTemporaryObjectExpr
+ //
+ // Use the usual methods for obtaining the expression of the base object,
+ // and record the adjustments that we need to make to obtain the sub-object
+ // that the whole expression 'Ex' refers to. This trick is usual,
+ // in the sense that CodeGen takes a similar route.
- Inner = CE->getSubExpr()->IgnoreParens();
-}
- }
+ SmallVector CommaLHSs;
+ SmallVector Adjustments;
+
+ const Expr *Init = Ex->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
- // Create a temporary object region for the inner expression (which may have
- // a more derived type) and bind the value into it.
const TypedValueRegion *TR = nullptr;
if (const MaterializeTemporaryExpr *MT =
dyn_cast(Result)) {
StorageDuration SD = MT->getStorageDuration();
// If this object is bound to a reference with static storage duration, we
// put it in a different region to prevent "address leakage" warnings.
if (SD == SD_Static || SD == SD_Thread)
-TR = MRMgr.getCXXStaticTempObjectRegion(Inner);
+ TR = MRMgr.getCXXStaticTempObjectRegion(Init);
}
if (!TR)
-TR = MRMgr.getCXXTempObjectRegion(Inner, LC);
+TR = MRMgr.getCXXTempObjectRegion(Init, LC);
SVal Reg = loc::MemRegionVal(TR);
+ // Make the necessary adjustments to obtain the sub-object.
+ for (auto I = Adjustments.rbegin(), E = Adjustments.rend(); I != E; ++I) {
+const SubobjectAdjustment = *I;
+switch (Adj.Kind) {
+case SubobjectAdjustment::DerivedToBaseAdjustment:
+ Reg = StoreMgr.evalDerivedToBase(Reg, Adj.DerivedToBase.BasePath);
+ break;
+case SubobjectAdjustment::FieldAdjustment:
+ Reg = StoreMgr.getLValueField(Adj.Field, Reg);
+ break;
+case SubobjectAdjustment::MemberPointerAdjustment:
+ // FIXME: Unimplemented.
+ State->bindDefault(Reg, UnknownVal());
+ return State;
+}
+ }
+
+ // Try to recover some path sensitivity in case we couldn't compute the value.
if (V.isUnknown())
V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
currBldrCtx->blockCount());
+ // Bind the value of the expression to the sub-object region, and then bind
+ // the sub-object region to our expression.
State = State->bindLoc(Reg, V);
-
- // Re-apply the casts (from innermost to outermost) for type sanity.
- for (SmallVectorImpl::reverse_iterator I = Casts.rbegin(),
- E =
[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
This is great!
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:205
- // We need to be careful about treating a derived type's value as
- // bindings for a base type. Unless we're creating a temporary pointer
region,
- // start by stripping and recording base casts.
- SmallVector Casts;
- const Expr *Inner = Ex->IgnoreParens();
- if (!Loc::isLocType(Result->getType())) {
-while (const CastExpr *CE = dyn_cast(Inner)) {
- if (CE->getCastKind() == CK_DerivedToBase ||
- CE->getCastKind() == CK_UncheckedDerivedToBase)
-Casts.push_back(CE);
- else if (CE->getCastKind() != CK_NoOp)
-break;
-
- Inner = CE->getSubExpr()->IgnoreParens();
-}
- }
+ // MaterializeTemporaryExpr may appear out of place, after a few field and
+ // base-class accesses have been made to the object, even though semantically
I think it would good to show an AST representation in the comment to make it
clear what "out of place" means. This could be a very simplified version of the
dumped AST for a temporary with a field access.
https://reviews.llvm.org/D26839
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members
NoQ marked an inline comment as done.
NoQ added inline comments.
Comment at: test/Analysis/lifetime-extension.cpp:11
+ int j[2];
+ S s;
+ A() : i(1) {
alexshap wrote:
> what is the role of S in this test ?
I copy-pasted this from the original bug report. The only purpose of S is to
provide a non-trivial destructor. I also thought it'd be a nice twist to the
test to make the destructor non-obvious, but it doesn't really matter. Removed.
https://reviews.llvm.org/D26839
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members
NoQ updated this revision to Diff 78478.
https://reviews.llvm.org/D26839
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/lifetime-extension.cpp
Index: test/Analysis/lifetime-extension.cpp
===
--- /dev/null
+++ test/Analysis/lifetime-extension.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -Wno-unused -std=c++11 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
+struct A {
+ int i;
+ int j[2];
+ A() : i(1) {
+j[0] = 2;
+j[1] = 3;
+ }
+ ~A() {}
+};
+
+void clang_analyzer_eval(bool);
+
+void f() {
+ const int = A().i; // no-crash
+ const int = A().j[1]; // no-crash
+ const int = (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}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -202,51 +202,56 @@
MemRegionManager = StateMgr.getRegionManager();
StoreManager = StateMgr.getStoreManager();
- // We need to be careful about treating a derived type's value as
- // bindings for a base type. Unless we're creating a temporary pointer region,
- // start by stripping and recording base casts.
- SmallVector Casts;
- const Expr *Inner = Ex->IgnoreParens();
- if (!Loc::isLocType(Result->getType())) {
-while (const CastExpr *CE = dyn_cast(Inner)) {
- if (CE->getCastKind() == CK_DerivedToBase ||
- CE->getCastKind() == CK_UncheckedDerivedToBase)
-Casts.push_back(CE);
- else if (CE->getCastKind() != CK_NoOp)
-break;
-
- Inner = CE->getSubExpr()->IgnoreParens();
-}
- }
+ // MaterializeTemporaryExpr may appear out of place, after a few field and
+ // base-class accesses have been made to the object, even though semantically
+ // it is the whole object that gets materialized and lifetime-extended.
+ // Use the usual methods for obtaining the expression of the base object,
+ // and record the adjustments that we need to make to obtain the sub-object
+ // that the whole expression 'Ex' refers to. This trick is usual,
+ // in the sense that CodeGen takes a similar route.
+ SmallVector CommaLHSs;
+ SmallVector Adjustments;
+
+ const Expr *Init = Ex->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
- // Create a temporary object region for the inner expression (which may have
- // a more derived type) and bind the value into it.
const TypedValueRegion *TR = nullptr;
if (const MaterializeTemporaryExpr *MT =
dyn_cast(Result)) {
StorageDuration SD = MT->getStorageDuration();
// If this object is bound to a reference with static storage duration, we
// put it in a different region to prevent "address leakage" warnings.
if (SD == SD_Static || SD == SD_Thread)
-TR = MRMgr.getCXXStaticTempObjectRegion(Inner);
+ TR = MRMgr.getCXXStaticTempObjectRegion(Init);
}
if (!TR)
-TR = MRMgr.getCXXTempObjectRegion(Inner, LC);
+TR = MRMgr.getCXXTempObjectRegion(Init, LC);
SVal Reg = loc::MemRegionVal(TR);
+ // Make the necessary adjustments to obtain the sub-object.
+ for (auto I = Adjustments.rbegin(), E = Adjustments.rend(); I != E; ++I) {
+const SubobjectAdjustment = *I;
+switch (Adj.Kind) {
+case SubobjectAdjustment::DerivedToBaseAdjustment:
+ Reg = StoreMgr.evalDerivedToBase(Reg, Adj.DerivedToBase.BasePath);
+ break;
+case SubobjectAdjustment::FieldAdjustment:
+ Reg = StoreMgr.getLValueField(Adj.Field, Reg);
+ break;
+case SubobjectAdjustment::MemberPointerAdjustment:
+ // FIXME: Unimplemented.
+ State->bindDefault(Reg, UnknownVal());
+ return State;
+}
+ }
+
+ // Try to recover some path sensitivity in case we couldn't compute the value.
if (V.isUnknown())
V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
currBldrCtx->blockCount());
+ // Bind the value of the expression to the sub-object region, and then bind
+ // the sub-object region to our expression.
State = State->bindLoc(Reg, V);
-
- // Re-apply the casts (from innermost to outermost) for type sanity.
- for (SmallVectorImpl::reverse_iterator I = Casts.rbegin(),
- E = Casts.rend();
- I != E; ++I) {
-Reg = StoreMgr.evalDerivedToBase(Reg, *I);
- }
-
State = State->BindExpr(Result, LC, Reg);
return State;
}
@@ -596,9 +601,9 @@
SVal dest = state->getLValue(varDecl, Pred->getLocationContext());
const MemRegion *Region = dest.castAs().getRegion();
- if (const ReferenceType *refType =
[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members
alexshap added inline comments.
Comment at: test/Analysis/lifetime-extension.cpp:11
+ int j[2];
+ S s;
+ A() : i(1) {
what is the role of S in this test ?
https://reviews.llvm.org/D26839
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members
NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun, a.sidorin.
NoQ added subscribers: nandor, cfe-commits.
1. Re-use approach used in codegen. `MaterializeTemporaryExpr` may be
positioned in a strange manner, above the member access to the temporary, which
makes it a bit tricky to find the expression that actually corresponds to the
temporary object. FIXME: Hmm, probably we should re-use this approach in CFG as
well (cf. https://reviews.llvm.org/D22419).
2. Create the temporary region that corresponds to the full temporary object,
rather than to the sub-object. This was a bug.
3. If lifetime extension occurs, use the temporary object region's
path-sensitive type, rather than reference variable type, in order to call the
correct destructor.
https://reviews.llvm.org/D26839
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/lifetime-extension.cpp
Index: test/Analysis/lifetime-extension.cpp
===
--- /dev/null
+++ test/Analysis/lifetime-extension.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -Wno-unused -std=c++11 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
+struct S {
+ ~S() {
+ }
+};
+
+struct A {
+ int i;
+ int j[2];
+ S s;
+ A() : i(1) {
+j[0] = 2;
+j[1] = 3;
+ }
+};
+
+void clang_analyzer_eval(bool);
+
+void f() {
+ const int = A().i; // no-crash
+ const int = A().j[1]; // no-crash
+ const int = (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}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -202,51 +202,56 @@
MemRegionManager = StateMgr.getRegionManager();
StoreManager = StateMgr.getStoreManager();
- // We need to be careful about treating a derived type's value as
- // bindings for a base type. Unless we're creating a temporary pointer region,
- // start by stripping and recording base casts.
- SmallVector Casts;
- const Expr *Inner = Ex->IgnoreParens();
- if (!Loc::isLocType(Result->getType())) {
-while (const CastExpr *CE = dyn_cast(Inner)) {
- if (CE->getCastKind() == CK_DerivedToBase ||
- CE->getCastKind() == CK_UncheckedDerivedToBase)
-Casts.push_back(CE);
- else if (CE->getCastKind() != CK_NoOp)
-break;
-
- Inner = CE->getSubExpr()->IgnoreParens();
-}
- }
+ // MaterializeTemporaryExpr may appear out of place, after a few field and
+ // base-class accesses have been made to the object, even though semantically
+ // it is the whole object that gets materialized and lifetime-extended.
+ // Use the usual methods for obtaining the expression of the base object,
+ // and record the adjustments that we need to make to obtain the sub-object
+ // that the whole expression 'Ex' refers to. This trick is usual,
+ // in the sense that CodeGen takes a similar route.
+ SmallVector CommaLHSs;
+ SmallVector Adjustments;
+
+ const Expr *Init = Ex->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
- // Create a temporary object region for the inner expression (which may have
- // a more derived type) and bind the value into it.
const TypedValueRegion *TR = nullptr;
if (const MaterializeTemporaryExpr *MT =
dyn_cast(Result)) {
StorageDuration SD = MT->getStorageDuration();
// If this object is bound to a reference with static storage duration, we
// put it in a different region to prevent "address leakage" warnings.
if (SD == SD_Static || SD == SD_Thread)
-TR = MRMgr.getCXXStaticTempObjectRegion(Inner);
+ TR = MRMgr.getCXXStaticTempObjectRegion(Init);
}
if (!TR)
-TR = MRMgr.getCXXTempObjectRegion(Inner, LC);
+TR = MRMgr.getCXXTempObjectRegion(Init, LC);
SVal Reg = loc::MemRegionVal(TR);
+ // Make the necessary adjustments to obtain the sub-object.
+ for (auto I = Adjustments.rbegin(), E = Adjustments.rend(); I != E; ++I) {
+const SubobjectAdjustment = *I;
+switch (Adj.Kind) {
+case SubobjectAdjustment::DerivedToBaseAdjustment:
+ Reg = StoreMgr.evalDerivedToBase(Reg, Adj.DerivedToBase.BasePath);
+ break;
+case SubobjectAdjustment::FieldAdjustment:
+ Reg = StoreMgr.getLValueField(Adj.Field, Reg);
+ break;
+case SubobjectAdjustment::MemberPointerAdjustment:
+ // FIXME: Unimplemented.
+ State->bindDefault(Reg, UnknownVal());
+ return State;
+}
+ }
+
+ // Try to recover some path sensitivity in case we couldn't compute the value.
if (V.isUnknown())
V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
