Author: Richard Smith Date: 2020-10-30T10:19:50-07:00 New Revision: 2177e4555ab84771c611a3295ab1149af7f79c29
URL: https://github.com/llvm/llvm-project/commit/2177e4555ab84771c611a3295ab1149af7f79c29 DIFF: https://github.com/llvm/llvm-project/commit/2177e4555ab84771c611a3295ab1149af7f79c29.diff LOG: PR47861: Expand dangling reference warning to look through copy construction, and to assume that assignment operators return *this. Added: Modified: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/include/clang/Basic/OperatorKinds.h clang/lib/Sema/SemaInit.cpp clang/test/CXX/expr/expr.prim/expr.prim.lambda/p11-1y.cpp clang/test/SemaCXX/conditional-expr.cpp clang/test/SemaCXX/constant-expression-cxx11.cpp clang/test/SemaCXX/cxx0x-initializer-stdinitializerlist.cpp clang/test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp clang/test/SemaCXX/return-stack-addr.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 47becc959fb2..fe6b88321f6b 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -9077,6 +9077,10 @@ def err_ret_local_block : Error< def note_local_var_initializer : Note< "%select{via initialization of|binding reference}0 variable " "%select{%2 |}1here">; +def note_lambda_capture_initializer : Note< + "%select{implicitly |}2captured%select{| by reference}3" + "%select{%select{ due to use|}2 here|" + " via initialization of lambda capture %0}1">; def note_init_with_default_member_initalizer : Note< "initializing field %0 with default member initializer">; diff --git a/clang/include/clang/Basic/OperatorKinds.h b/clang/include/clang/Basic/OperatorKinds.h index d66189144511..3315df246981 100644 --- a/clang/include/clang/Basic/OperatorKinds.h +++ b/clang/include/clang/Basic/OperatorKinds.h @@ -49,6 +49,11 @@ getRewrittenOverloadedOperator(OverloadedOperatorKind Kind) { } } +/// Determine if this is a compound assignment operator. +inline bool isCompoundAssignmentOperator(OverloadedOperatorKind Kind) { + return Kind >= OO_PlusEqual && Kind <= OO_PipeEqual; +} + } // end namespace clang #endif diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp index 419b25d278f5..5131ce446d04 100644 --- a/clang/lib/Sema/SemaInit.cpp +++ b/clang/lib/Sema/SemaInit.cpp @@ -6710,15 +6710,22 @@ struct IndirectLocalPathEntry { VarInit, LValToRVal, LifetimeBoundCall, + TemporaryCopy, + LambdaCaptureInit, GslReferenceInit, GslPointerInit } Kind; Expr *E; - const Decl *D = nullptr; + union { + const Decl *D = nullptr; + const LambdaCapture *Capture; + }; IndirectLocalPathEntry() {} IndirectLocalPathEntry(EntryKind K, Expr *E) : Kind(K), E(E) {} IndirectLocalPathEntry(EntryKind K, Expr *E, const Decl *D) : Kind(K), E(E), D(D) {} + IndirectLocalPathEntry(EntryKind K, Expr *E, const LambdaCapture *Capture) + : Kind(K), E(E), Capture(Capture) {} }; using IndirectLocalPath = llvm::SmallVectorImpl<IndirectLocalPathEntry>; @@ -6912,6 +6919,26 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) { if (ATL.getAttrAs<LifetimeBoundAttr>()) return true; } + + // Assume that all assignment operators with a "normal" return type return + // *this, that is, an lvalue reference that is the same type as the implicit + // object parameter (or the LHS for a non-member operator$=). + OverloadedOperatorKind OO = FD->getDeclName().getCXXOverloadedOperator(); + if (OO == OO_Equal || isCompoundAssignmentOperator(OO)) { + QualType RetT = FD->getReturnType(); + if (RetT->isLValueReferenceType()) { + ASTContext &Ctx = FD->getASTContext(); + QualType LHST; + auto *MD = dyn_cast<CXXMethodDecl>(FD); + if (MD && MD->isCXXInstanceMember()) + LHST = Ctx.getLValueReferenceType(MD->getThisObjectType()); + else + LHST = MD->getParamDecl(0)->getType(); + if (Ctx.hasSameType(RetT, LHST)) + return true; + } + } + return false; } @@ -7257,15 +7284,37 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, // The lifetime of an init-capture is that of the closure object constructed // by a lambda-expression. if (auto *LE = dyn_cast<LambdaExpr>(Init)) { + LambdaExpr::capture_iterator CapI = LE->capture_begin(); for (Expr *E : LE->capture_inits()) { + assert(CapI != LE->capture_end()); + const LambdaCapture &Cap = *CapI++; if (!E) continue; + if (Cap.capturesVariable()) + Path.push_back({IndirectLocalPathEntry::LambdaCaptureInit, E, &Cap}); if (E->isGLValue()) visitLocalsRetainedByReferenceBinding(Path, E, RK_ReferenceBinding, Visit, EnableLifetimeWarnings); else visitLocalsRetainedByInitializer(Path, E, Visit, true, EnableLifetimeWarnings); + if (Cap.capturesVariable()) + Path.pop_back(); + } + } + + // Assume that a copy or move from a temporary references the same objects + // that the temporary does. + if (auto *CCE = dyn_cast<CXXConstructExpr>(Init)) { + if (CCE->getConstructor()->isCopyOrMoveConstructor()) { + if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(CCE->getArg(0))) { + Expr *Arg = MTE->getSubExpr(); + Path.push_back({IndirectLocalPathEntry::TemporaryCopy, Arg, + CCE->getConstructor()}); + visitLocalsRetainedByInitializer(Path, Arg, Visit, true, + /*EnableLifetimeWarnings*/false); + Path.pop_back(); + } } } @@ -7342,14 +7391,31 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, } } +/// Whether a path to an object supports lifetime extension. +enum PathLifetimeKind { + /// Lifetime-extend along this path. + Extend, + /// We should lifetime-extend, but we don't because (due to technical + /// limitations) we can't. This happens for default member initializers, + /// which we don't clone for every use, so we don't have a unique + /// MaterializeTemporaryExpr to update. + ShouldExtend, + /// Do not lifetime extend along this path. + NoExtend +}; + /// 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 IndirectLocalPath &Path) { +/// supposed to lifetime-extend along. +static PathLifetimeKind +shouldLifetimeExtendThroughPath(const IndirectLocalPath &Path) { + PathLifetimeKind Kind = PathLifetimeKind::Extend; for (auto Elem : Path) { - if (Elem.Kind != IndirectLocalPathEntry::DefaultInit) - return false; + if (Elem.Kind == IndirectLocalPathEntry::DefaultInit) + Kind = PathLifetimeKind::ShouldExtend; + else if (Elem.Kind != IndirectLocalPathEntry::LambdaCaptureInit) + return PathLifetimeKind::NoExtend; } - return true; + return Kind; } /// Find the range for the first interesting entry in the path at or after I. @@ -7360,6 +7426,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I, case IndirectLocalPathEntry::AddressOf: case IndirectLocalPathEntry::LValToRVal: case IndirectLocalPathEntry::LifetimeBoundCall: + case IndirectLocalPathEntry::TemporaryCopy: case IndirectLocalPathEntry::GslReferenceInit: case IndirectLocalPathEntry::GslPointerInit: // These exist primarily to mark the path as not permitting or @@ -7372,6 +7439,11 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I, LLVM_FALLTHROUGH; case IndirectLocalPathEntry::DefaultInit: return Path[I].E->getSourceRange(); + + case IndirectLocalPathEntry::LambdaCaptureInit: + if (!Path[I].Capture->capturesVariable()) + continue; + return Path[I].E->getSourceRange(); } } return E->getSourceRange(); @@ -7449,17 +7521,16 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, return false; } - // Lifetime-extend the temporary. - if (Path.empty()) { + switch (shouldLifetimeExtendThroughPath(Path)) { + case PathLifetimeKind::Extend: // Update the storage duration of the materialized temporary. // FIXME: Rebuild the expression instead of mutating it. MTE->setExtendingDecl(ExtendingEntity->getDecl(), ExtendingEntity->allocateManglingNumber()); // Also visit the temporaries lifetime-extended by this initializer. return true; - } - if (shouldLifetimeExtendThroughPath(Path)) { + case PathLifetimeKind::ShouldExtend: // We're supposed to lifetime-extend the temporary along this path (per // the resolution of DR1815), but we don't support that yet. // @@ -7468,7 +7539,9 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, // lifetime extend its temporaries. Diag(DiagLoc, diag::warn_unsupported_lifetime_extension) << RK << DiagRange; - } else { + break; + + case PathLifetimeKind::NoExtend: // 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. @@ -7479,6 +7552,7 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, << RK << !Entity.getParent() << ExtendingEntity->getDecl()->isImplicit() << ExtendingEntity->getDecl() << Init->isGLValue() << DiagRange; + break; } break; } @@ -7499,7 +7573,8 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, return false; } bool IsSubobjectMember = ExtendingEntity != &Entity; - Diag(DiagLoc, shouldLifetimeExtendThroughPath(Path) + Diag(DiagLoc, shouldLifetimeExtendThroughPath(Path) != + PathLifetimeKind::NoExtend ? diag::err_dangling_member : diag::warn_dangling_member) << ExtendingDecl << IsSubobjectMember << RK << DiagRange; @@ -7606,6 +7681,7 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, break; case IndirectLocalPathEntry::LifetimeBoundCall: + case IndirectLocalPathEntry::TemporaryCopy: case IndirectLocalPathEntry::GslPointerInit: case IndirectLocalPathEntry::GslReferenceInit: // FIXME: Consider adding a note for these. @@ -7618,7 +7694,7 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, break; } - case IndirectLocalPathEntry::VarInit: + case IndirectLocalPathEntry::VarInit: { const VarDecl *VD = cast<VarDecl>(Elem.D); Diag(VD->getLocation(), diag::note_local_var_initializer) << VD->getType()->isReferenceType() @@ -7626,6 +7702,19 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity, << nextPathEntryRange(Path, I + 1, L); break; } + + case IndirectLocalPathEntry::LambdaCaptureInit: + if (!Elem.Capture->capturesVariable()) + break; + // FIXME: We can't easily tell apart an init-capture from a nested + // capture of an init-capture. + const VarDecl *VD = Elem.Capture->getCapturedVar(); + Diag(Elem.Capture->getLocation(), diag::note_lambda_capture_initializer) + << VD << VD->isInitCapture() << Elem.Capture->isExplicit() + << (Elem.Capture->getCaptureKind() == LCK_ByRef) << VD + << nextPathEntryRange(Path, I + 1, L); + break; + } } // We didn't lifetime-extend, so don't go any further; we don't need more diff --git a/clang/test/CXX/expr/expr.prim/expr.prim.lambda/p11-1y.cpp b/clang/test/CXX/expr/expr.prim/expr.prim.lambda/p11-1y.cpp index 7fd94b658f46..4a0cf39bc56b 100644 --- a/clang/test/CXX/expr/expr.prim/expr.prim.lambda/p11-1y.cpp +++ b/clang/test/CXX/expr/expr.prim/expr.prim.lambda/p11-1y.cpp @@ -54,9 +54,11 @@ auto bad_init_7 = [a{{1}}] {}; // expected-error {{cannot deduce type for lambda template<typename...T> void pack_1(T...t) { (void)[a(t...)] {}; } // expected-error {{initializer missing for lambda capture 'a'}} template void pack_1<>(); // expected-note {{instantiation of}} -// FIXME: Might need lifetime extension for the temporary here. -// See DR1695. -auto a = [a(4), b = 5, &c = static_cast<const int&&>(0)] { +// No lifetime-extension of the temporary here. +auto a_copy = [&c = static_cast<const int&&>(0)] {}; // expected-warning {{temporary whose address is used as value of local variable 'a_copy' will be destroyed at the end of the full-expression}} expected-note {{via initialization of lambda capture 'c'}} + +// But there is lifetime extension here. +auto &&a = [a(4), b = 5, &c = static_cast<const int&&>(0)] { static_assert(sizeof(a) == sizeof(int), ""); static_assert(sizeof(b) == sizeof(int), ""); using T = decltype(c); diff --git a/clang/test/SemaCXX/conditional-expr.cpp b/clang/test/SemaCXX/conditional-expr.cpp index 68e23a1f5727..9a5e2bac4341 100644 --- a/clang/test/SemaCXX/conditional-expr.cpp +++ b/clang/test/SemaCXX/conditional-expr.cpp @@ -1,5 +1,5 @@ -// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 -Wsign-conversion %s -// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++17 -Wsign-conversion %s +// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify=expected,expected-cxx11 -std=c++11 -Wsign-conversion %s +// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify=expected,expected-cxx17 -std=c++17 -Wsign-conversion %s // C++ rules for ?: are a lot stricter than C rules, and have to take into // account more conversion options. @@ -406,7 +406,8 @@ namespace lifetime_extension { struct D { A &&a; }; void f_indirect(bool b) { - D d = b ? D{B()} : D{C()}; + D d = b ? D{B()} // expected-cxx11-warning {{temporary whose address is used as value of local variable 'd' will be destroyed at the end of the full-expression}} + : D{C()}; // expected-cxx11-warning {{temporary whose address is used as value of local variable 'd' will be destroyed at the end of the full-expression}} } } diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp index 0e97798cc197..8b80f5438d8e 100644 --- a/clang/test/SemaCXX/constant-expression-cxx11.cpp +++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp @@ -387,6 +387,7 @@ constexpr B &&b4 = ((1, 2), 3, 4, B { {10}, {{20}} }); static_assert(&b4 != &b2, ""); // Proposed DR: copy-elision doesn't trigger lifetime extension. +// expected-warning@+1 2{{temporary whose address is used as value of local variable 'b5' will be destroyed at the end of the full-expression}} constexpr B b5 = B{ {0}, {0} }; // expected-error {{constant expression}} expected-note {{reference to temporary}} expected-note {{here}} namespace NestedNonStatic { @@ -396,6 +397,7 @@ namespace NestedNonStatic { struct A { int &&r; }; struct B { A &&a; }; constexpr B a = { A{0} }; // ok + // expected-warning@+1 {{temporary bound to reference member of local variable 'b' will be destroyed at the end of the full-expression}} constexpr B b = { A(A{0}) }; // expected-error {{constant expression}} expected-note {{reference to temporary}} expected-note {{here}} } diff --git a/clang/test/SemaCXX/cxx0x-initializer-stdinitializerlist.cpp b/clang/test/SemaCXX/cxx0x-initializer-stdinitializerlist.cpp index 9380330b41f3..24500b684d58 100644 --- a/clang/test/SemaCXX/cxx0x-initializer-stdinitializerlist.cpp +++ b/clang/test/SemaCXX/cxx0x-initializer-stdinitializerlist.cpp @@ -376,3 +376,13 @@ namespace weird_initlist { // ... but we do in constant evaluation. constexpr auto y = {weird{}, weird{}, weird{}, weird{}, weird{}}; // expected-error {{constant}} expected-note {{type 'const std::initializer_list<weird_initlist::weird>' has unexpected layout}} } + +auto v = std::initializer_list<int>{1,2,3}; // expected-warning {{array backing local initializer list 'v' will be destroyed at the end of the full-expression}} + +std::initializer_list<int> get(int cond) { + if (cond == 0) + return {}; + if (cond == 1) + return {1, 2, 3}; // expected-warning {{returning address of local temporary object}} + return std::initializer_list<int>{1, 2, 3}; // expected-warning {{returning address of local temporary object}} +} diff --git a/clang/test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp b/clang/test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp index a98366c8794a..8d7845dc607d 100644 --- a/clang/test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp +++ b/clang/test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp @@ -177,19 +177,15 @@ void doit() { sample::X cx{5}; auto L = [=](auto a) { const int z = 3; - // FIXME: The warning below is correct but for some reason doesn't show - // up in C++17 mode. return [&,a](auto b) { -#if __cplusplus > 201702L - // expected-warning@-2 {{address of stack memory associated with local variable 'z' returned}} + // expected-warning@-1 {{address of stack memory associated with local variable 'z' returned}} // expected-note@#call {{in instantiation of}} -#endif - const int y = 5; - return [=](auto c) { + const int y = 5; + return [=](auto c) { int d[sizeof(a) == sizeof(c) || sizeof(c) == sizeof(b) ? 2 : 1]; f(x, d); f(y, d); - f(z, d); + f(z, d); // expected-note {{implicitly captured by reference due to use here}} decltype(a) A = a; decltype(b) B = b; const int &i = cx.i; diff --git a/clang/test/SemaCXX/return-stack-addr.cpp b/clang/test/SemaCXX/return-stack-addr.cpp index 08aaa749b4e3..2fa2de10f0d6 100644 --- a/clang/test/SemaCXX/return-stack-addr.cpp +++ b/clang/test/SemaCXX/return-stack-addr.cpp @@ -156,6 +156,54 @@ void ret_from_lambda() { (void) [=]() -> int& { int a; return a; }; // expected-warning {{reference to stack}} (void) [&]() -> int& { int &a = b; return a; }; (void) [=]() mutable -> int& { int &a = b; return a; }; + + (void) [] { + int a; + return [&] { // expected-warning {{address of stack memory associated with local variable 'a' returned}} + return a; // expected-note {{implicitly captured by reference due to use here}} + }; + }; + (void) [] { + int a; + return [&a] {}; // expected-warning {{address of stack memory associated with local variable 'a' returned}} expected-note {{captured by reference here}} + }; + (void) [] { + int a; + return [=] { + return a; + }; + }; + (void) [] { + int a; + return [a] {}; + }; + (void) [] { + int a; + // expected-warning@+1 {{C++14}} + return [&b = a] {}; // expected-warning {{address of stack memory associated with local variable 'a' returned}} expected-note {{captured by reference via initialization of lambda capture 'b'}} + }; + (void) [] { + int a; + // expected-warning@+1 {{C++14}} + return [b = &a] {}; // expected-warning {{address of stack memory associated with local variable 'a' returned}} expected-note {{captured via initialization of lambda capture 'b'}} + }; +} + +struct HoldsPointer { int *p; }; + +HoldsPointer ret_via_member_1() { + int n; + return {&n}; // expected-warning {{address of stack memory associated with local variable 'n' returned}} +} +HoldsPointer ret_via_member_2() { + int n; + return HoldsPointer(HoldsPointer{&n}); // expected-warning {{address of stack memory associated with local variable 'n' returned}} +} +// FIXME: We could diagnose this too. +HoldsPointer ret_via_member_3() { + int n; + const HoldsPointer hp = HoldsPointer{&n}; + return hp; } namespace mem_ptr { @@ -163,3 +211,11 @@ namespace mem_ptr { int X::*f(); int &r(X *p) { return p->*f(); } } + +namespace PR47861 { + struct A { + A(int i); + A &operator+=(int i); + }; + A const &b = A(5) += 5; // expected-warning {{temporary bound to local reference 'b' will be destroyed at the end of the full-expression}} +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
