llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> This patch completes the implementation of path-sensitive lifetime tracking by supporting container interior paths (`.*`) and deep-nested invalidation. - Enables `PathElement::getInterior` generation in `FactsGenerator` for GSL Owners and Views (e.g. member functions, function parameters, lambda captures). - Removes bypass checks in `FactsGenerator::handleInvalidatingCall` to track container invalidation on fields. - Updates `Checker` to use strict prefix comparison (`isStrictPrefixOf`) for container invalidations, ensuring invalidation of container contents (interior) correctly invalidates iterators but not other sibling fields. - Reorganizes tests in `invalidations.cpp` by resolving duplicates and distributing them logically. - Updates unit tests and sema tests with correct expectations for interior paths. TAG=agy CONV=2cfd8d00-18d7-4a03-8d78-2aba2f9a8f23 --- Patch is 37.90 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/207523.diff 5 Files Affected: - (modified) clang/lib/Analysis/LifetimeSafety/Checker.cpp (+9-2) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+17-6) - (modified) clang/test/Sema/LifetimeSafety/Inputs/lifetime-analysis.h (+6-3) - (modified) clang/test/Sema/LifetimeSafety/invalidations.cpp (+223-65) - (modified) clang/unittests/Analysis/LifetimeSafetyTest.cpp (+109-67) ``````````diff diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp index a749a49e7762a..559cc410abd42 100644 --- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp @@ -220,11 +220,18 @@ class LifetimeChecker { /// Get loans directly pointing to the invalidated container LoanSet DirectlyInvalidatedLoans = LoanPropagation.getLoans(InvalidatedOrigin, IOF); + bool AllowEquality = + isa_and_nonnull<CXXDeleteExpr>(IOF->getInvalidationExpr()); auto IsInvalidated = [&](const Loan *L) { for (LoanID InvalidID : DirectlyInvalidatedLoans) { const Loan *InvalidL = FactMgr.getLoanMgr().getLoan(InvalidID); - if (InvalidL->getAccessPath().isPrefixOf(L->getAccessPath())) - return true; + if (AllowEquality) { + if (InvalidL->getAccessPath().isPrefixOf(L->getAccessPath())) + return true; + } else { + if (InvalidL->getAccessPath().isStrictPrefixOf(L->getAccessPath())) + return true; + } } return false; }; diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 2b80da34f356e..d56d073af2d5f 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -962,11 +962,6 @@ void FactsGenerator::handleInvalidatingCall(const Expr *Call, if (!isInvalidationMethod(*MD)) return; - // Heuristics to turn-down false positives. Skip member field expressions for - // now (NFC). - if (!isa<DeclRefExpr>(Args[0]->IgnoreImpCasts())) - return; - OriginList *ThisList = getOriginsList(*Args[0]); if (ThisList) CurrentBlockFacts.push_back(FactMgr.createFact<InvalidateOriginFact>( @@ -1079,6 +1074,22 @@ void FactsGenerator::handleLifetimeCaptureBy(const FunctionDecl *FD, static std::optional<PathElement> getPathElementForLifetimeBoundArg(const FunctionDecl *FD, unsigned ArgIndex, const Expr *ArgExpr) { + if (!ArgExpr) + return std::nullopt; + if (ArgIndex == 0) { + const auto *Method = dyn_cast<CXXMethodDecl>(FD); + bool IsContainerArg = + (Method && Method->isInstance()) || shouldTrackFirstArgument(FD); + if (IsContainerArg) { + QualType ArgType = ArgExpr->getType(); + if (const Type *ArgTypePtr = ArgType.getTypePtrOrNull()) { + if (isGslOwnerType(ArgType) || + (ArgTypePtr->isPointerType() && + isGslOwnerType(ArgTypePtr->getPointeeType()))) + return PathElement::getInterior(); + } + } + } return std::nullopt; } @@ -1161,7 +1172,7 @@ void FactsGenerator::handleFunctionCall(const Expr *Call, // Only flow the outer origin because inner lengths may mismatch. CurrentBlockFacts.push_back(FactMgr.createFact<OriginFlowFact>( CallList->getOuterOriginID(), ArgList->getOuterOriginID(), KillSrc, - std::nullopt)); + PathElement::getInterior())); KillSrc = false; } else if (IsArgLifetimeBound(I)) { // Only flow the outer origin here. For lifetimebound args in diff --git a/clang/test/Sema/LifetimeSafety/Inputs/lifetime-analysis.h b/clang/test/Sema/LifetimeSafety/Inputs/lifetime-analysis.h index 024c3c2bc51b7..815dcf8b6766d 100644 --- a/clang/test/Sema/LifetimeSafety/Inputs/lifetime-analysis.h +++ b/clang/test/Sema/LifetimeSafety/Inputs/lifetime-analysis.h @@ -28,11 +28,11 @@ template<typename T> struct remove_reference<T &> { typedef T type; }; template<typename T> struct remove_reference<T &&> { typedef T type; }; template< class InputIt, class T > -InputIt find( InputIt first, InputIt last, const T& value ); +InputIt find(InputIt first, InputIt last, const T& value); template< class ForwardIt1, class ForwardIt2 > -ForwardIt1 search( ForwardIt1 first, ForwardIt1 last, - ForwardIt2 s_first, ForwardIt2 s_last ); +ForwardIt1 search(ForwardIt1 first, ForwardIt1 last, + ForwardIt2 s_first, ForwardIt2 s_last); template<typename T> typename remove_reference<T>::type &&move(T &&t) noexcept; @@ -225,6 +225,9 @@ struct basic_string { ~basic_string(); basic_string& operator=(const basic_string&); basic_string& operator+=(const basic_string&); + basic_string& append(const basic_string&); + basic_string& replace(unsigned pos, unsigned count, + const basic_string& str); basic_string& operator+=(const T*); void push_back(T); diff --git a/clang/test/Sema/LifetimeSafety/invalidations.cpp b/clang/test/Sema/LifetimeSafety/invalidations.cpp index 1572e78a27e14..46980405e46fd 100644 --- a/clang/test/Sema/LifetimeSafety/invalidations.cpp +++ b/clang/test/Sema/LifetimeSafety/invalidations.cpp @@ -335,37 +335,41 @@ void IteratorInvalidatedThroughPointerParameter(std::vector<int> *v) { // expect (void)it; // expected-note {{later used here}} } +void IteratorInvalidatedThroughPointerParameterNotUsed(std::vector<int> *v) { + // Ok as 'it' is not used. Only 'v' is used. + auto it = v->begin(); + v->push_back(42); + v->push_back(43); + v->clear(); +} + void ParenthesizedContainerInvalidatesIterator() { - // FIXME: Support invalidation through non-DRE lvalue expressions. std::vector<int> v; - auto it = v.begin(); - (v).push_back(42); - (void)it; + auto it = v.begin(); // expected-warning {{local variable 'v' is later invalidated}} + (v).push_back(42); // expected-note {{local variable 'v' is invalidated here}} + (void)it; // expected-note {{later used here}} } } // namespace InvalidatingThroughContainerAliases namespace ContainerObjectAliases { -// FIXME: Distinguish owner-borrow from content-borrow. -void PointerParameterObjectUseIsOk(std::vector<int> *v) { // expected-warning {{parameter 'v' is later invalidated}} - v->push_back(42); // expected-note {{parameter 'v' is invalidated here}} - (void)v; // expected-note {{later used here}} +void PointerParameterObjectUseIsOk(std::vector<int> *v) { + v->push_back(42); + (void)v; } -// FIXME: Distinguish owner-borrow from content-borrow. void LocalPointerAliasObjectUseIsOk() { std::vector<int> vv; - std::vector<int> *v = &vv; // expected-warning {{local variable 'vv' is later invalidated}} - v->push_back(42); // expected-note {{local variable 'vv' is invalidated here}} - (void)*v; // expected-note {{later used here}} + std::vector<int> *v = &vv; + v->push_back(42); + (void)*v; } -// FIXME: Distinguish owner-borrow from content-borrow. void LocalReferenceAliasObjectUseIsOk() { std::vector<int> vv; - std::vector<int> &v = vv; // expected-warning {{local variable 'vv' is later invalidated}} - v.push_back(42); // expected-note {{local variable 'vv' is invalidated here}} - (void)v; // expected-note {{later used here}} + std::vector<int> &v = vv; + v.push_back(42); + (void)v; } } // namespace ContainerObjectAliases @@ -402,18 +406,8 @@ void SelfInvalidatingMap() { // Therefore the following is safe in practice. // On the other hand, std::flat_map (since C++23) does not provide pointer stability on // insertion and following is unsafe for this container. - // FIXME: The warnings below are false positives (self-invalidation of the Owner). - // Modifying a container should not invalidate the container object itself. - // To resolve this, we need to: - // 1. Distinguish owner-borrow (borrowing the container object) from content-borrow (borrowing elements inside the container). - // 2. Make AccessPaths more precise to reason at element/field granularity rather than treating the whole container as a single storage location. - mp[1] = "42"; // expected-warning {{local variable 'mp' is later invalidated}} \ - // expected-note {{local variable 'mp' is invalidated here}} \ - // expected-note {{later used here}} + mp[1] = "42"; mp[2] = mp[1]; // expected-warning {{local variable 'mp' is later invalidated}} \ - // expected-warning {{local variable 'mp' is later invalidated}} \ - // expected-note {{local variable 'mp' is invalidated here}} \ - // expected-note {{later used here}} \ // expected-note {{local variable 'mp' is invalidated here}} \ // expected-note {{later used here}} } @@ -440,6 +434,18 @@ void reassign(std::string str, std::string str2) { str = str2; // expected-note {{parameter 'str' is invalidated here}} (void)view; // expected-note {{later used here}} } + +void append_call(std::string str) { + std::string_view view = str; // expected-warning {{parameter 'str' is later invalidated}} + str.append("456"); // expected-note {{parameter 'str' is invalidated here}} + (void)view; // expected-note {{later used here}} +} + +void replace_call(std::string str) { + std::string_view view = str; // expected-warning {{parameter 'str' is later invalidated}} + str.replace(0, 1, "456"); // expected-note {{parameter 'str' is invalidated here}} + (void)view; // expected-note {{later used here}} +} } // namespace Strings // FIXME: This should be diagnosed as use-after-invalidation but with potential move. @@ -456,14 +462,11 @@ struct S { std::vector<std::string> strings1; std::vector<std::string> strings2; }; -// FIXME: Make Paths more precise to reason at field granularity. -// Currently we only detect invalidations to direct declarations and not members. void Invalidate1Use1IsInvalid() { - // FIXME: Detect this. S s; - auto it = s.strings1.begin(); - s.strings1.push_back("1"); - *it; + auto it = s.strings1.begin(); // expected-warning {{local variable 's' is later invalidated}} + s.strings1.push_back("1"); // expected-note {{local variable 's' is invalidated here}} + *it; // expected-note {{later used here}} } void Invalidate2Use1IsOk() { S s; @@ -472,24 +475,26 @@ void Invalidate2Use1IsOk() { *it; } void ConditionalContainerInvalidatesIterator(bool flag) { - // FIXME: Support invalidation through conditional lvalue expressions. std::vector<int> v1, v2; - auto it = v1.begin(); - (flag ? v1 : v2).push_back(42); - (void)it; + auto it = v1.begin(); // expected-warning {{local variable 'v1' is later invalidated}} + (flag ? v1 : v2).push_back(42); // expected-note {{local variable 'v1' is invalidated here}} + (void)it; // expected-note {{later used here}} } void ConditionalFieldInvalidatesIterator(bool flag) { - // FIXME: Support conditional invalidation through field expressions. S s; - auto it = s.strings1.begin(); - (flag ? s.strings1 : s.strings2).push_back("1"); - *it; + auto it1 = s.strings1.begin(); // expected-warning {{local variable 's' is later invalidated}} + auto it2 = s.strings2.begin(); // expected-warning {{local variable 's' is later invalidated}} + // FIXME: This note is inaccurate. + // It should say 's.strings1' is invalidated. Same for 's.strings2'. + (flag ? s.strings1 : s.strings2).push_back("1"); // expected-note 2 {{local variable 's' is invalidated here}} + *it1; // expected-note {{later used here}} + *it2; // expected-note {{later used here}} } void Invalidate1Use2ViaRefIsOk() { S s; auto it = s.strings2.begin(); auto& strings1 = s.strings1; - strings1.push_back("1"); // OK + strings1.push_back("1"); *it; } void Invalidate1UseSIsOk() { @@ -498,12 +503,11 @@ void Invalidate1UseSIsOk() { s.strings2.push_back("1"); (void)*p; } -// FIXME: Distinguish owner-borrow from content-borrow. void PointerToContainerIsOk() { std::vector<std::string> s; - std::vector<std::string>* p = &s; // expected-warning {{local variable 's' is later invalidated}} - p->push_back("1"); // expected-note {{local variable 's' is invalidated here}} - (void)*p; // expected-note {{later used here}} + std::vector<std::string>* p = &s; + p->push_back("1"); + (void)*p; } void IteratorFromPointerToContainerIsInvalidated() { std::vector<std::string> s; @@ -512,26 +516,50 @@ void IteratorFromPointerToContainerIsInvalidated() { p->push_back("1"); // expected-note {{local variable 's' is invalidated here}} *it; // expected-note {{later used here}} } -// FIXME: Distinguish invalidating an element's contents from invalidating -// iterators into the outer container. void ChangingRegionOwnedByContainerIsOk() { std::vector<std::string> subdirs; - for (std::string& path : subdirs) // expected-warning {{local variable 'subdirs' is later invalidated}} expected-note {{later used here}} - path = std::string(); // expected-note {{local variable 'subdirs' is invalidated here}} + for (std::string& path : subdirs) + path = std::string(); +} + +struct SField { int a; int b;}; +void PointerToVectorElementField() { + std::vector<SField> v = {{1, 2}, {3, 4}}; + int* ptr = &v[0].a; // expected-warning {{local variable 'v' is later invalidated}} + v.resize(100); // expected-note {{local variable 'v' is invalidated here}} + *ptr = 10; // expected-note {{later used here}} +} + +void Invalidate1Use2ViaRefIsInvalid() { + S s; + auto it1 = s.strings1.begin(); + auto it2 = s.strings2.begin(); // expected-warning {{local variable 's' is later invalidated}} + auto& strings2 = s.strings2; + strings2.push_back("1"); // expected-note {{local variable 's' is invalidated here}} + *it1; + *it2; // expected-note {{later used here}} } +void InvalidateBothInASingleExpression(bool cond) { + S s; + auto it1 = s.strings1.begin(); // expected-warning {{local variable 's' is later invalidated}} + auto it2 = s.strings2.begin(); // expected-warning {{local variable 's' is later invalidated}} + auto& both = cond ? s.strings1 : s.strings2; + both.push_back("1"); // expected-note 2 {{local variable 's' is invalidated here}} + *it1; // expected-note {{later used here}} + *it2; // expected-note {{later used here}} +} } // namespace ContainersAsFields namespace InvalidatedField { std::string StableString; -// FIXME: Distinguish owner-borrow from interior-borrow. struct SinkOwnerBorrow { - std::string *dest_; // expected-note {{this field dangles}} + std::string *dest_; - SinkOwnerBorrow(std::string *dest, int n) : dest_(dest) { // expected-warning {{parameter 'dest' escapes to the field 'dest_' and is later invalidated}} + SinkOwnerBorrow(std::string *dest, int n) : dest_(dest) { if (n > 0) - dest->clear(); // expected-note {{parameter 'dest' is invalidated here}} + dest->clear(); } }; @@ -599,6 +627,25 @@ struct S { strings.push_back("1"); } }; + +// FIXME: Detect invalidation of fields. +// https://github.com/llvm/llvm-project/issues/180992 +struct InvalidateMemberFields { + InvalidateMemberFields(); + void invalidateField() { + auto it = container.begin(); + container.push_back("1"); + *it; + } + void invalidateFieldRef() { + auto it = contiainerRef.begin(); + contiainerRef.push_back("1"); + *it; + } +private: + std::vector<std::string> container; + std::vector<std::string>& contiainerRef; +}; } // namespace InvalidatedField namespace InvalidatedGlobal { @@ -742,6 +789,13 @@ void MapSubscriptDoesNotInvalidate() { *it; } +void MapOperatorBracket() { + std::unordered_map<int, int> m; + auto it = m.begin(); + m[1]; + *it; +} + void PrintMax(const int& a, const int& b); void MapSubscriptMultipleCallsDoesNotInvalidate(std::map<int, int> mp, int a, int b) { @@ -749,14 +803,7 @@ void MapSubscriptMultipleCallsDoesNotInvalidate(std::map<int, int> mp, int a, in } void FlatMapSubscriptMultipleCallsInvalidate(std::flat_map<int, int> mp, int a, int b) { - // FIXME: The duplicate warning below is a false positive caused by self-invalidation of the Owner 'mp'. - // While the warning on the temporary reference returned by mp[a] is a true positive (it dangles), - // the second warning on 'mp' itself is redundant and incorrect. - // Resolving this requires distinguishing owner-borrow from content-borrow. PrintMax(mp[a], mp[b]); // expected-warning {{parameter 'mp' is later invalidated}} \ - // expected-warning {{parameter 'mp' is later invalidated}} \ - // expected-note {{parameter 'mp' is invalidated here}} \ - // expected-note {{later used here}} \ // expected-note {{parameter 'mp' is invalidated here}} \ // expected-note {{later used here}} } @@ -896,7 +943,7 @@ struct StringOwner { void member_destructor_invalidates_pointer() { StringOwner owner = {"42", "43"}; const char *p = owner.s.data(); - owner.t.~basic_string(); // OK + owner.t.~basic_string(); (void)*p; } @@ -936,6 +983,38 @@ void invalid_after_ternary_reset(bool flag) { } // namespace unique_ptr_invalidation + + +namespace NestedContainers { +// FIXME: Maybe come up with a access path representation to detect this. +void InnerIteratorInvalidated() { + std::vector<std::vector<int>> v; + v.resize(1); + // FIXME: Detect this. + // We cannot differentiate between v.* and v.*.*. + // An annotation system along with lifetimebound could be helpful to describe the paths returned. + auto it = v[0].begin(); + v[0].push_back(1); + *it; +} +void InnerIteratorInvalidatedOneUseOtherIsStillBad() { + // FIXME: Detect this. + std::vector<std::vector<int>> v; + v.resize(100); + auto it = v[0].begin(); + v[1].push_back(1); + *it; +} + +void OuterIteratorNotInvalidated() { + std::vector<std::vector<int>> v; + v.resize(1); + auto it = v.begin(); + v[0].push_back(1); + it->clear(); // OK +} +} // namespace NestedContainers + namespace DeepFieldNesting { struct Level3 { std::vector<std::string> vec; @@ -966,6 +1045,14 @@ void SiblingLevel2Ok() { *it; } +// Modifying same vector: Invalid +void SameVectorInvalid() { + Level1 obj; + auto it = obj.inner2_1.inner3_1.vec.begin(); // expected-warning {{local variable 'obj' is later invalidated}} + obj.inner2_1.inner3_1.vec.push_back("1"); // expected-note {{local variable 'obj' is invalidated here}} + *it; // expected-note {{later used here}} +} + // Modifying sibling non-container field at Level 3: OK void SiblingFieldLevel3Ok() { Level1 obj; @@ -974,7 +1061,31 @@ void SiblingFieldLevel3Ok() { *it; } -// Modifying parent structure after use: OK +// Modifying parent at Level 2 (reassigning struct): Invalid +// FIXME: We don't currently detect invalidation of member containers when the parent struct is reassigned. +void ParentLevel2Invalid() { + Level1 obj; + auto it = obj.inner2_1.inner3_1.vec.begin(); + Level3 new_val; + obj.inner2_1.inner3_1 = new_val; + *it; +} + +// Deep nesting with pointers: Invalid +void PointerNestedInvalid(Level1* ptr) { // expected-warning {{parameter 'ptr' is later invalidated}} + auto it = ptr->inner2_1.inner3_1.vec.begin(); + ptr->inner2_1.inner3_1.vec.push_back("1"); // expected-note {{parameter 'ptr' is invalidated here}} + *it; // expected-note {{later used here}} +} + +// 7. Deep nesting with references: Invalid +void ReferenceNestedInvalid(Level1& ref) { // expected-warning {{parameter 'ref' is later invalidated}} + auto it = ref.inner2_1.inner3_1.vec.begin(); + ref.inner2_1.inner3_1.vec.push_back("1"); // expected-note {{parameter 'ref' is invalidated here}} + *it; // expected-note {{later used here}} +} + +// 8. Modifying parent structure after use: OK void ParentModifiedAfterUseOk() { Level1 obj; auto it = obj.inner2_1.inner3_1.vec.begin(); @@ -983,14 +1094,14 @@ void ParentModifiedAfterUseOk() { obj.inner2_1.inner3_1 = new_val; // OK, because ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/207523 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
