From 0e3102d1dc043d8626c07112f5f64ae94ec59b66 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 24 Jul 2018 00:55:08 +0000 Subject: [PATCH] Warn if a local variable's initializer retains a pointer/reference to a non-lifetime-extended temporary object. llvm-svn: 337790 --- clang/include/clang/Basic/DiagnosticGroups.td | 5 +- clang/include/clang/Basic/DiagnosticSemaKinds.td | 38 ++++---- clang/lib/Sema/SemaInit.cpp | 110 ++++++++--------------- clang/test/CXX/drs/dr16xx.cpp | 4 +- clang/test/SemaCXX/address-of-temporary.cpp | 19 ++-- clang/test/SemaCXX/warn-dangling-local.cpp | 20 +++++ 6 files changed, 96 insertions(+), 100 deletions(-) create mode 100644 clang/test/SemaCXX/warn-dangling-local.cpp diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index ab0a556..7087db7 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -273,6 +273,10 @@ def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">; def DanglingElse: DiagGroup<"dangling-else">; def DanglingField : DiagGroup<"dangling-field">; def DanglingInitializerList : DiagGroup<"dangling-initializer-list">; +def ReturnStackAddress : DiagGroup<"return-stack-address">; +def Dangling : DiagGroup<"dangling", [DanglingField, + DanglingInitializerList, + ReturnStackAddress]>; def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">; def ExpansionToDefined : DiagGroup<"expansion-to-defined">; def FlagEnum : DiagGroup<"flag-enum">; @@ -407,7 +411,6 @@ def RedeclaredClassMember : DiagGroup<"redeclared-class-member">; def GNURedeclaredEnum : DiagGroup<"gnu-redeclared-enum">; def RedundantMove : DiagGroup<"redundant-move">; def Register : DiagGroup<"register", [DeprecatedRegister]>; -def ReturnStackAddress : DiagGroup<"return-stack-address">; def ReturnTypeCLinkage : DiagGroup<"return-type-c-linkage">; def ReturnType : DiagGroup<"return-type", [ReturnTypeCLinkage]>; def BindToTemporaryCopy : DiagGroup<"bind-to-temporary-copy", diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 62fd7de..33634c8 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -1845,10 +1845,6 @@ def err_reference_bind_failed : Error< "type $|could not bind to %select{rvalue|lvalue}1 of incompatible type}0,2">; def err_reference_bind_init_list : Error< "reference to type %0 cannot bind to an initializer list">; -def warn_temporary_array_to_pointer_decay : Warning< - "pointer is initialized by a temporary array, which will be destroyed at the " - "end of the full-expression">, - InGroup>; def err_init_list_bad_dest_type : Error< "%select{|non-aggregate }0type %1 cannot be initialized with an initializer " "list">; @@ -7876,15 +7872,31 @@ def warn_init_ptr_member_to_parameter_addr : Warning< def note_ref_or_ptr_member_declared_here : Note< "%select{reference|pointer}0 member declared here">; -def err_bind_ref_member_to_temporary : Error< +def err_dangling_member : Error< "%select{reference|backing array for 'std::initializer_list'}2 " "%select{|subobject of }1member %0 " "%select{binds to|is}2 a temporary object " - "whose lifetime would be shorter than the constructed object">; + "whose lifetime would be shorter than the lifetime of " + "the constructed object">; +def warn_dangling_member : Warning< + "%select{reference|backing array for 'std::initializer_list'}2 " + "%select{|subobject of }1member %0 " + "%select{binds to|is}2 a temporary object " + "whose lifetime is shorter than the lifetime of the constructed object">, + InGroup; def note_lifetime_extending_member_declared_here : Note< "%select{%select{reference|'std::initializer_list'}0 member|" "member with %select{reference|'std::initializer_list'}0 subobject}1 " "declared here">; +def warn_dangling_variable : Warning< + "%select{temporary %select{whose address is used as value of|bound to}3 " + "%select{%select{|reference }3member of local variable|" + "local %select{variable|reference}3}1|" + "array backing " + "%select{initializer list subobject of local variable|" + "local initializer list}1}0 " + "%2 will be destroyed at the end of the full-expression">, + InGroup; def warn_new_dangling_reference : Warning< "temporary bound to reference member of allocated object " "will be destroyed at the end of the full-expression">, @@ -7895,16 +7907,12 @@ def warn_new_dangling_initializer_list : Warning< "the allocated initializer list}0 " "will be destroyed at the end of the full-expression">, InGroup; -def warn_unsupported_temporary_not_extended : Warning< - "sorry, lifetime extension of temporary created " - "by aggregate initialization using default member initializer " - "is not supported; lifetime of temporary " - "will end at the end of the full-expression">, InGroup; -def warn_unsupported_init_list_not_extended : Warning< - "sorry, lifetime extension of backing array of initializer list created " +def warn_unsupported_lifetime_extension : Warning< + "sorry, lifetime extension of " + "%select{temporary|backing array of initializer list}0 created " "by aggregate initialization using default member initializer " - "is not supported; lifetime of backing array will end at the end of the " - "full-expression">, InGroup; + "is not supported; lifetime of %select{temporary|backing array}0 " + "will end at the end of the full-expression">, InGroup; // For non-floating point, expressions of the form x == x or x != x // should result in a warning, since these always evaluate to a constant. diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp index be284da..76044ee 100644 --- a/clang/lib/Sema/SemaInit.cpp +++ b/clang/lib/Sema/SemaInit.cpp @@ -6167,49 +6167,6 @@ PerformConstructorInitialization(Sema &S, return CurInit; } -/// Determine whether the specified InitializedEntity definitely has a lifetime -/// longer than the current full-expression. Conservatively returns false if -/// it's unclear. -static bool -InitializedEntityOutlivesFullExpression(const InitializedEntity &Entity) { - const InitializedEntity *Top = &Entity; - while (Top->getParent()) - Top = Top->getParent(); - - switch (Top->getKind()) { - case InitializedEntity::EK_Variable: - case InitializedEntity::EK_Result: - case InitializedEntity::EK_StmtExprResult: - case InitializedEntity::EK_Exception: - case InitializedEntity::EK_Member: - case InitializedEntity::EK_Binding: - case InitializedEntity::EK_New: - case InitializedEntity::EK_Base: - case InitializedEntity::EK_Delegating: - return true; - - case InitializedEntity::EK_ArrayElement: - case InitializedEntity::EK_VectorElement: - case InitializedEntity::EK_BlockElement: - case InitializedEntity::EK_LambdaToBlockConversionBlockElement: - case InitializedEntity::EK_ComplexElement: - // Could not determine what the full initialization is. Assume it might not - // outlive the full-expression. - return false; - - case InitializedEntity::EK_Parameter: - case InitializedEntity::EK_Parameter_CF_Audited: - case InitializedEntity::EK_Temporary: - case InitializedEntity::EK_LambdaCapture: - case InitializedEntity::EK_CompoundLiteralInit: - case InitializedEntity::EK_RelatedResult: - // The entity being initialized might not outlive the full-expression. - return false; - } - - llvm_unreachable("unknown entity kind"); -} - namespace { enum LifetimeKind { /// The lifetime of a temporary bound to this entity ends at the end of the @@ -6393,6 +6350,13 @@ static bool isVarOnPath(IndirectLocalPath &Path, VarDecl *VD) { return false; } +static bool pathContainsInit(IndirectLocalPath &Path) { + return std::any_of(Path.begin(), Path.end(), [=](IndirectLocalPathEntry E) { + return E.Kind == IndirectLocalPathEntry::DefaultInit || + E.Kind == IndirectLocalPathEntry::VarInit; + }); +} + static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Expr *Init, LocalVisitor Visit, bool RevisitSubinits); @@ -6660,6 +6624,12 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, // If the initializer is the address of a local, we could have a lifetime // problem. if (UO->getOpcode() == UO_AddrOf) { + // If this is &rvalue, then it's ill-formed and we have already diagnosed + // it. Don't produce a redundant warning about the lifetime of the + // temporary. + if (isa(UO->getSubExpr())) + return; + Path.push_back({IndirectLocalPathEntry::AddressOf, UO}); visitLocalsRetainedByReferenceBinding(Path, UO->getSubExpr(), RK_ReferenceBinding, Visit); @@ -6761,9 +6731,14 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, case LK_Extended: { auto *MTE = dyn_cast(L); - if (!MTE) - // FIXME: Warn on this. + if (!MTE) { + // The initialized entity has lifetime beyond the full-expression, + // and the local entity does too, so don't warn. + // + // FIXME: We should consider warning if a static / thread storage + // duration variable retains an automatic storage duration local. return false; + } // Lifetime-extend the temporary. if (Path.empty()) { @@ -6779,17 +6754,21 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, // We're supposed to lifetime-extend the temporary along this path (per // the resolution of DR1815), but we don't support that yet. // - // FIXME: Properly handle these situations. - // For the default member initializer case, perhaps the easiest approach + // FIXME: Properly handle this situation. Perhaps the easiest approach // would be to clone the initializer expression on each use that would // lifetime extend its temporaries. - Diag(DiagLoc, RK == RK_ReferenceBinding - ? diag::warn_unsupported_temporary_not_extended - : diag::warn_unsupported_init_list_not_extended) - << DiagRange; + Diag(DiagLoc, diag::warn_unsupported_lifetime_extension) + << RK << DiagRange; } else { - // FIXME: Warn on this. - return false; + // If the path goes through the initialization of a variable or field, + // it can't possibly reach a temporary created in this full-expression. + // We will have already diagnosed any problems with the initializer. + if (pathContainsInit(Path)) + return false; + + Diag(DiagLoc, diag::warn_dangling_variable) + << RK << !Entity.getParent() << ExtendingEntity->getDecl() + << Init->isGLValue() << DiagRange; } break; } @@ -6802,7 +6781,9 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, if (auto *ExtendingDecl = ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) { bool IsSubobjectMember = ExtendingEntity != &Entity; - Diag(DiagLoc, diag::err_bind_ref_member_to_temporary) + Diag(DiagLoc, shouldLifetimeExtendThroughPath(Path) + ? diag::err_dangling_member + : diag::warn_dangling_member) << ExtendingDecl << IsSubobjectMember << RK << DiagRange; // Don't bother adding a note pointing to the field if we're inside // its default member initializer; our primary diagnostic points to @@ -6826,9 +6807,7 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, // 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; - })) + if (pathContainsInit(Path)) return false; auto *DRE = dyn_cast(L); @@ -6858,7 +6837,7 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, Diag(DiagLoc, RK == RK_ReferenceBinding ? diag::warn_new_dangling_reference : diag::warn_new_dangling_initializer_list) - << (ExtendingEntity != &Entity) << DiagRange; + << !Entity.getParent() << DiagRange; } else { // We can't determine if the allocation outlives the local declaration. return false; @@ -7184,21 +7163,6 @@ InitializationSequence::Perform(Sema &S, return ExprError(); } - // 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)) { - const Expr *Init = Args[0]->skipRValueSubobjectAdjustments(); - if (auto *MTE = dyn_cast(Init)) - Init = MTE->GetTemporaryExpr(); - Expr::LValueClassification Kind = Init->ClassifyLValue(S.Context); - if (Kind == Expr::LV_ClassTemporary || Kind == Expr::LV_ArrayTemporary) - S.Diag(Init->getLocStart(), diag::warn_temporary_array_to_pointer_decay) - << Init->getSourceRange(); - } - QualType DestType = Entity.getType().getNonReferenceType(); // FIXME: Ugly hack around the fact that Entity.getType() is not // the same as Entity.getDecl()->getType() in cases involving type merging, diff --git a/clang/test/CXX/drs/dr16xx.cpp b/clang/test/CXX/drs/dr16xx.cpp index d9d404c..4f2f06e0 100644 --- a/clang/test/CXX/drs/dr16xx.cpp +++ b/clang/test/CXX/drs/dr16xx.cpp @@ -300,7 +300,7 @@ namespace dr1696 { // dr1696: 7 #if __cplusplus >= 201103L struct B { A &&a; // expected-note {{declared here}} - B() : a{} {} // expected-error {{reference member 'a' binds to a temporary object whose lifetime would be shorter than the constructed object}} + B() : a{} {} // expected-error {{reference member 'a' binds to a temporary object whose lifetime would be shorter than the lifetime of the constructed object}} } b; #endif @@ -308,7 +308,7 @@ namespace dr1696 { // dr1696: 7 C(); const A &a; // expected-note {{declared here}} }; - C::C() : a(A()) {} // expected-error {{reference member 'a' binds to a temporary object whose lifetime would be shorter than the constructed object}} + C::C() : a(A()) {} // expected-error {{reference member 'a' binds to a temporary object whose lifetime would be shorter than the lifetime of the constructed object}} #if __cplusplus >= 201103L // This is OK in C++14 onwards, per DR1815, though we don't support that yet: diff --git a/clang/test/SemaCXX/address-of-temporary.cpp b/clang/test/SemaCXX/address-of-temporary.cpp index 5eef1c5..eb0ca3c 100644 --- a/clang/test/SemaCXX/address-of-temporary.cpp +++ b/clang/test/SemaCXX/address-of-temporary.cpp @@ -26,11 +26,12 @@ namespace PointerToArrayDecay { template void consume(T); struct S { int *p; }; - void g0() { int *p = Y().a; } // expected-warning{{pointer is initialized by a temporary array}} - void g1() { int *p = Y{}.a; } // expected-warning{{pointer is initialized by a temporary array}} - void g2() { int *p = A{}; } // expected-warning{{pointer is initialized by a temporary array}} - void g3() { int *p = (A){}; } // expected-warning{{pointer is initialized by a temporary array}} - void g4() { Z *p = AZ{}; } // expected-warning{{pointer is initialized by a temporary array}} + void g0() { int *p = Y().a; } // expected-warning{{will be destroyed at the end of the full-expression}} + void g1() { int *p = Y{}.a; } // expected-warning{{will be destroyed at the end of the full-expression}} + void g2() { int *p = A{}; } // expected-warning{{will be destroyed at the end of the full-expression}} + void g3() { int *p = (A){}; } // expected-warning{{will be destroyed at the end of the full-expression}} + void g4() { Z *p = AZ{}; } // expected-warning{{will be destroyed at the end of the full-expression}} + void g5() { Z *p = &(Z&)(AZ{}[0]); } // expected-warning{{will be destroyed at the end of the full-expression}} void h0() { consume(Y().a); } void h1() { consume(Y{}.a); } @@ -38,10 +39,10 @@ namespace PointerToArrayDecay { void h3() { consume((A){}); } void h4() { consume(AZ{}); } - void i0() { S s = { Y().a }; } // expected-warning{{pointer is initialized by a temporary array}} - void i1() { S s = { Y{}.a }; } // expected-warning{{pointer is initialized by a temporary array}} - void i2() { S s = { A{} }; } // expected-warning{{pointer is initialized by a temporary array}} - void i3() { S s = { (A){} }; } // expected-warning{{pointer is initialized by a temporary array}} + void i0() { S s = { Y().a }; } // expected-warning{{will be destroyed at the end of the full-expression}} + void i1() { S s = { Y{}.a }; } // expected-warning{{will be destroyed at the end of the full-expression}} + void i2() { S s = { A{} }; } // expected-warning{{will be destroyed at the end of the full-expression}} + void i3() { S s = { (A){} }; } // expected-warning{{will be destroyed at the end of the full-expression}} void j0() { (void)S { Y().a }; } void j1() { (void)S { Y{}.a }; } diff --git a/clang/test/SemaCXX/warn-dangling-local.cpp b/clang/test/SemaCXX/warn-dangling-local.cpp new file mode 100644 index 0000000..19a722f --- /dev/null +++ b/clang/test/SemaCXX/warn-dangling-local.cpp @@ -0,0 +1,20 @@ +// RUN: %clang_cc1 -verify -std=c++11 %s + +using T = int[]; + +void f() { + int *p = &(int&)(int&&)0; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}} + + int *q = (int *const &)T{1, 2, 3}; // expected-warning {{temporary whose address is used as value of local variable 'q' will be destroyed at the end of the full-expression}} + + // FIXME: We don't warn here because the 'int*' temporary is not const, but + // it also can't have actually changed since it was created, so we could + // still warn. + int *r = (int *&&)T{1, 2, 3}; + + // FIXME: The wording of this warning is not quite right. There are two + // temporaries here: an 'int* const' temporary that points to the array, and + // is lifetime-extended, and an array temporary that the pointer temporary + // points to, which doesn't live long enough. + int *const &s = (int *const &)T{1, 2, 3}; // expected-warning {{temporary bound to local reference 's' will be destroyed at the end of the full-expression}} +} -- 2.7.4