From b1560bdb2bc67006f3b8f7e84ee0356632bf8126 Mon Sep 17 00:00:00 2001 From: Haojian Wu Date: Fri, 23 Aug 2024 17:50:27 +0200 Subject: [PATCH] Reland "[clang] Merge lifetimebound and GSL code paths for lifetime analysis (#104906)" (#105838) Reland without the `EnableLifetimeWarnings` removal. I will remove the EnableLifetimeWarnings in a follow-up patch. I have added a test to prevent regression. --- clang/docs/ReleaseNotes.rst | 2 + clang/lib/Sema/CheckExprLifetime.cpp | 127 ++++++++---------- .../Sema/warn-lifetime-analysis-nocfg.cpp | 20 +++ 3 files changed, 77 insertions(+), 72 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index baedc3cd6f03fc..17a707102d041f 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -234,6 +234,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). + - Improved diagnostic when trying to befriend a concept. (#GH45182). Improvements to Clang's time-trace diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 7389046eaddde1..c1362559536962 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -326,66 +326,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 +363,10 @@ 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, + bool EnableLifetimeWarnings) { const FunctionDecl *Callee; ArrayRef Args; @@ -458,6 +400,34 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call, /*EnableLifetimeWarnings=*/false); 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, + /*EnableLifetimeWarnings=*/true); + else + visitLocalsRetainedByInitializer(Path, Arg, Visit, true, + /*EnableLifetimeWarnings=*/true); + Path.pop_back(); + }; bool CheckCoroCall = false; if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) { @@ -478,6 +448,12 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call, CheckCoroObjArg = false; if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg) VisitLifetimeBoundArg(Callee, ObjectArg); + else if (EnableLifetimeWarnings) { + if (auto *CME = dyn_cast(Callee); + CME && shouldTrackImplicitObjectArg(CME)) + VisitGSLPointerArg(Callee, ObjectArg, + !Callee->getReturnType()->isReferenceType()); + } } for (unsigned I = 0, @@ -485,6 +461,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 (EnableLifetimeWarnings && I == 0) { + 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); + } + } } } @@ -557,11 +544,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, EnableLifetimeWarnings); } - if (isa(Init)) { - if (EnableLifetimeWarnings) - handleGslAnnotatedTypes(Path, Init, Visit); - return visitLifetimeBoundArguments(Path, Init, Visit); - } + if (isa(Init)) + return visitFunctionCallArguments(Path, Init, Visit, + EnableLifetimeWarnings); switch (Init->getStmtClass()) { case Stmt::DeclRefExprClass: { @@ -835,11 +820,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path, } } - 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, + EnableLifetimeWarnings); switch (Init->getStmtClass()) { case Stmt::UnaryOperatorClass: { diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 09dfb2b5d96a89..cd1904db327105 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -479,3 +479,23 @@ 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}} + +// Verify no regression on the follow case. +std::string_view test2(int i, std::optional a) { + if (i) + return std::move(*a); + return std::move(a.value()); +} +}