Fold dangling-field warning into general initialization lifetime checks.
This reinstates r337627, reverted in r337671, with a fix to correctly
handle the lvalueness of array subscript expressions on pointers.
llvm-svn: 337726
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index f94eabd..6c2c6d1 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -6227,7 +6227,7 @@
/// Determine the declaration which an initialized entity ultimately refers to,
/// for the purpose of lifetime-extending a temporary bound to a reference in
/// the initialization of \p Entity.
-static LifetimeResult getEntityForTemporaryLifetimeExtension(
+static LifetimeResult getEntityLifetime(
const InitializedEntity *Entity,
const InitializedEntity *InitField = nullptr) {
// C++11 [class.temporary]p5:
@@ -6239,8 +6239,7 @@
case InitializedEntity::EK_Member:
// For subobjects, we look at the complete object.
if (Entity->getParent())
- return getEntityForTemporaryLifetimeExtension(Entity->getParent(),
- Entity);
+ return getEntityLifetime(Entity->getParent(), Entity);
// except:
// C++17 [class.base.init]p8:
@@ -6291,14 +6290,12 @@
case InitializedEntity::EK_ArrayElement:
// For subobjects, we look at the complete object.
- return getEntityForTemporaryLifetimeExtension(Entity->getParent(),
- InitField);
+ return getEntityLifetime(Entity->getParent(), InitField);
case InitializedEntity::EK_Base:
// For subobjects, we look at the complete object.
if (Entity->getParent())
- return getEntityForTemporaryLifetimeExtension(Entity->getParent(),
- InitField);
+ return getEntityLifetime(Entity->getParent(), InitField);
return {InitField, LK_MemInitializer};
case InitializedEntity::EK_Delegating:
@@ -6311,46 +6308,61 @@
case InitializedEntity::EK_BlockElement:
case InitializedEntity::EK_LambdaToBlockConversionBlockElement:
case InitializedEntity::EK_LambdaCapture:
- case InitializedEntity::EK_Exception:
case InitializedEntity::EK_VectorElement:
case InitializedEntity::EK_ComplexElement:
return {nullptr, LK_FullExpression};
+
+ case InitializedEntity::EK_Exception:
+ // FIXME: Can we diagnose lifetime problems with exceptions?
+ return {nullptr, LK_FullExpression};
}
llvm_unreachable("unknown entity kind");
}
namespace {
-enum ExtensionKind {
+enum ReferenceKind {
/// Lifetime would be extended by a reference binding to a temporary.
- EK_ReferenceBinding,
+ RK_ReferenceBinding,
/// Lifetime would be extended by a std::initializer_list object binding to
/// its backing array.
- EK_StdInitializerList,
+ RK_StdInitializerList,
};
-using IndirectTemporaryPathEntry =
- llvm::PointerUnion<CXXDefaultInitExpr *, ValueDecl *>;
-using IndirectTemporaryPath = llvm::SmallVectorImpl<IndirectTemporaryPathEntry>;
+
+/// A temporary or local variable.
+using Local = llvm::PointerUnion<MaterializeTemporaryExpr*, ValueDecl*>;
+
+/// Expressions we stepped over when looking for the local state. Any steps
+/// that would inhibit lifetime extension or take us out of subexpressions of
+/// the initializer are included.
+struct IndirectLocalPathEntry {
+ enum {
+ DefaultInit,
+ AddressOf,
+ } Kind;
+ Expr *E;
+};
+
+using IndirectLocalPath = llvm::SmallVectorImpl<IndirectLocalPathEntry>;
struct RevertToOldSizeRAII {
- IndirectTemporaryPath &Path;
+ IndirectLocalPath &Path;
unsigned OldSize = Path.size();
- RevertToOldSizeRAII(IndirectTemporaryPath &Path) : Path(Path) {}
+ RevertToOldSizeRAII(IndirectLocalPath &Path) : Path(Path) {}
~RevertToOldSizeRAII() { Path.resize(OldSize); }
};
}
-template <typename TemporaryVisitor>
-static void visitTemporariesExtendedByInitializer(IndirectTemporaryPath &Path,
- Expr *Init,
- TemporaryVisitor Visit);
+template <typename LocalVisitor>
+static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
+ Expr *Init, LocalVisitor Visit,
+ bool RevisitSubinits);
-/// Visit the temporaries whose lifetimes would be extended by binding a
-/// reference to the glvalue expression \c Init.
-template <typename TemporaryVisitor>
-static void
-visitTemporariesExtendedByReferenceBinding(IndirectTemporaryPath &Path,
- Expr *Init, ExtensionKind EK,
- TemporaryVisitor Visit) {
+/// Visit the locals that would be reachable through a reference bound to the
+/// glvalue expression \c Init.
+template <typename LocalVisitor>
+static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
+ Expr *Init, ReferenceKind RK,
+ LocalVisitor Visit) {
RevertToOldSizeRAII RAII(Path);
// Walk past any constructs which we can lifetime-extend across.
@@ -6375,14 +6387,18 @@
Init = CE->getSubExpr();
// Per the current approach for DR1299, look through array element access
- // when performing lifetime extension.
- if (auto *ASE = dyn_cast<ArraySubscriptExpr>(Init))
- Init = ASE->getBase();
+ // on array glvalues when performing lifetime extension.
+ if (auto *ASE = dyn_cast<ArraySubscriptExpr>(Init)) {
+ auto *ICE = dyn_cast<ImplicitCastExpr>(ASE->getBase());
+ if (!ICE || ICE->getCastKind() != CK_ArrayToPointerDecay)
+ return;
+ Init = ICE->getSubExpr();
+ }
// Step into CXXDefaultInitExprs so we can diagnose cases where a
// constructor inherits one as an implicit mem-initializer.
if (auto *DIE = dyn_cast<CXXDefaultInitExpr>(Init)) {
- Path.push_back(DIE);
+ Path.push_back({IndirectLocalPathEntry::DefaultInit, DIE});
Init = DIE->getExpr();
if (auto *EWC = dyn_cast<ExprWithCleanups>(Init))
@@ -6391,25 +6407,36 @@
} while (Init != Old);
if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init)) {
- if (Visit(Path, MTE, EK))
- visitTemporariesExtendedByInitializer(Path, MTE->GetTemporaryExpr(),
- Visit);
+ if (Visit(Path, Local(MTE), RK))
+ visitLocalsRetainedByInitializer(Path, MTE->GetTemporaryExpr(), Visit,
+ true);
+ }
+
+ // If we find the name of a local non-reference parameter, we could have a
+ // lifetime problem.
+ if (auto *DRE = dyn_cast<DeclRefExpr>(Init->IgnoreParens())) {
+ auto *VD = dyn_cast<VarDecl>(DRE->getDecl());
+ if (VD && VD->hasLocalStorage() &&
+ !DRE->refersToEnclosingVariableOrCapture()) {
+ // FIXME: Recurse to the initializer of a local reference.
+ if (!VD->getType()->isReferenceType())
+ Visit(Path, Local(VD), RK);
+ }
}
}
-/// Visit the temporaries whose lifetimes would be extended by
-/// lifetime-extending the object initialized by the prvalue expression \c
-/// Init.
-template <typename TemporaryVisitor>
-static void visitTemporariesExtendedByInitializer(IndirectTemporaryPath &Path,
- Expr *Init,
- TemporaryVisitor Visit) {
+/// Visit the locals that would be reachable through an object initialized by
+/// the prvalue expression \c Init.
+template <typename LocalVisitor>
+static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
+ Expr *Init, LocalVisitor Visit,
+ bool RevisitSubinits) {
RevertToOldSizeRAII RAII(Path);
// Step into CXXDefaultInitExprs so we can diagnose cases where a
// constructor inherits one as an implicit mem-initializer.
if (auto *DIE = dyn_cast<CXXDefaultInitExpr>(Init)) {
- Path.push_back(DIE);
+ Path.push_back({IndirectLocalPathEntry::DefaultInit, DIE});
Init = DIE->getExpr();
if (auto *EWC = dyn_cast<ExprWithCleanups>(Init))
@@ -6426,17 +6453,24 @@
// initializing an initializer_list object from the array extends the
// lifetime of the array exactly like binding a reference to a temporary.
if (auto *ILE = dyn_cast<CXXStdInitializerListExpr>(Init))
- return visitTemporariesExtendedByReferenceBinding(
- Path, ILE->getSubExpr(), EK_StdInitializerList, Visit);
+ return visitLocalsRetainedByReferenceBinding(Path, ILE->getSubExpr(),
+ RK_StdInitializerList, Visit);
if (InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) {
+ // We already visited the elements of this initializer list while
+ // performing the initialization. Don't visit them again unless we've
+ // changed the lifetime of the initialized entity.
+ if (!RevisitSubinits)
+ return;
+
if (ILE->isTransparent())
- return visitTemporariesExtendedByInitializer(Path, ILE->getInit(0),
- Visit);
+ return visitLocalsRetainedByInitializer(Path, ILE->getInit(0), Visit,
+ RevisitSubinits);
if (ILE->getType()->isArrayType()) {
for (unsigned I = 0, N = ILE->getNumInits(); I != N; ++I)
- visitTemporariesExtendedByInitializer(Path, ILE->getInit(I), Visit);
+ visitLocalsRetainedByInitializer(Path, ILE->getInit(I), Visit,
+ RevisitSubinits);
return;
}
@@ -6448,8 +6482,8 @@
// bound to temporaries, those temporaries are also lifetime-extended.
if (RD->isUnion() && ILE->getInitializedFieldInUnion() &&
ILE->getInitializedFieldInUnion()->getType()->isReferenceType())
- visitTemporariesExtendedByReferenceBinding(Path, ILE->getInit(0),
- EK_ReferenceBinding, Visit);
+ visitLocalsRetainedByReferenceBinding(Path, ILE->getInit(0),
+ RK_ReferenceBinding, Visit);
else {
unsigned Index = 0;
for (const auto *I : RD->fields()) {
@@ -6459,25 +6493,38 @@
continue;
Expr *SubInit = ILE->getInit(Index);
if (I->getType()->isReferenceType())
- visitTemporariesExtendedByReferenceBinding(
- Path, SubInit, EK_ReferenceBinding, Visit);
+ visitLocalsRetainedByReferenceBinding(Path, SubInit,
+ RK_ReferenceBinding, Visit);
else
// This might be either aggregate-initialization of a member or
// initialization of a std::initializer_list object. Regardless,
// we should recursively lifetime-extend that initializer.
- visitTemporariesExtendedByInitializer(Path, SubInit, Visit);
+ visitLocalsRetainedByInitializer(Path, SubInit, Visit,
+ RevisitSubinits);
++Index;
}
}
}
+ return;
+ }
+
+ // If the initializer is the address of a local, we could have a lifetime
+ // problem.
+ if (auto *Op = dyn_cast<UnaryOperator>(Init->IgnoreParenImpCasts())) {
+ if (Op->getOpcode() == UO_AddrOf) {
+ Path.push_back({IndirectLocalPathEntry::AddressOf, Op});
+ Init = Op->getSubExpr();
+ return visitLocalsRetainedByReferenceBinding(Path, Init,
+ RK_ReferenceBinding, Visit);
+ }
}
}
/// Determine whether this is an indirect path to a temporary that we are
/// supposed to lifetime-extend along (but don't).
-static bool shouldLifetimeExtendThroughPath(const IndirectTemporaryPath &Path) {
+static bool shouldLifetimeExtendThroughPath(const IndirectLocalPath &Path) {
for (auto Elem : Path) {
- if (!Elem.is<CXXDefaultInitExpr*>())
+ if (Elem.Kind != IndirectLocalPathEntry::DefaultInit)
return false;
}
return true;
@@ -6485,7 +6532,7 @@
void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
Expr *Init) {
- LifetimeResult LR = getEntityForTemporaryLifetimeExtension(&Entity);
+ LifetimeResult LR = getEntityLifetime(&Entity);
LifetimeKind LK = LR.getInt();
const InitializedEntity *ExtendingEntity = LR.getPointer();
@@ -6494,9 +6541,54 @@
if (LK == LK_FullExpression)
return;
- auto TemporaryVisitor = [&](IndirectTemporaryPath &Path,
- MaterializeTemporaryExpr *MTE,
- ExtensionKind EK) -> bool {
+ auto TemporaryVisitor = [&](IndirectLocalPath &Path, Local L,
+ ReferenceKind RK) -> bool {
+ // If we found a path to a local variable or similar, check whether the
+ // initialized object will outlive it.
+ if (auto *VD = L.dyn_cast<ValueDecl*>()) {
+ switch (LK) {
+ case LK_FullExpression:
+ llvm_unreachable("already handled this");
+
+ case LK_Extended:
+ break;
+
+ case LK_MemInitializer: {
+ // Paths via a default initializer can only occur during error recovery
+ // (there's no other way that a default initializer can refer to a
+ // local). Don't produce a bogus warning on those cases.
+ if (std::any_of(Path.begin(), Path.end(), [](IndirectLocalPathEntry E) {
+ return E.Kind == IndirectLocalPathEntry::DefaultInit;
+ }))
+ break;
+
+ if (auto *Member =
+ ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) {
+ bool AddressTaken =
+ !Path.empty() &&
+ Path.back().Kind == IndirectLocalPathEntry::AddressOf;
+ Diag(Init->getExprLoc(),
+ AddressTaken ? diag::warn_init_ptr_member_to_parameter_addr
+ : diag::warn_bind_ref_member_to_parameter)
+ << Member << VD << isa<ParmVarDecl>(VD) << Init->getSourceRange();
+ Diag(Member->getLocation(),
+ diag::note_ref_or_ptr_member_declared_here)
+ << (unsigned)AddressTaken;
+ }
+ break;
+ }
+
+ case LK_New:
+ break;
+
+ case LK_Return:
+ // FIXME: Move -Wreturn-stack-address checks here.
+ return false;
+ }
+ return false;
+ }
+
+ auto *MTE = L.get<MaterializeTemporaryExpr*>();
switch (LK) {
case LK_FullExpression:
llvm_unreachable("already handled this");
@@ -6520,11 +6612,11 @@
// would be to clone the initializer expression on each use that would
// lifetime extend its temporaries.
Diag(MTE->getExprLoc(),
- EK == EK_ReferenceBinding
+ RK == RK_ReferenceBinding
? diag::warn_default_member_init_temporary_not_extended
: diag::warn_default_member_init_init_list_not_extended);
} else {
- llvm_unreachable("unexpected indirect temporary path");
+ // FIXME: Warn on this.
}
break;
@@ -6532,18 +6624,20 @@
// Under C++ DR1696, if a mem-initializer (or a default member
// initializer used by the absence of one) would lifetime-extend a
// temporary, the program is ill-formed.
- if (auto *ExtendingDecl = ExtendingEntity->getDecl()) {
+ if (auto *ExtendingDecl =
+ ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) {
bool IsSubobjectMember = ExtendingEntity != &Entity;
Diag(MTE->getExprLoc(), diag::err_bind_ref_member_to_temporary)
<< ExtendingDecl << Init->getSourceRange() << IsSubobjectMember
- << EK;
+ << RK;
// Don't bother adding a note pointing to the field if we're inside its
// default member initializer; our primary diagnostic points to the
// same place in that case.
- if (Path.empty() || !Path.back().is<CXXDefaultInitExpr*>()) {
+ if (Path.empty() ||
+ Path.back().Kind != IndirectLocalPathEntry::DefaultInit) {
Diag(ExtendingDecl->getLocation(),
diag::note_lifetime_extending_member_declared_here)
- << EK << IsSubobjectMember;
+ << RK << IsSubobjectMember;
}
} else {
// We have a mem-initializer but no particular field within it; this
@@ -6557,7 +6651,7 @@
break;
case LK_New:
- if (EK == EK_ReferenceBinding) {
+ if (RK == RK_ReferenceBinding) {
Diag(MTE->getExprLoc(), diag::warn_new_dangling_reference);
} else {
Diag(MTE->getExprLoc(), diag::warn_new_dangling_initializer_list)
@@ -6573,9 +6667,14 @@
// FIXME: Model these as CodeSynthesisContexts to fix the note emission
// order.
for (auto Elem : llvm::reverse(Path)) {
- if (auto *DIE = Elem.dyn_cast<CXXDefaultInitExpr*>()) {
- Diag(DIE->getExprLoc(), diag::note_in_default_member_initalizer_here)
- << DIE->getField();
+ switch (Elem.Kind) {
+ case IndirectLocalPathEntry::DefaultInit:
+ Diag(Elem.E->getExprLoc(), diag::note_in_default_member_initalizer_here)
+ << cast<CXXDefaultInitExpr>(Elem.E)->getField();
+ break;
+
+ case IndirectLocalPathEntry::AddressOf:
+ break;
}
}
@@ -6584,12 +6683,12 @@
return false;
};
- llvm::SmallVector<IndirectTemporaryPathEntry, 8> Path;
+ llvm::SmallVector<IndirectLocalPathEntry, 8> Path;
if (Init->isGLValue())
- visitTemporariesExtendedByReferenceBinding(Path, Init, EK_ReferenceBinding,
- TemporaryVisitor);
+ visitLocalsRetainedByReferenceBinding(Path, Init, RK_ReferenceBinding,
+ TemporaryVisitor);
else
- visitTemporariesExtendedByInitializer(Path, Init, TemporaryVisitor);
+ visitLocalsRetainedByInitializer(Path, Init, TemporaryVisitor, false);
}
static void DiagnoseNarrowingInInitList(Sema &S,
@@ -6853,6 +6952,7 @@
// Diagnose cases where we initialize a pointer to an array temporary, and the
// pointer obviously outlives the temporary.
+ // FIXME: Fold this into checkInitializerLifetime.
if (Args.size() == 1 && Args[0]->getType()->isArrayType() &&
Entity.getType()->isPointerType() &&
InitializedEntityOutlivesFullExpression(Entity)) {
@@ -7015,11 +7115,6 @@
}
}
- // Even though we didn't materialize a temporary, the binding may still
- // extend the lifetime of a temporary. This happens if we bind a
- // reference to the result of a cast to reference type.
- S.checkInitializerLifetime(Entity, CurInit.get());
-
CheckForNullPointerDereference(S, CurInit.get());
break;
@@ -7036,10 +7131,6 @@
Step->Type, CurInit.get(), Entity.getType()->isLValueReferenceType());
CurInit = MTE;
- // Maybe lifetime-extend the temporary's subobjects to match the
- // entity's lifetime.
- S.checkInitializerLifetime(Entity, CurInit.get());
-
// If we're extending this temporary to automatic storage duration -- we
// need to register its cleanup during the full-expression's cleanups.
if (MTE->getStorageDuration() == SD_Automatic &&
@@ -7490,10 +7581,6 @@
// Wrap it in a construction of a std::initializer_list<T>.
CurInit = new (S.Context) CXXStdInitializerListExpr(Step->Type, MTE);
- // Maybe lifetime-extend the array temporary's subobjects to match the
- // entity's lifetime.
- S.checkInitializerLifetime(Entity, CurInit.get());
-
// Bind the result, in case the library has given initializer_list a
// non-trivial destructor.
if (shouldBindAsTemporary(Entity))
@@ -7612,6 +7699,11 @@
}
}
+ // Check whether the initializer has a shorter lifetime than the initialized
+ // entity, and if not, either lifetime-extend or warn as appropriate.
+ if (auto *Init = CurInit.get())
+ S.checkInitializerLifetime(Entity, Init);
+
// Diagnose non-fatal problems with the completed initialization.
if (Entity.getKind() == InitializedEntity::EK_Member &&
cast<FieldDecl>(Entity.getDecl())->isBitField())