llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Gábor Horváth (Xazax-hun) <details> <summary>Changes</summary> A non-trivial destructor or __attribute__((cleanup(fn))) callback running at scope exit may read a borrow the object holds (e.g. a [[gsl::Pointer]] whose out-of-line ~T() or cleanup function dereferences its captured view). The analysis never sees those bodies, so model the action as a use of the object, keeping the borrow live to that point: a borrowed-from object destroyed earlier (reverse-declaration order) is now reported instead of silently dangling. The implicit use has no source expression, so UseFact gains a SourceLocation anchor and reportUseAfterScope/reportUseAfterInvalidation gain SourceLocation overloads for the "used here" note. Owners are excluded from the destructor case (their destruction frees their own storage, already modeled by the ExpireFact). Assisted-by: Claude Opus 4.8 --- Full diff: https://github.com/llvm/llvm-project/pull/204650.diff 9 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h (+9) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h (+2) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h (+14) - (modified) clang/lib/Analysis/LifetimeSafety/Checker.cpp (+18-2) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+30) - (modified) clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp (+3-1) - (modified) clang/lib/Sema/SemaLifetimeSafety.h (+47) - (modified) clang/test/Sema/LifetimeSafety/invalidations.cpp (+24) - (modified) clang/test/Sema/LifetimeSafety/safety.cpp (+50) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h index 88b509e1b94df..9bc81e9dc08f9 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h @@ -240,16 +240,25 @@ class UseFact : public Fact { // True if this use is a write operation (e.g., left-hand side of assignment). // Write operations are exempted from use-after-free checks. bool IsWritten = false; + // For an implicit use with no source expression (a scope-exit destructor or + // cleanup callback reading a borrow): the location to anchor diagnostics at. + SourceLocation ImplicitLoc; public: static bool classof(const Fact *F) { return F->getKind() == Kind::Use; } UseFact(const Expr *UseExpr, const OriginList *OList) : Fact(Kind::Use), UseExpr(UseExpr), OList(OList) {} + UseFact(SourceLocation ImplicitLoc, const OriginList *OList) + : Fact(Kind::Use), UseExpr(nullptr), OList(OList), + ImplicitLoc(ImplicitLoc) {} const OriginList *getUsedOrigins() const { return OList; } void setUsedOrigins(const OriginList *NewList) { OList = NewList; } const Expr *getUseExpr() const { return UseExpr; } + /// True if this use has no source expression; use getImplicitLoc() instead. + bool isImplicit() const { return UseExpr == nullptr; } + SourceLocation getImplicitLoc() const { return ImplicitLoc; } void markAsWritten() { IsWritten = true; } bool isWritten() const { return IsWritten; } diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h index 5ac67263681ac..c923b99dfc743 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h @@ -84,6 +84,8 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> { void handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds); + void handleCleanupFunction(const CFGCleanupFunction &CleanupFunction); + void handleFullExprCleanup(const CFGFullExprCleanup &FullExprCleanup); void handleExitBlock(); diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h index 28886b826f72f..ec207be0823ff 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h @@ -65,6 +65,12 @@ class LifetimeSafetySemaHelper { const Expr *MovedExpr, SourceLocation FreeLoc, llvm::ArrayRef<const Expr *> ExprChain) {} + // Variant for an implicit use with no source expression; `UseLoc` anchors the + // "used here" note. + virtual void reportUseAfterScope(const Expr *IssueExpr, SourceLocation UseLoc, + const Expr *MovedExpr, + SourceLocation FreeLoc, + llvm::ArrayRef<const Expr *> ExprChain) {} virtual void reportUseAfterReturn(const Expr *IssueExpr, const Expr *ReturnExpr, @@ -88,6 +94,14 @@ class LifetimeSafetySemaHelper { virtual void reportUseAfterInvalidation(const ParmVarDecl *PVD, const Expr *UseExpr, const Expr *InvalidationExpr) {} + // Variants for an implicit use with no source expression; `UseLoc` anchors + // the "used here" note. + virtual void reportUseAfterInvalidation(const Expr *IssueExpr, + SourceLocation UseLoc, + const Expr *InvalidationExpr) {} + virtual void reportUseAfterInvalidation(const ParmVarDecl *PVD, + SourceLocation UseLoc, + const Expr *InvalidationExpr) {} virtual void reportInvalidatedField(const Expr *IssueExpr, const FieldDecl *Field, const Expr *InvalidationExpr) {} diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp index d41d6f43f837b..ab91d9851912a 100644 --- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp @@ -72,7 +72,8 @@ class LifetimeChecker { static SourceLocation GetFactLoc(llvm::PointerUnion<const UseFact *, const OriginEscapesFact *> F) { if (const auto *UF = F.dyn_cast<const UseFact *>()) - return UF->getUseExpr()->getExprLoc(); + return UF->isImplicit() ? UF->getImplicitLoc() + : UF->getUseExpr()->getExprLoc(); if (const auto *OEF = F.dyn_cast<const OriginEscapesFact *>()) { if (auto *ReturnEsc = dyn_cast<ReturnEscapeFact>(OEF)) return ReturnEsc->getReturnExpr()->getExprLoc(); @@ -254,7 +255,22 @@ class LifetimeChecker { SourceLocation ExpiryLoc = Warning.ExpiryLoc; if (const auto *UF = CausingFact.dyn_cast<const UseFact *>()) { - if (Warning.InvalidatedByExpr) { + // An implicit use has no source expression; anchor diagnostics at its + // location. + if (UF->isImplicit()) { + if (Warning.InvalidatedByExpr) { + if (IssueExpr) + SemaHelper->reportUseAfterInvalidation( + IssueExpr, UF->getImplicitLoc(), Warning.InvalidatedByExpr); + else if (InvalidatedPVD) + SemaHelper->reportUseAfterInvalidation(InvalidatedPVD, + UF->getImplicitLoc(), + Warning.InvalidatedByExpr); + } else + SemaHelper->reportUseAfterScope( + IssueExpr, UF->getImplicitLoc(), MovedExpr, ExpiryLoc, + getExprChain(LoanPropagation.buildOriginFlowChain(UF, LID))); + } else if (Warning.InvalidatedByExpr) { if (IssueExpr) // Use-after-invalidation of an object on stack. SemaHelper->reportUseAfterInvalidation(IssueExpr, UF->getUseExpr(), diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 545836cd76fb9..2e9b5f0723d04 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -125,6 +125,9 @@ void FactsGenerator::run() { else if (std::optional<CFGLifetimeEnds> LifetimeEnds = Element.getAs<CFGLifetimeEnds>()) handleLifetimeEnds(*LifetimeEnds); + else if (std::optional<CFGCleanupFunction> CleanupFunction = + Element.getAs<CFGCleanupFunction>()) + handleCleanupFunction(*CleanupFunction); else if (std::optional<CFGFullExprCleanup> FullExprCleanup = Element.getAs<CFGFullExprCleanup>()) { handleFullExprCleanup(*FullExprCleanup); @@ -754,6 +757,20 @@ void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) { const VarDecl *LifetimeEndsVD = LifetimeEnds.getVarDecl(); if (!LifetimeEndsVD) return; + // A non-trivial destructor at scope exit may read a borrow the object holds + // (e.g. a [[gsl::Pointer]] whose out-of-line ~T() dereferences its view). The + // analysis never sees that body, so model the destruction as a use, keeping + // the borrow live to that point so a borrowed-from object destroyed earlier + // (reverse-declaration order) is reported. Owners are excluded: their + // destruction frees their own storage (modeled by the ExpireFact), not a + // borrow into another object. + QualType VDTy = LifetimeEndsVD->getType(); + if (const CXXRecordDecl *RD = VDTy->getAsCXXRecordDecl(); + RD && RD->hasDefinition() && RD->hasNonTrivialDestructor() && + !isGslOwnerType(VDTy) && hasOrigins(VDTy)) + if (OriginList *List = getOriginsList(*LifetimeEndsVD)) + CurrentBlockFacts.push_back(FactMgr.createFact<UseFact>( + LifetimeEnds.getTriggerStmt()->getEndLoc(), List)); // Expire the origin when its variable's lifetime ends to ensure liveness // doesn't persist through loop back-edges. std::optional<OriginID> ExpiredOID; @@ -769,6 +786,19 @@ void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) { ExpiredOID)); } +void FactsGenerator::handleCleanupFunction( + const CFGCleanupFunction &CleanupFunction) { + // A variable with __attribute__((cleanup(fn))) has fn(&var) called at scope + // exit; like a non-trivial destructor, that callback may read a borrow the + // variable holds, so model it as a use of the variable. + const VarDecl *VD = CleanupFunction.getVarDecl(); + if (!VD || !hasOrigins(VD->getType())) + return; + if (OriginList *List = getOriginsList(*VD)) + CurrentBlockFacts.push_back( + FactMgr.createFact<UseFact>(VD->getLocation(), List)); +} + void FactsGenerator::handleFullExprCleanup( const CFGFullExprCleanup &FullExprCleanup) { for (const auto *MTE : FullExprCleanup.getExpiringMTEs()) diff --git a/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp b/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp index cfbcacf04b1b0..0b6e18bb40f34 100644 --- a/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp @@ -56,7 +56,9 @@ struct Lattice { static SourceLocation GetFactLoc(CausingFactType F) { if (const auto *UF = F.dyn_cast<const UseFact *>()) - return UF->getUseExpr()->getExprLoc(); + // An implicit use has no source expression; use its explicit location. + return UF->isImplicit() ? UF->getImplicitLoc() + : UF->getUseExpr()->getExprLoc(); if (const auto *OEF = F.dyn_cast<const OriginEscapesFact *>()) { if (auto *ReturnEsc = dyn_cast<ReturnEscapeFact>(OEF)) return ReturnEsc->getReturnExpr()->getExprLoc(); diff --git a/clang/lib/Sema/SemaLifetimeSafety.h b/clang/lib/Sema/SemaLifetimeSafety.h index a8bde363e3397..fa2a4c52ae6b6 100644 --- a/clang/lib/Sema/SemaLifetimeSafety.h +++ b/clang/lib/Sema/SemaLifetimeSafety.h @@ -100,6 +100,25 @@ class LifetimeSafetySemaHelperImpl : public LifetimeSafetySemaHelper { << UseExpr->getSourceRange(); } + void reportUseAfterScope(const Expr *IssueExpr, SourceLocation UseLoc, + const Expr *MovedExpr, SourceLocation FreeLoc, + llvm::ArrayRef<const Expr *> ExprChain) override { + unsigned DiagID = MovedExpr + ? diag::warn_lifetime_safety_use_after_scope_moved + : diag::warn_lifetime_safety_use_after_scope; + + S.Diag(IssueExpr->getExprLoc(), DiagID) + << getDiagSubjectDescription(IssueExpr) << IssueExpr->getSourceRange(); + if (MovedExpr) + S.Diag(MovedExpr->getExprLoc(), diag::note_lifetime_safety_moved_here) + << MovedExpr->getSourceRange(); + S.Diag(FreeLoc, diag::note_lifetime_safety_destroyed_here); + + reportAliasingChain(ExprChain); + + S.Diag(UseLoc, diag::note_lifetime_safety_used_here); + } + void reportUseAfterReturn(const Expr *IssueExpr, const Expr *ReturnExpr, const Expr *MovedExpr) override { unsigned DiagID = MovedExpr @@ -193,6 +212,34 @@ class LifetimeSafetySemaHelperImpl : public LifetimeSafetySemaHelper { S.Diag(UseExpr->getExprLoc(), diag::note_lifetime_safety_used_here) << UseExpr->getSourceRange(); } + void reportUseAfterInvalidation(const Expr *IssueExpr, SourceLocation UseLoc, + const Expr *InvalidationExpr) override { + auto WarnDiag = isa<CXXDeleteExpr>(InvalidationExpr) + ? diag::warn_lifetime_safety_use_after_free + : diag::warn_lifetime_safety_invalidation; + auto UseDiag = isa<CXXDeleteExpr>(InvalidationExpr) + ? diag::note_lifetime_safety_freed_here + : diag::note_lifetime_safety_invalidated_here; + S.Diag(IssueExpr->getExprLoc(), WarnDiag) + << getDiagSubjectDescription(IssueExpr) << IssueExpr->getSourceRange(); + S.Diag(InvalidationExpr->getExprLoc(), UseDiag) + << InvalidationExpr->getSourceRange(); + S.Diag(UseLoc, diag::note_lifetime_safety_used_here); + } + void reportUseAfterInvalidation(const ParmVarDecl *PVD, SourceLocation UseLoc, + const Expr *InvalidationExpr) override { + auto WarnDiag = isa<CXXDeleteExpr>(InvalidationExpr) + ? diag::warn_lifetime_safety_use_after_free + : diag::warn_lifetime_safety_invalidation; + auto UseDiag = isa<CXXDeleteExpr>(InvalidationExpr) + ? diag::note_lifetime_safety_freed_here + : diag::note_lifetime_safety_invalidated_here; + S.Diag(PVD->getSourceRange().getBegin(), WarnDiag) + << getDiagSubjectDescription(PVD) << PVD->getSourceRange(); + S.Diag(InvalidationExpr->getExprLoc(), UseDiag) + << InvalidationExpr->getSourceRange(); + S.Diag(UseLoc, diag::note_lifetime_safety_used_here); + } void reportInvalidatedField(const Expr *IssueExpr, const FieldDecl *DanglingField, diff --git a/clang/test/Sema/LifetimeSafety/invalidations.cpp b/clang/test/Sema/LifetimeSafety/invalidations.cpp index 301822f066de8..47019c8754d96 100644 --- a/clang/test/Sema/LifetimeSafety/invalidations.cpp +++ b/clang/test/Sema/LifetimeSafety/invalidations.cpp @@ -920,3 +920,27 @@ void invalid_after_ternary_reset(bool flag) { } } // namespace unique_ptr_invalidation + +// A non-trivial destructor at scope exit is modeled as an implicit use (a +// UseFact with no source expression). The live-origins join helper reads such a +// fact's explicit location rather than dereferencing its null use-expression, so +// a function with both a tracked borrow and such an implicit use (e.g. a +// std::function local) is analyzed without crashing. +namespace implicit_use_join { +void view_and_callable() { + std::string text = "long enough heap-allocated backing string value here!"; + std::string_view tok = text; // a tracked borrow (live origin) + std::function<void()> c = [] {}; // non-trivial dtor at scope exit + (void)c; + (void)tok.size(); // no-warning (must not crash) +} + +void view_and_callable_mutation() { + std::string text = "long enough heap-allocated backing string value here!"; + std::string_view tok = text; // expected-warning {{local variable 'text' is later invalidated}} + std::function<void()> c = [] {}; + (void)c; + text.push_back('x'); // expected-note {{invalidated here}} + (void)tok.size(); // expected-note {{later used here}} +} +} // namespace implicit_use_join diff --git a/clang/test/Sema/LifetimeSafety/safety.cpp b/clang/test/Sema/LifetimeSafety/safety.cpp index 7a2644e46a6e1..dd77121904ddb 100644 --- a/clang/test/Sema/LifetimeSafety/safety.cpp +++ b/clang/test/Sema/LifetimeSafety/safety.cpp @@ -3866,3 +3866,53 @@ struct [[gsl::Pointer()]] PtrWithInt { int x; }; PtrWithInt f() { return PtrWithInt{10}; } + +// A scope-exit destructor or cleanup callback may read a borrow the object +// holds. The analysis never sees the out-of-line body, so it is modeled as a +// use of the object: a borrowed-from object destroyed earlier (reverse- +// declaration order) is reported. +namespace ScopeExitUse { +struct [[gsl::Pointer]] Ref { + std::string_view sv; + Ref() = default; + Ref &operator=(std::string_view s [[clang::lifetime_capture_by(this)]]); + ~Ref(); // non-trivial, out-of-line: may read sv +}; + +void dtor_reverse_order() { + Ref r; // destroyed LAST + std::string backing; // destroyed FIRST + r = backing; // expected-warning {{local variable 'backing' does not live long enough}} +} // expected-note {{destroyed here}} expected-note {{later used here}} + +void dtor_safe_order() { + std::string backing; + Ref r; + r = backing; // no-warning +} + +struct [[gsl::Pointer]] TrivialRef { + std::string_view sv; + TrivialRef &operator=(std::string_view s [[clang::lifetime_capture_by(this)]]); + // trivial destructor cannot read sv +}; +void trivial_dtor_no_use() { + TrivialRef r; + std::string backing; + r = backing; // no-warning +} + +void cleanup_ref(Ref *r); // out-of-line: may read r->sv +void cleanup_reverse_order() { + Ref g __attribute__((cleanup(cleanup_ref))); // cleaned up LAST // expected-note {{later used here}} + std::string backing; // destroyed FIRST + g = backing; // expected-warning {{local variable 'backing' does not live long enough}} +} // expected-note {{destroyed here}} + +void cleanup_safe_order() { + std::string backing; + Ref g __attribute__((cleanup(cleanup_ref))); + g = backing; // no-warning +} +} // namespace ScopeExitUse + `````````` </details> https://github.com/llvm/llvm-project/pull/204650 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
