https://github.com/hokein created https://github.com/llvm/llvm-project/pull/105884
Reland the remaining change of https://github.com/llvm/llvm-project/pull/104906, with a proper fix for the regression. >From 82069c8eb2c31734ce149b510317cded04b3835e Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Fri, 23 Aug 2024 21:42:20 +0200 Subject: [PATCH] [clang] Remove the EnableLifetimeWarnings flag in lifetime analysis, --- clang/lib/Sema/CheckExprLifetime.cpp | 156 +++++++----------- .../Sema/warn-lifetime-analysis-nocfg.cpp | 11 ++ 2 files changed, 72 insertions(+), 95 deletions(-) diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index c1362559536962..ef43a0d25e0c9e 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -237,13 +237,11 @@ static bool pathContainsInit(IndirectLocalPath &Path) { static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Expr *Init, LocalVisitor Visit, - bool RevisitSubinits, - bool EnableLifetimeWarnings); + bool RevisitSubinits); static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, Expr *Init, ReferenceKind RK, - LocalVisitor Visit, - bool EnableLifetimeWarnings); + LocalVisitor Visit); template <typename T> static bool isRecordWithAttr(QualType Type) { if (auto *RD = Type->getAsCXXRecordDecl()) @@ -365,8 +363,7 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) { // Visit lifetimebound or gsl-pointer arguments. static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, - LocalVisitor Visit, - bool EnableLifetimeWarnings) { + LocalVisitor Visit) { const FunctionDecl *Callee; ArrayRef<Expr *> Args; @@ -381,6 +378,8 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, if (!Callee) return; + bool EnableGSLAnalysis = !Callee->getASTContext().getDiagnostics().isIgnored( + diag::warn_dangling_lifetime_pointer, SourceLocation()); Expr *ObjectArg = nullptr; if (isa<CXXOperatorCallExpr>(Call) && Callee->isCXXInstanceMember()) { ObjectArg = Args[0]; @@ -393,11 +392,9 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, Path.push_back({IndirectLocalPathEntry::LifetimeBoundCall, Arg, D}); if (Arg->isGLValue()) visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding, - Visit, - /*EnableLifetimeWarnings=*/false); + Visit); else - visitLocalsRetainedByInitializer(Path, Arg, Visit, true, - /*EnableLifetimeWarnings=*/false); + visitLocalsRetainedByInitializer(Path, Arg, Visit, true); Path.pop_back(); }; auto VisitGSLPointerArg = [&](const Decl *D, Expr *Arg, bool Value) { @@ -408,7 +405,8 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, // Once we initialized a value with a reference, it can no longer dangle. if (!Value) { for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) { - if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit) + if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit || + PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall) continue; if (PE.Kind == IndirectLocalPathEntry::GslPointerInit || PE.Kind == IndirectLocalPathEntry::GslPointerAssignment) @@ -421,11 +419,9 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, Arg, D}); if (Arg->isGLValue()) visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding, - Visit, - /*EnableLifetimeWarnings=*/true); + Visit); else - visitLocalsRetainedByInitializer(Path, Arg, Visit, true, - /*EnableLifetimeWarnings=*/true); + visitLocalsRetainedByInitializer(Path, Arg, Visit, true); Path.pop_back(); }; @@ -448,7 +444,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, CheckCoroObjArg = false; if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg) VisitLifetimeBoundArg(Callee, ObjectArg); - else if (EnableLifetimeWarnings) { + else if (EnableGSLAnalysis) { if (auto *CME = dyn_cast<CXXMethodDecl>(Callee); CME && shouldTrackImplicitObjectArg(CME)) VisitGSLPointerArg(Callee, ObjectArg, @@ -461,15 +457,15 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, I != N; ++I) { if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>()) VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]); - else if (EnableLifetimeWarnings && I == 0) { + else if (EnableGSLAnalysis && I == 0) { // GSL if (shouldTrackFirstArgument(Callee)) { VisitGSLPointerArg(Callee, Args[0], !Callee->getReturnType()->isReferenceType()); - } else { - if (auto *CCE = dyn_cast<CXXConstructExpr>(Call); - CCE && CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) - VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0], - true); + } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call); + CCE && + CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) { + VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0], + true); } } } @@ -479,8 +475,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, /// glvalue expression \c Init. static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, Expr *Init, ReferenceKind RK, - LocalVisitor Visit, - bool EnableLifetimeWarnings) { + LocalVisitor Visit) { RevertToOldSizeRAII RAII(Path); // Walk past any constructs which we can lifetime-extend across. @@ -517,8 +512,7 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, else // We can't lifetime extend through this but we might still find some // retained temporaries. - return visitLocalsRetainedByInitializer(Path, Init, Visit, true, - EnableLifetimeWarnings); + return visitLocalsRetainedByInitializer(Path, Init, Visit, true); } // Step into CXXDefaultInitExprs so we can diagnose cases where a @@ -532,21 +526,18 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init)) { if (Visit(Path, Local(MTE), RK)) - visitLocalsRetainedByInitializer(Path, MTE->getSubExpr(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, MTE->getSubExpr(), Visit, true); } if (auto *M = dyn_cast<MemberExpr>(Init)) { // Lifetime of a non-reference type field is same as base object. if (auto *F = dyn_cast<FieldDecl>(M->getMemberDecl()); F && !F->getType()->isReferenceType()) - visitLocalsRetainedByInitializer(Path, M->getBase(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, M->getBase(), Visit, true); } if (isa<CallExpr>(Init)) - return visitFunctionCallArguments(Path, Init, Visit, - EnableLifetimeWarnings); + return visitFunctionCallArguments(Path, Init, Visit); switch (Init->getStmtClass()) { case Stmt::DeclRefExprClass: { @@ -565,8 +556,7 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, } else if (VD->getInit() && !isVarOnPath(Path, VD)) { Path.push_back({IndirectLocalPathEntry::VarInit, DRE, VD}); visitLocalsRetainedByReferenceBinding(Path, VD->getInit(), - RK_ReferenceBinding, Visit, - EnableLifetimeWarnings); + RK_ReferenceBinding, Visit); } } break; @@ -578,15 +568,13 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, // handling all sorts of rvalues passed to a unary operator. const UnaryOperator *U = cast<UnaryOperator>(Init); if (U->getOpcode() == UO_Deref) - visitLocalsRetainedByInitializer(Path, U->getSubExpr(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, U->getSubExpr(), Visit, true); break; } case Stmt::ArraySectionExprClass: { - visitLocalsRetainedByInitializer(Path, - cast<ArraySectionExpr>(Init)->getBase(), - Visit, true, EnableLifetimeWarnings); + visitLocalsRetainedByInitializer( + Path, cast<ArraySectionExpr>(Init)->getBase(), Visit, true); break; } @@ -594,11 +582,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, case Stmt::BinaryConditionalOperatorClass: { auto *C = cast<AbstractConditionalOperator>(Init); if (!C->getTrueExpr()->getType()->isVoidType()) - visitLocalsRetainedByReferenceBinding(Path, C->getTrueExpr(), RK, Visit, - EnableLifetimeWarnings); + visitLocalsRetainedByReferenceBinding(Path, C->getTrueExpr(), RK, Visit); if (!C->getFalseExpr()->getType()->isVoidType()) - visitLocalsRetainedByReferenceBinding(Path, C->getFalseExpr(), RK, Visit, - EnableLifetimeWarnings); + visitLocalsRetainedByReferenceBinding(Path, C->getFalseExpr(), RK, Visit); break; } @@ -621,8 +607,7 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, /// the prvalue expression \c Init. static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Expr *Init, LocalVisitor Visit, - bool RevisitSubinits, - bool EnableLifetimeWarnings) { + bool RevisitSubinits) { RevertToOldSizeRAII RAII(Path); Expr *Old; @@ -663,18 +648,16 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, if (VD && VD->getType().isConstQualified() && VD->getInit() && !isVarOnPath(Path, VD)) { Path.push_back({IndirectLocalPathEntry::VarInit, DRE, VD}); - visitLocalsRetainedByInitializer( - Path, VD->getInit(), Visit, true, EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, VD->getInit(), Visit, + true); } } else if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L)) { if (MTE->getType().isConstQualified()) visitLocalsRetainedByInitializer(Path, MTE->getSubExpr(), - Visit, true, - EnableLifetimeWarnings); + Visit, true); } return false; - }, - EnableLifetimeWarnings); + }); // We assume that objects can be retained by pointers cast to integers, // but not if the integer is cast to floating-point type or to _Complex. @@ -703,9 +686,8 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, // Model array-to-pointer decay as taking the address of the array // lvalue. Path.push_back({IndirectLocalPathEntry::AddressOf, CE}); - return visitLocalsRetainedByReferenceBinding(Path, CE->getSubExpr(), - RK_ReferenceBinding, Visit, - EnableLifetimeWarnings); + return visitLocalsRetainedByReferenceBinding( + Path, CE->getSubExpr(), RK_ReferenceBinding, Visit); default: return; @@ -720,8 +702,7 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, // lifetime of the array exactly like binding a reference to a temporary. if (auto *ILE = dyn_cast<CXXStdInitializerListExpr>(Init)) return visitLocalsRetainedByReferenceBinding(Path, ILE->getSubExpr(), - RK_StdInitializerList, Visit, - EnableLifetimeWarnings); + RK_StdInitializerList, Visit); if (InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) { // We already visited the elements of this initializer list while @@ -732,14 +713,12 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, if (ILE->isTransparent()) return visitLocalsRetainedByInitializer(Path, ILE->getInit(0), Visit, - RevisitSubinits, - EnableLifetimeWarnings); + RevisitSubinits); if (ILE->getType()->isArrayType()) { for (unsigned I = 0, N = ILE->getNumInits(); I != N; ++I) visitLocalsRetainedByInitializer(Path, ILE->getInit(I), Visit, - RevisitSubinits, - EnableLifetimeWarnings); + RevisitSubinits); return; } @@ -752,14 +731,12 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, if (RD->isUnion() && ILE->getInitializedFieldInUnion() && ILE->getInitializedFieldInUnion()->getType()->isReferenceType()) visitLocalsRetainedByReferenceBinding(Path, ILE->getInit(0), - RK_ReferenceBinding, Visit, - EnableLifetimeWarnings); + RK_ReferenceBinding, Visit); else { unsigned Index = 0; for (; Index < RD->getNumBases() && Index < ILE->getNumInits(); ++Index) visitLocalsRetainedByInitializer(Path, ILE->getInit(Index), Visit, - RevisitSubinits, - EnableLifetimeWarnings); + RevisitSubinits); for (const auto *I : RD->fields()) { if (Index >= ILE->getNumInits()) break; @@ -768,14 +745,13 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Expr *SubInit = ILE->getInit(Index); if (I->getType()->isReferenceType()) visitLocalsRetainedByReferenceBinding(Path, SubInit, - RK_ReferenceBinding, Visit, - EnableLifetimeWarnings); + 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. - visitLocalsRetainedByInitializer( - Path, SubInit, Visit, RevisitSubinits, EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, SubInit, Visit, + RevisitSubinits); ++Index; } } @@ -796,10 +772,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Path.push_back({IndirectLocalPathEntry::LambdaCaptureInit, E, &Cap}); if (E->isGLValue()) visitLocalsRetainedByReferenceBinding(Path, E, RK_ReferenceBinding, - Visit, EnableLifetimeWarnings); + Visit); else - visitLocalsRetainedByInitializer(Path, E, Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, E, Visit, true); if (Cap.capturesVariable()) Path.pop_back(); } @@ -813,16 +788,14 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Expr *Arg = MTE->getSubExpr(); Path.push_back({IndirectLocalPathEntry::TemporaryCopy, Arg, CCE->getConstructor()}); - visitLocalsRetainedByInitializer(Path, Arg, Visit, true, - /*EnableLifetimeWarnings*/ false); + visitLocalsRetainedByInitializer(Path, Arg, Visit, true); Path.pop_back(); } } } if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init)) - return visitFunctionCallArguments(Path, Init, Visit, - EnableLifetimeWarnings); + return visitFunctionCallArguments(Path, Init, Visit); switch (Init->getStmtClass()) { case Stmt::UnaryOperatorClass: { @@ -838,8 +811,7 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Path.push_back({IndirectLocalPathEntry::AddressOf, UO}); visitLocalsRetainedByReferenceBinding(Path, UO->getSubExpr(), - RK_ReferenceBinding, Visit, - EnableLifetimeWarnings); + RK_ReferenceBinding, Visit); } break; } @@ -852,11 +824,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, break; if (BO->getLHS()->getType()->isPointerType()) - visitLocalsRetainedByInitializer(Path, BO->getLHS(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, BO->getLHS(), Visit, true); else if (BO->getRHS()->getType()->isPointerType()) - visitLocalsRetainedByInitializer(Path, BO->getRHS(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, BO->getRHS(), Visit, true); break; } @@ -866,11 +836,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, // In C++, we can have a throw-expression operand, which has 'void' type // and isn't interesting from a lifetime perspective. if (!C->getTrueExpr()->getType()->isVoidType()) - visitLocalsRetainedByInitializer(Path, C->getTrueExpr(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, C->getTrueExpr(), Visit, true); if (!C->getFalseExpr()->getType()->isVoidType()) - visitLocalsRetainedByInitializer(Path, C->getFalseExpr(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, C->getFalseExpr(), Visit, true); break; } @@ -972,8 +940,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity, const InitializedEntity *ExtendingEntity, LifetimeKind LK, - const AssignedEntity *AEntity, Expr *Init, - bool EnableLifetimeWarnings) { + const AssignedEntity *AEntity, Expr *Init) { assert((AEntity && LK == LK_Assignment) || (InitEntity && LK != LK_Assignment)); // If this entity doesn't have an interesting lifetime, don't bother looking @@ -1267,19 +1234,20 @@ static void checkExprLifetimeImpl(Sema &SemaRef, }; llvm::SmallVector<IndirectLocalPathEntry, 8> Path; - if (EnableLifetimeWarnings && LK == LK_Assignment && + if (!SemaRef.getDiagnostics().isIgnored(diag::warn_dangling_lifetime_pointer, + SourceLocation()) && + LK == LK_Assignment && isRecordWithAttr<PointerAttr>(AEntity->LHS->getType())) Path.push_back({IndirectLocalPathEntry::GslPointerAssignment, Init}); if (Init->isGLValue()) visitLocalsRetainedByReferenceBinding(Path, Init, RK_ReferenceBinding, - TemporaryVisitor, - EnableLifetimeWarnings); + TemporaryVisitor); else visitLocalsRetainedByInitializer( Path, Init, TemporaryVisitor, // Don't revisit the sub inits for the intialization case. - /*RevisitSubinits=*/!InitEntity, EnableLifetimeWarnings); + /*RevisitSubinits=*/!InitEntity); } void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity, @@ -1287,10 +1255,8 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity, auto LTResult = getEntityLifetime(&Entity); LifetimeKind LK = LTResult.getInt(); const InitializedEntity *ExtendingEntity = LTResult.getPointer(); - bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored( - diag::warn_dangling_lifetime_pointer, SourceLocation()); checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, - /*AEntity*/ nullptr, Init, EnableLifetimeWarnings); + /*AEntity*/ nullptr, Init); } void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity, @@ -1306,7 +1272,7 @@ void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity, checkExprLifetimeImpl(SemaRef, /*InitEntity=*/nullptr, /*ExtendingEntity=*/nullptr, LK_Assignment, &Entity, - Init, EnableLifetimeWarnings); + Init); } } // namespace clang::sema diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index cd1904db327105..3cfd9d57eca7a3 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -498,4 +498,15 @@ std::string_view test2(int i, std::optional<std::string_view> a) { return std::move(*a); return std::move(a.value()); } + +struct Foo; +struct FooView { + FooView(const Foo& foo [[clang::lifetimebound]]); +}; +FooView test3(int i, std::optional<Foo> a) { + if (i) + return *a; // expected-warning {{address of stack memory}} + return a.value(); // expected-warning {{address of stack memory}} +} + } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
