https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/207520
>From df5a10d9d558f33a5b4217f987e4edd1aae5248a Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <[email protected]> Date: Sat, 4 Jul 2026 16:25:06 +0000 Subject: [PATCH] [LifetimeSafety] Support field-sensitivity in lifetime tracking This patch enables field-sensitivity when tracking lifetimes of nested objects. - FactsGenerator now generates `PathElement::getField` for `MemberExpr` accesses, mapping fields to loans. - LoanPropagation now propagates field paths along flow facts, appending fields to base loans. - Removes false-positive warnings in `invalidations.cpp` where modifications to one field were incorrectly reported as invalidating iterators/pointers to another field. - Adds comprehensive unit tests checking nested field access and placeholder fields. TAG=agy CONV=2cfd8d00-18d7-4a03-8d78-2aba2f9a8f23 --- .../LifetimeSafety/FactsGenerator.cpp | 6 +- .../LifetimeSafety/LoanPropagation.cpp | 38 +++++- .../Sema/LifetimeSafety/invalidations.cpp | 90 +++++++++++++-- .../unittests/Analysis/LifetimeSafetyTest.cpp | 109 ++++++++++++------ 4 files changed, 193 insertions(+), 50 deletions(-) diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 0c39f64b939d0..2b80da34f356e 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -287,11 +287,11 @@ void FactsGenerator::VisitMemberExpr(const MemberExpr *ME) { assert(Dst && "Field member should have an origin list as it is GL value"); OriginList *Src = getOriginsList(*ME->getBase()); assert(Src && "Base expression should be a pointer/reference type"); - // Flow loans from base to field, but do not append field path element yet - // (NFC). + // Flow loans from base to field, extending each loan's path with the field. + // E.g., if base has loan to `obj`, field gets loan to `obj.field`. CurrentBlockFacts.push_back(FactMgr.createFact<OriginFlowFact>( Dst->getOuterOriginID(), Src->getOuterOriginID(), - /*Kill=*/true, std::nullopt)); + /*Kill=*/true, PathElement::getField(FD))); } } diff --git a/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp b/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp index a67b1b3c0f826..762042d361a52 100644 --- a/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp @@ -176,14 +176,28 @@ class AnalysisImpl /// A flow from source to destination. If `KillDest` is true, this replaces /// the destination's loans with the source's. Otherwise, the source's loans /// are merged into the destination's. + /// If OriginFlowFact has a PathElement, loans from source are extended + /// before propagating (e.g., loan to `x` becomes loan to `x.field`). Lattice transfer(Lattice In, const OriginFlowFact &F) { OriginID DestOID = F.getDestOriginID(); OriginID SrcOID = F.getSrcOriginID(); + LoanSet SrcLoans = getLoans(In, SrcOID); + LoanSet LoansToFlow = SrcLoans; + + // Extend loans if a path element is specified (e.g., for field access). + if (auto Element = F.getPathElement()) { + LoansToFlow = LoanSetFactory.getEmptySet(); + for (LoanID LID : SrcLoans) { + Loan *ExtendedLoan = + FactMgr.getLoanMgr().getOrCreateExtendedLoan(LID, *Element); + LoansToFlow = LoanSetFactory.add(LoansToFlow, ExtendedLoan->getID()); + } + } + LoanSet DestLoans = F.getKillDest() ? LoanSetFactory.getEmptySet() : getLoans(In, DestOID); - LoanSet SrcLoans = getLoans(In, SrcOID); - LoanSet MergedLoans = utils::join(DestLoans, SrcLoans, LoanSetFactory); + LoanSet MergedLoans = utils::join(DestLoans, LoansToFlow, LoanSetFactory); return setLoans(In, DestOID, MergedLoans); } @@ -208,6 +222,7 @@ class AnalysisImpl assert(getLoans(StartOID, StartPoint).contains(TargetLoan) && "TargetLoan must be present in the StartOID at the StartPoint"); + LoanID CurrLoanID = TargetLoan; OriginID CurrOID = StartOID; llvm::SmallVector<OriginID> OriginFlowChain; llvm::ArrayRef<const Fact *> Facts = FactMgr.getBlockContaining(StartPoint); @@ -217,7 +232,7 @@ class AnalysisImpl for (const Fact *F : llvm::reverse(llvm::make_range(Facts.begin(), StartIt))) { if (const auto *IF = F->getAs<IssueFact>()) - if (IF->getLoanID() == TargetLoan) { + if (IF->getLoanID() == CurrLoanID) { assert(IF->getOriginID() == CurrOID); return OriginFlowChain; } @@ -229,8 +244,23 @@ class AnalysisImpl continue; const OriginID SrcOriginID = OFF->getSrcOriginID(); - if (!getLoans(SrcOriginID, OFF).contains(TargetLoan)) + std::optional<LoanID> NextLoanID; + if (auto AddPath = OFF->getPathElement()) { + auto Candidates = + FactMgr.getLoanMgr().getBaseLoans(CurrLoanID, *AddPath); + for (LoanID Candidate : Candidates) { + if (getLoans(SrcOriginID, OFF).contains(Candidate)) { + NextLoanID = Candidate; + break; + } + } + } else { + if (getLoans(SrcOriginID, OFF).contains(CurrLoanID)) + NextLoanID = CurrLoanID; + } + if (!NextLoanID) continue; + CurrLoanID = *NextLoanID; OriginFlowChain.push_back(SrcOriginID); CurrOID = SrcOriginID; } diff --git a/clang/test/Sema/LifetimeSafety/invalidations.cpp b/clang/test/Sema/LifetimeSafety/invalidations.cpp index be1acc6bc7fbc..1572e78a27e14 100644 --- a/clang/test/Sema/LifetimeSafety/invalidations.cpp +++ b/clang/test/Sema/LifetimeSafety/invalidations.cpp @@ -485,13 +485,12 @@ void ConditionalFieldInvalidatesIterator(bool flag) { (flag ? s.strings1 : s.strings2).push_back("1"); *it; } -// FIXME: Requires field-sensitive AccessPaths to fix. void Invalidate1Use2ViaRefIsOk() { S s; - auto it = s.strings2.begin(); // expected-warning {{local variable 's' is later invalidated}} + auto it = s.strings2.begin(); auto& strings1 = s.strings1; - strings1.push_back("1"); // expected-note {{local variable 's' is invalidated here}} - *it; // expected-note {{later used here}} + strings1.push_back("1"); // OK + *it; } void Invalidate1UseSIsOk() { S s; @@ -894,12 +893,11 @@ struct StringOwner { std::string s, t; }; -// FIXME: False-positive void member_destructor_invalidates_pointer() { StringOwner owner = {"42", "43"}; - const char *p = owner.s.data(); // expected-warning {{local variable 'owner' is later invalidated}} - owner.t.~basic_string(); // expected-note {{local variable 'owner' is invalidated here}} - (void)*p; // expected-note {{later used here}} + const char *p = owner.s.data(); + owner.t.~basic_string(); // OK + (void)*p; } } // namespace explicit_destructor @@ -937,3 +935,79 @@ void invalid_after_ternary_reset(bool flag) { } } // namespace unique_ptr_invalidation + +namespace DeepFieldNesting { +struct Level3 { + std::vector<std::string> vec; + int x; +}; +struct Level2 { + Level3 inner3_1; + Level3 inner3_2; +}; +struct Level1 { + Level2 inner2_1; + Level2 inner2_2; +}; + +// Modifying sibling at Level 3: OK +void SiblingLevel3Ok() { + Level1 obj; + auto it = obj.inner2_1.inner3_1.vec.begin(); + obj.inner2_1.inner3_2.vec.push_back("1"); + *it; +} + +// Modifying sibling at Level 2: OK +void SiblingLevel2Ok() { + Level1 obj; + auto it = obj.inner2_1.inner3_1.vec.begin(); + obj.inner2_2.inner3_1.vec.push_back("1"); + *it; +} + +// Modifying sibling non-container field at Level 3: OK +void SiblingFieldLevel3Ok() { + Level1 obj; + auto it = obj.inner2_1.inner3_1.vec.begin(); + obj.inner2_1.inner3_1.x = 42; + *it; +} + +// Modifying parent structure after use: OK +void ParentModifiedAfterUseOk() { + Level1 obj; + auto it = obj.inner2_1.inner3_1.vec.begin(); + *it; // Use here + Level3 new_val; + obj.inner2_1.inner3_1 = new_val; // OK, because 'it' is no longer used! +} + +// Pointers with sibling modification: OK +void PointerSiblingLevel3Ok(Level1* ptr) { + auto it = ptr->inner2_1.inner3_1.vec.begin(); + ptr->inner2_1.inner3_2.vec.push_back("1"); // OK + *it; +} + +// References with sibling modification: OK +void ReferenceSiblingLevel3Ok(Level1& ref) { + auto it = ref.inner2_1.inner3_1.vec.begin(); + ref.inner2_1.inner3_2.vec.push_back("1"); // OK + *it; +} +} // namespace DeepFieldNesting + +namespace StructFieldDisambiguation { +struct S { + std::vector<int> v; + int x; +}; + +void TestStructVsField(S& s) { + int* px = &s.x; + s.v.push_back(1); // Invalidates s.v.* (interior), but must NOT invalidate s.x + *px = 42; // OK +} +} // namespace StructFieldDisambiguation + diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp b/clang/unittests/Analysis/LifetimeSafetyTest.cpp index 78b7449958140..e13b8f88cb834 100644 --- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp +++ b/clang/unittests/Analysis/LifetimeSafetyTest.cpp @@ -12,6 +12,7 @@ #include "clang/Analysis/Analyses/LifetimeSafety/Loans.h" #include "clang/Testing/TestAST.h" #include "llvm/ADT/StringMap.h" +#include "llvm/Support/raw_ostream.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include <optional> @@ -136,11 +137,16 @@ class LifetimeTestHelper { } bool isLoanToATemporary(LoanID LID) { - return Analysis.getFactManager() - .getLoanMgr() - .getLoan(LID) - ->getAccessPath() - .getAsMaterializeTemporaryExpr() != nullptr; + const Loan *L = Analysis.getFactManager().getLoanMgr().getLoan(LID); + return L->getAccessPath().getAsMaterializeTemporaryExpr() != nullptr; + } + + std::string getAccessPathString(LoanID LID) { + const Loan *L = Analysis.getFactManager().getLoanMgr().getLoan(LID); + std::string S; + llvm::raw_string_ostream OS(S); + L->getAccessPath().dump(OS); + return S; } // Gets the set of loans that are live at the given program point. A loan is @@ -287,7 +293,7 @@ class OriginsInfo { /// variable expected to be the source of a loan. /// \param Annotation A string identifying the program point (created with /// POINT()) where the check should be performed. -MATCHER_P2(HasLoansToImpl, LoanVars, Annotation, "") { +MATCHER_P2(HasLoansToImpl, LoanPathStrs, Annotation, "") { const OriginInfo &Info = arg; std::optional<OriginID> OIDOpt = Info.Helper.getOriginForDecl(Info.OriginVar); if (!OIDOpt) { @@ -303,36 +309,12 @@ MATCHER_P2(HasLoansToImpl, LoanVars, Annotation, "") { << Annotation << "'"; return false; } - std::vector<LoanID> ActualLoans(ActualLoansSetOpt->begin(), - ActualLoansSetOpt->end()); - - std::vector<LoanID> ExpectedLoans; - for (const auto &LoanVar : LoanVars) { - std::vector<LoanID> ExpectedLIDs = Info.Helper.getLoansForVar(LoanVar); - if (ExpectedLIDs.empty()) { - *result_listener << "could not find loan for var '" << LoanVar << "'"; - return false; - } - ExpectedLoans.insert(ExpectedLoans.end(), ExpectedLIDs.begin(), - ExpectedLIDs.end()); - } - std::sort(ExpectedLoans.begin(), ExpectedLoans.end()); - std::sort(ActualLoans.begin(), ActualLoans.end()); - if (ExpectedLoans != ActualLoans) { - *result_listener << "Expected: {"; - for (const auto &LoanID : ExpectedLoans) { - *result_listener << LoanID.Value << ", "; - } - *result_listener << "} Actual: {"; - for (const auto &LoanID : ActualLoans) { - *result_listener << LoanID.Value << ", "; - } - *result_listener << "}"; - return false; - } + std::vector<std::string> ActualLoanPaths; + for (LoanID LID : *ActualLoansSetOpt) + ActualLoanPaths.push_back(Info.Helper.getAccessPathString(LID)); - return ExplainMatchResult(UnorderedElementsAreArray(ExpectedLoans), - ActualLoans, result_listener); + return ExplainMatchResult(UnorderedElementsAreArray(LoanPathStrs), + ActualLoanPaths, result_listener); } enum class LivenessKindFilter { Maybe, Must, All }; @@ -1213,6 +1195,63 @@ TEST_F(LifetimeAnalysisTest, LifetimeboundConversionOperator) { EXPECT_THAT(Origin("v"), HasLoansTo({"owner"}, "p1")); } +TEST_F(LifetimeAnalysisTest, NestedFieldAccess) { + SetupTest(R"( + struct Inner { int val; }; + struct Outer { Inner f; }; + void target() { + Outer o; + Outer *p = &o; + int* p1 = &o.f.val; + POINT(a); + int* p2 = &p->f.val; + POINT(b); + } + )"); + EXPECT_THAT(Origin("p1"), HasLoansTo({"o.f.val"}, "a")); + EXPECT_THAT(Origin("p2"), HasLoansTo({"o.f.val"}, "b")); +} + +TEST_F(LifetimeAnalysisTest, PlaceholderParamField) { + SetupTest(R"( + struct S { int val; }; + void target(S* p) { + int* p1 = &p->val; + POINT(a); + } + )"); + EXPECT_THAT(Origin("p1"), HasLoansTo({"$p.val"}, "a")); +} + +TEST_F(LifetimeAnalysisTest, PlaceholderThisField) { + SetupTest(R"( + struct S { + int f; + void target() { + int* p1 = &f; + POINT(a); + } + }; + )"); + EXPECT_THAT(Origin("p1"), HasLoansTo({"$this.f"}, "a")); +} + +TEST_F(LifetimeAnalysisTest, PlaceholderThisNestedField) { + SetupTest(R"( + struct S1 { + int f; + }; + struct S { + S1 s1; + void target() { + int* p1 = &s1.f; + POINT(a); + } + }; + )"); + EXPECT_THAT(Origin("p1"), HasLoansTo({"$this.s1.f"}, "a")); +} + TEST_F(LifetimeAnalysisTest, LivenessDeadPointer) { SetupTest(R"( void target() { _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
