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> reportUseAfterReturn did not receive the aliasing chain, so a returned dangling value was reported without saying which calls or variables carried the borrow, unlike use-after-scope. Build the chain from the escaping origin with a new buildOriginFlowChain overload for escape facts. Like the UseFact overload, it drops the casts that just load the returned variable, which would otherwise repeat the "returned here" note. Assisted by: Opus 5.5 --- Patch is 47.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/226395.diff 9 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h (+4-3) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h (+6) - (modified) clang/lib/Analysis/LifetimeSafety/Checker.cpp (+5-3) - (modified) clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp (+19-4) - (modified) clang/lib/Sema/SemaLifetimeSafety.h (+5-1) - (modified) clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp (+1-1) - (modified) clang/test/Sema/LifetimeSafety/explicit-object-param-no-crash.cpp (+2-2) - (modified) clang/test/Sema/LifetimeSafety/nocfg.cpp (+24-24) - (modified) clang/test/Sema/LifetimeSafety/safety.cpp (+45-45) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h index 18e5e8473e414..6854902059e50 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h @@ -74,11 +74,12 @@ class LifetimeSafetySemaHelper { SourceLocation FreeLoc, llvm::ArrayRef<const Expr *> ExprChain) {} - // TODO: Pass the expiry location and aliasing chain like - // reportUseAfterScope. + // TODO: Report where the object was destroyed when that happens before the + // return (inner scopes, temporaries). virtual void reportUseAfterReturn(const Expr *IssueExpr, const Expr *ReturnExpr, - const Expr *MovedExpr) {} + const Expr *MovedExpr, + llvm::ArrayRef<const Expr *> ExprChain) {} virtual void reportDanglingField(const Expr *IssueExpr, const FieldDecl *Field, diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h index d46712ce2b1a5..de7eca39228ca 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LoanPropagation.h @@ -57,6 +57,12 @@ class LoanPropagationAnalysis { const LoanID TargetLoan, const CFG *Cfg) const; + /// Like the above, starting from the origin \p OEF lets escape. Empty if it + /// does not hold \p TargetLoan. + llvm::SmallVector<OriginID> buildOriginFlowChain(const OriginEscapesFact *OEF, + const LoanID TargetLoan, + const CFG *Cfg) const; + private: class Impl; std::unique_ptr<Impl> PImpl; diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp index c4cc872dcd568..30ae9961781c8 100644 --- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp @@ -330,11 +330,13 @@ class LifetimeChecker { // FIXME: Diagnose invalidated return escapes separately. } else llvm_unreachable("Unhandled OriginEscapesFact type"); - } else if (const auto *RetEscape = dyn_cast<ReturnEscapeFact>(OEF)) + } else if (const auto *RetEscape = dyn_cast<ReturnEscapeFact>(OEF)) { // Return stack address. SemaHelper->reportUseAfterReturn( - IssueExpr, RetEscape->getReturnExpr(), MovedExpr); - else if (const auto *FieldEscape = dyn_cast<FieldEscapeFact>(OEF)) { + IssueExpr, RetEscape->getReturnExpr(), MovedExpr, + getExprChain( + LoanPropagation.buildOriginFlowChain(OEF, LID, Cfg))); + } else if (const auto *FieldEscape = dyn_cast<FieldEscapeFact>(OEF)) { // Dangling field. bool IsCapturedByLambda = FactMgr.isFieldCapturedByLambda(FieldEscape->getFieldDecl()); diff --git a/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp b/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp index 80ba7d08a3103..df66f029a5400 100644 --- a/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp @@ -230,18 +230,27 @@ class AnalysisImpl for (const OriginList *Cur = UF->getUsedOrigins(); Cur; Cur = Cur->peelOuterOrigin()) if (getLoans(Cur->getOuterOriginID(), UF).contains(TargetLoan)) - return dropLoadsInUse( + return dropLeadingLoads( buildOriginFlowChain(UF, Cur->getOuterOriginID(), TargetLoan, Cfg)); return {}; } + llvm::SmallVector<OriginID> buildOriginFlowChain(const OriginEscapesFact *OEF, + const LoanID TargetLoan, + const CFG *Cfg) const { + OriginID OID = OEF->getEscapedOriginID(); + if (!getLoans(OID, OEF).contains(TargetLoan)) + return {}; + return dropLeadingLoads(buildOriginFlowChain(OEF, OID, TargetLoan, Cfg)); + } + private: /// An expression's origin only receives loans from its subexpressions, so - /// until the chain reaches a declaration it is inside the use expression. - /// Casts there just load the used variable, so drop them. + /// until the chain reaches a declaration it is inside the expression the + /// chain starts from. Casts there just load its variable, so drop them. llvm::SmallVector<OriginID> - dropLoadsInUse(llvm::SmallVector<OriginID> Chain) const { + dropLeadingLoads(llvm::SmallVector<OriginID> Chain) const { const OriginManager &OM = FactMgr.getOriginMgr(); auto FirstDecl = llvm::find_if( Chain, [&](OriginID OID) { return !OM.getOrigin(OID).getExpr(); }); @@ -346,4 +355,10 @@ llvm::SmallVector<OriginID> LoanPropagationAnalysis::buildOriginFlowChain( const UseFact *UF, const LoanID TargetLoan, const CFG *Cfg) const { return PImpl->buildOriginFlowChain(UF, TargetLoan, Cfg); } +llvm::SmallVector<OriginID> +LoanPropagationAnalysis::buildOriginFlowChain(const OriginEscapesFact *OEF, + const LoanID TargetLoan, + const CFG *Cfg) const { + return PImpl->buildOriginFlowChain(OEF, TargetLoan, Cfg); +} } // namespace clang::lifetimes::internal diff --git a/clang/lib/Sema/SemaLifetimeSafety.h b/clang/lib/Sema/SemaLifetimeSafety.h index 620032c27f955..e16aea60d2319 100644 --- a/clang/lib/Sema/SemaLifetimeSafety.h +++ b/clang/lib/Sema/SemaLifetimeSafety.h @@ -158,7 +158,8 @@ class LifetimeSafetySemaHelperImpl : public LifetimeSafetySemaHelper { } void reportUseAfterReturn(const Expr *IssueExpr, const Expr *ReturnExpr, - const Expr *MovedExpr) override { + const Expr *MovedExpr, + llvm::ArrayRef<const Expr *> ExprChain) override { unsigned DiagID = MovedExpr ? diag::warn_lifetime_safety_return_stack_addr_moved : diag::warn_lifetime_safety_return_stack_addr; @@ -169,6 +170,9 @@ class LifetimeSafetySemaHelperImpl : public LifetimeSafetySemaHelper { if (MovedExpr) S.Diag(MovedExpr->getExprLoc(), diag::note_lifetime_safety_moved_here) << MovedExpr->getSourceRange(); + + reportAliasingChain(ExprChain); + S.Diag(ReturnExpr->getExprLoc(), diag::note_lifetime_safety_returned_here) << ReturnExpr->getSourceRange(); } diff --git a/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp b/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp index 22fe5e6bddfb5..4deb6d1f8688d 100644 --- a/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp +++ b/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp @@ -550,7 +550,7 @@ struct CaptureViewToView { CaptureViewToView test_view_to_view() { MyObj obj; View v(obj); // expected-warning {{stack memory associated with local variable 'obj' is returned}} - CaptureViewToView x(v); + CaptureViewToView x(v); // expected-note {{local variable 'v' aliases the storage of local variable 'obj'}} return x; // expected-note {{returned here}} } diff --git a/clang/test/Sema/LifetimeSafety/explicit-object-param-no-crash.cpp b/clang/test/Sema/LifetimeSafety/explicit-object-param-no-crash.cpp index 71c86b9f793fb..5fb42cd7c9258 100644 --- a/clang/test/Sema/LifetimeSafety/explicit-object-param-no-crash.cpp +++ b/clang/test/Sema/LifetimeSafety/explicit-object-param-no-crash.cpp @@ -75,13 +75,13 @@ const int *object_arg_is_not_moved(Holder &&h [[clang::lifetimebound]]) { } const int *t1(Holder h) { - const int *ptr = h.borrow(); // expected-warning {{stack memory associated with parameter 'h' is returned}} + const int *ptr = h.borrow(); // expected-warning {{stack memory associated with parameter 'h' is returned}} expected-note {{result of call to 'borrow' aliases the storage of parameter 'h' because the implicit object parameter is marked as lifetimebound}} static_cast<Holder &&>(h).consume(); return ptr; // expected-note {{returned here}} } const int *t2(Holder h) { - const int *ptr = h.borrow(); // expected-warning {{stack memory associated with parameter 'h' is returned}} + const int *ptr = h.borrow(); // expected-warning {{stack memory associated with parameter 'h' is returned}} expected-note {{result of call to 'borrow' aliases the storage of parameter 'h' because the implicit object parameter is marked as lifetimebound}} std::move(h).consume(); return ptr; // expected-note {{returned here}} } diff --git a/clang/test/Sema/LifetimeSafety/nocfg.cpp b/clang/test/Sema/LifetimeSafety/nocfg.cpp index 7f4a58c1836dd..6986d1d61e897 100644 --- a/clang/test/Sema/LifetimeSafety/nocfg.cpp +++ b/clang/test/Sema/LifetimeSafety/nocfg.cpp @@ -179,7 +179,7 @@ struct LifetimeBoundCtor { }; auto lifetimebound_make_unique_single_param() { - return std::make_unique<LifetimeBoundCtor>(MyIntOwner{}); // tu-warning {{stack memory associated with temporary object is returned}} tu-note {{returned here}} + return std::make_unique<LifetimeBoundCtor>(MyIntOwner{}); // tu-warning {{stack memory associated with temporary object is returned}} tu-note {{returned here}} tu-note {{result of call to 'make_unique<LifetimeBoundCtor, MyIntOwner>' aliases the storage of temporary object because parameter 'args' is inferred as lifetimebound}} } @@ -255,14 +255,14 @@ std::string_view containerWithAnnotatedElements() { use(c2); std::vector<std::string> local; - return local.at(0); // expected-warning {{address of stack memory associated with local variable}} \ + return local.at(0); // expected-warning {{address of stack memory associated with local variable}} cfg-note {{result of call to 'at' aliases the storage of local variable 'local' because the implicit object parameter is inferred as lifetimebound}} \ // cfg-warning {{stack memory associated with local variable 'local' is returned}} cfg-note {{returned here}} } std::string_view localUniquePtr(int i) { std::unique_ptr<std::string> c1; if (i) - return *c1; // expected-warning {{address of stack memory associated with local variable}} \ + return *c1; // expected-warning {{address of stack memory associated with local variable}} cfg-note {{result of call to 'operator*' aliases the storage of local variable 'c1' because the implicit object parameter is inferred as lifetimebound}} \ // cfg-warning {{stack memory associated with local variable 'c1' is returned}} cfg-note {{returned here}} std::unique_ptr<std::string_view> c2; return *c2; // expect no-warning. @@ -271,7 +271,7 @@ std::string_view localUniquePtr(int i) { std::string_view localOptional(int i) { std::optional<std::string> o; if (i) - return o.value(); // expected-warning {{address of stack memory associated with local variable}} \ + return o.value(); // expected-warning {{address of stack memory associated with local variable}} cfg-note {{result of call to 'value' aliases the storage of local variable 'o' because the implicit object parameter is inferred as lifetimebound}} \ // cfg-warning {{stack memory associated with local variable 'o' is returned}} cfg-note {{returned here}} std::optional<std::string_view> abc; return abc.value(); // expect no warning @@ -295,14 +295,14 @@ int *danglingUniquePtrFromTemp2() { } const int& danglingRefToOptionalFromTemp3() { - return std::optional<int>().value(); // expected-warning {{returning reference to local temporary object}} \ + return std::optional<int>().value(); // expected-warning {{returning reference to local temporary object}} cfg-note {{result of call to 'value' aliases the storage of temporary object because the implicit object parameter is inferred as lifetimebound}} \ // cfg-warning {{stack memory associated with temporary object is returned}} cfg-note {{returned here}} } std::optional<std::string> getTempOptStr(); std::string_view danglingRefToOptionalFromTemp4() { - return getTempOptStr().value(); // expected-warning {{returning address of local temporary object}} \ + return getTempOptStr().value(); // expected-warning {{returning address of local temporary object}} cfg-note {{result of call to 'value' aliases the storage of temporary object because the implicit object parameter is inferred as lifetimebound}} \ // cfg-warning {{stack memory associated with temporary object is returned}} cfg-note {{returned here}} } @@ -355,7 +355,7 @@ int &usedToBeFalsePositive(std::vector<int> &v) { int &doNotFollowReferencesForLocalOwner() { // Warning caught by CFG analysis. std::unique_ptr<int> localOwner; - int &p = *localOwner // cfg-warning {{stack memory associated with local variable 'localOwner' is returned}} + int &p = *localOwner // cfg-warning {{stack memory associated with local variable 'localOwner' is returned}} cfg-note {{result of call to 'get' aliases the storage of local variable 'localOwner' because the implicit object parameter is inferred as lifetimebound}} .get(); return p; // cfg-note {{returned here}} } @@ -456,11 +456,11 @@ std::vector<std::string_view> GetTemporaryView(); std::string_view test_str_local() { std::vector<std::string> v; - return *std::find(v.begin(), // cfg-warning {{stack memory associated with local variable 'v' is returned}} cfg-note {{returned here}} + return *std::find(v.begin(), // cfg-warning {{stack memory associated with local variable 'v' is returned}} cfg-note {{returned here}} cfg-note {{result of call to 'begin' aliases the storage of local variable 'v' because the implicit object parameter is inferred as lifetimebound}} cfg-note {{result of call to 'find<__gnu_cxx::basic_iterator<std::basic_string<char>>, char[3]>' aliases the storage of local variable 'v' because parameter 'first' is inferred as lifetimebound}} cfg-note {{result of call to 'operator*' aliases the storage of local variable 'v' because the implicit object parameter is inferred as lifetimebound}} v.end(), "42"); } std::string_view test_str_temporary() { - return *std::find(GetTemporaryString().begin(), // cfg-warning {{stack memory associated with temporary object is returned}} cfg-note {{returned here}} + return *std::find(GetTemporaryString().begin(), // cfg-warning {{stack memory associated with temporary object is returned}} cfg-note {{returned here}} cfg-note {{result of call to 'begin' aliases the storage of temporary object because the implicit object parameter is inferred as lifetimebound}} cfg-note {{result of call to 'find<__gnu_cxx::basic_iterator<std::basic_string<char>>, char[3]>' aliases the storage of temporary object because parameter 'first' is inferred as lifetimebound}} cfg-note {{result of call to 'operator*' aliases the storage of temporary object because the implicit object parameter is inferred as lifetimebound}} GetTemporaryString().end(), "42"); } std::string_view test_view() { @@ -594,10 +594,10 @@ struct FooView { }; FooView test3(int i, std::optional<Foo> a) { if (i) - return *a; // expected-warning {{address of stack memory}} \ + return *a; // expected-warning {{address of stack memory}} cfg-note {{result of call to 'operator*' aliases the storage of parameter 'a' because the implicit object parameter is inferred as lifetimebound}} \ // cfg-warning {{stack memory associated with parameter 'a' is returned}} \ // cfg-note {{returned here}} - return a.value(); // expected-warning {{address of stack memory}} \ + return a.value(); // expected-warning {{address of stack memory}} cfg-note {{result of call to 'value' aliases the storage of parameter 'a' because the implicit object parameter is inferred as lifetimebound}} \ // cfg-warning {{stack memory associated with parameter 'a' is returned}} \ // cfg-note {{returned here}} } @@ -658,7 +658,7 @@ std::string_view test2() { // We expect dangling issues as the conversion operator is marked as lifetimebound。 std::string_view bad = StatusOr<Wrapper2<std::string_view>>().value(); // expected-warning {{temporary whose address is used as value of}} - return k.value(); // expected-warning {{address of stack memory associated}} \ + return k.value(); // expected-warning {{address of stack memory associated}} cfg-note {{result of call to 'value' aliases the storage of local variable 'k' because the implicit object parameter is marked as lifetimebound}} \ // cfg-warning {{stack memory associated with local variable 'k' is returned}} cfg-note {{returned here}} } } // namespace GH108272 @@ -810,7 +810,7 @@ std::vector<int*> test8(StatusOr<std::vector<int*>> aa) { // Pointer<Pointer> from Owner<Owner<Pointer>> Span<int*> test9(StatusOr<std::vector<int*>> aa) { - return aa.valueLB(); // expected-warning {{address of stack memory associated}} \ + return aa.valueLB(); // expected-warning {{address of stack memory associated}} cfg-note {{result of call to 'valueLB' aliases the storage of parameter 'aa' because the implicit object parameter is marked as lifetimebound}} \ // cfg-warning {{stack memory associated with parameter 'aa' is returned}} cfg-note {{returned here}} return aa.valueNoLB(); // OK. } @@ -819,7 +819,7 @@ Span<int*> test9(StatusOr<std::vector<int*>> aa) { // Pointer<Owner>> from Owner<Owner> Span<std::string> test10(StatusOr<std::vector<std::string>> aa) { - return aa.valueLB(); // expected-warning {{address of stack memory}} \ + return aa.valueLB(); // expected-warning {{address of stack memory}} cfg-note {{result of call to 'valueLB' aliases the storage of parameter 'aa' because the implicit object parameter is marked as lifetimebound}} \ // cfg-warning {{stack memory associated with parameter 'aa' is returned}} cfg-note {{returned here}} return aa.valueNoLB(); // OK. } @@ -877,7 +877,7 @@ std::string_view test1_1() { // cfg-warning {{temporary object does not live long enough}} cfg-note {{destroyed here}} \ // cfg-note {{result of call to 'Ref' aliases the storage of temporary object because parameter 'abc' is marked as lifetimebound}} use(t1); // cfg-note {{later used here}} - return Ref(std::string()); // expected-warning {{returning address}} \ + return Ref(std::string()); // expected-warning {{returning address}} cfg-note {{result of call to 'Ref' aliases the storage of temporary object because parameter 'abc' is marked as lifetimebound}} \ // cfg-warning {{stack memory associated with temporary object is returned}} cfg-note {{returned here}} } @@ -931,7 +931,7 @@ std::string_view test2_1(Foo<std::string> r1, Foo<std::string_view> r2) { // cfg-warning {{temporary object does not live long enough}} cfg-note {{destroyed here}} \ // cfg-note {{result of call to 'get' aliases the storage of temporary object because the implicit object parameter is marked as lifetimebound}} use(t1); // cfg-note {{later used here}} - return r1.get(); // expected-warning {{address of stack}} \ + return r1.get(); // expected-warning {{address of stack}} cfg-note {{result of call to 'get' aliases the storage of parameter 'r1' because the implicit object parameter is marked as lifetimebound}} \ // cfg-warning {{stack memory associated with parameter 'r1' is returned}} cfg-note {{returned here}} } std::string_view test2_2(Foo<std::string> r1, Foo<std::string_view> r2) { @@ -999,14 +999,14 @@ void test4() { namespace range_based_for_loop_variables { std::string_view test_view_loop_var(std::vector<std::string> st... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/226395 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
