diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 5c156a9c073a9c..bb47350f76b308 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -239,6 +239,8 @@ Improvements to Clang's diagnostics - Clang now diagnoses when the result of a [[nodiscard]] function is discarded after being cast in C. Fixes #GH104391. +- Don't emit duplicated dangling diagnostics. (#GH93386). + Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 7389046eaddde1..7e23c08cc79ffb 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 static bool isRecordWithAttr(QualType Type) { if (auto *RD = Type->getAsCXXRecordDecl()) @@ -326,66 +324,6 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) { return false; } -static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call, - LocalVisitor Visit) { - auto VisitPointerArg = [&](const Decl *D, Expr *Arg, bool Value) { - // We are not interested in the temporary base objects of gsl Pointers: - // Temp().ptr; // Here ptr might not dangle. - if (isa(Arg->IgnoreImpCasts())) - return; - // 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) - continue; - if (PE.Kind == IndirectLocalPathEntry::GslPointerInit || - PE.Kind == IndirectLocalPathEntry::GslPointerAssignment) - return; - break; - } - } - Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit - : IndirectLocalPathEntry::GslReferenceInit, - Arg, D}); - if (Arg->isGLValue()) - visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding, - Visit, - /*EnableLifetimeWarnings=*/true); - else - visitLocalsRetainedByInitializer(Path, Arg, Visit, true, - /*EnableLifetimeWarnings=*/true); - Path.pop_back(); - }; - - if (auto *MCE = dyn_cast(Call)) { - const auto *MD = cast_or_null(MCE->getDirectCallee()); - if (MD && shouldTrackImplicitObjectArg(MD)) - VisitPointerArg(MD, MCE->getImplicitObjectArgument(), - !MD->getReturnType()->isReferenceType()); - return; - } else if (auto *OCE = dyn_cast(Call)) { - FunctionDecl *Callee = OCE->getDirectCallee(); - if (Callee && Callee->isCXXInstanceMember() && - shouldTrackImplicitObjectArg(cast(Callee))) - VisitPointerArg(Callee, OCE->getArg(0), - !Callee->getReturnType()->isReferenceType()); - return; - } else if (auto *CE = dyn_cast(Call)) { - FunctionDecl *Callee = CE->getDirectCallee(); - if (Callee && shouldTrackFirstArgument(Callee)) - VisitPointerArg(Callee, CE->getArg(0), - !Callee->getReturnType()->isReferenceType()); - return; - } - - if (auto *CCE = dyn_cast(Call)) { - const auto *Ctor = CCE->getConstructor(); - const CXXRecordDecl *RD = Ctor->getParent(); - if (CCE->getNumArgs() > 0 && RD->hasAttr()) - VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0], true); - } -} - static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) { const TypeSourceInfo *TSI = FD->getTypeSourceInfo(); if (!TSI) @@ -423,8 +361,9 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) { return false; } -static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call, - LocalVisitor Visit) { +// Visit lifetimebound or gsl-pointer arguments. +static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, + LocalVisitor Visit) { const FunctionDecl *Callee; ArrayRef Args; @@ -439,6 +378,8 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call, if (!Callee) return; + bool EnableGSLAnalysis = !Callee->getASTContext().getDiagnostics().isIgnored( + diag::warn_dangling_lifetime_pointer, SourceLocation()); Expr *ObjectArg = nullptr; if (isa(Call) && Callee->isCXXInstanceMember()) { ObjectArg = Args[0]; @@ -451,11 +392,35 @@ static void visitLifetimeBoundArguments(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) { + // We are not interested in the temporary base objects of gsl Pointers: + // Temp().ptr; // Here ptr might not dangle. + if (isa(Arg->IgnoreImpCasts())) + return; + // 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) + continue; + if (PE.Kind == IndirectLocalPathEntry::GslPointerInit || + PE.Kind == IndirectLocalPathEntry::GslPointerAssignment) + return; + break; + } + } + Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit + : IndirectLocalPathEntry::GslReferenceInit, + Arg, D}); + if (Arg->isGLValue()) + visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding, + Visit); + else + visitLocalsRetainedByInitializer(Path, Arg, Visit, true); Path.pop_back(); }; @@ -478,6 +443,12 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call, CheckCoroObjArg = false; if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg) VisitLifetimeBoundArg(Callee, ObjectArg); + else if (EnableGSLAnalysis) { + if (auto *CME = dyn_cast(Callee); + CME && shouldTrackImplicitObjectArg(CME)) + VisitGSLPointerArg(Callee, ObjectArg, + !Callee->getReturnType()->isReferenceType()); + } } for (unsigned I = 0, @@ -485,6 +456,17 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call, I != N; ++I) { if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr()) VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]); + else if (EnableGSLAnalysis && I == 0) { // GSL + if (shouldTrackFirstArgument(Callee)) { + VisitGSLPointerArg(Callee, Args[0], + !Callee->getReturnType()->isReferenceType()); + } else if (auto *CCE = dyn_cast(Call); + CCE && + CCE->getConstructor()->getParent()->hasAttr()) { + VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0], + true); + } + } } } @@ -492,8 +474,7 @@ static void visitLifetimeBoundArguments(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. @@ -530,8 +511,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 @@ -545,23 +525,18 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, if (auto *MTE = dyn_cast(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(Init)) { // Lifetime of a non-reference type field is same as base object. if (auto *F = dyn_cast(M->getMemberDecl()); F && !F->getType()->isReferenceType()) - visitLocalsRetainedByInitializer(Path, M->getBase(), Visit, true, - EnableLifetimeWarnings); + visitLocalsRetainedByInitializer(Path, M->getBase(), Visit, true); } - if (isa(Init)) { - if (EnableLifetimeWarnings) - handleGslAnnotatedTypes(Path, Init, Visit); - return visitLifetimeBoundArguments(Path, Init, Visit); - } + if (isa(Init)) + return visitFunctionCallArguments(Path, Init, Visit); switch (Init->getStmtClass()) { case Stmt::DeclRefExprClass: { @@ -580,8 +555,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; @@ -593,15 +567,13 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, // handling all sorts of rvalues passed to a unary operator. const UnaryOperator *U = cast(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(Init)->getBase(), - Visit, true, EnableLifetimeWarnings); + visitLocalsRetainedByInitializer( + Path, cast(Init)->getBase(), Visit, true); break; } @@ -609,11 +581,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, case Stmt::BinaryConditionalOperatorClass: { auto *C = cast(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; } @@ -636,8 +606,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; @@ -678,18 +647,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(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. @@ -718,9 +685,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; @@ -735,8 +701,7 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, // lifetime of the array exactly like binding a reference to a temporary. if (auto *ILE = dyn_cast(Init)) return visitLocalsRetainedByReferenceBinding(Path, ILE->getSubExpr(), - RK_StdInitializerList, Visit, - EnableLifetimeWarnings); + RK_StdInitializerList, Visit); if (InitListExpr *ILE = dyn_cast(Init)) { // We already visited the elements of this initializer list while @@ -747,14 +712,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; } @@ -767,14 +730,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; @@ -783,14 +744,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; } } @@ -811,10 +771,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(); } @@ -828,18 +787,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(Init) || isa(Init)) { - if (EnableLifetimeWarnings) - handleGslAnnotatedTypes(Path, Init, Visit); - return visitLifetimeBoundArguments(Path, Init, Visit); - } + if (isa(Init) || isa(Init)) + return visitFunctionCallArguments(Path, Init, Visit); switch (Init->getStmtClass()) { case Stmt::UnaryOperatorClass: { @@ -855,8 +810,7 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, Path.push_back({IndirectLocalPathEntry::AddressOf, UO}); visitLocalsRetainedByReferenceBinding(Path, UO->getSubExpr(), - RK_ReferenceBinding, Visit, - EnableLifetimeWarnings); + RK_ReferenceBinding, Visit); } break; } @@ -869,11 +823,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; } @@ -883,11 +835,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; } @@ -989,8 +939,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 @@ -1284,19 +1233,20 @@ static void checkExprLifetimeImpl(Sema &SemaRef, }; llvm::SmallVector Path; - if (EnableLifetimeWarnings && LK == LK_Assignment && + if (!SemaRef.getDiagnostics().isIgnored(diag::warn_dangling_lifetime_pointer, + SourceLocation()) && + LK == LK_Assignment && isRecordWithAttr(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, @@ -1304,10 +1254,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, @@ -1323,7 +1271,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 09dfb2b5d96a89..86ee90ed6df8dd 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -479,3 +479,16 @@ void testForBug49342() { auto it = std::iter{} - 2; // Used to be false positive. } + +namespace GH93386 { +// verify no duplicated diagnostics are emitted. +struct [[gsl::Pointer]] S { + S(const std::vector& abc [[clang::lifetimebound]]); +}; + +S test(std::vector a) { + return S(a); // expected-warning {{address of stack memory associated with}} +} + +auto s = S(std::vector()); // expected-warning {{temporary whose address is used as value of local variable}} +}