llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-temporal-safety @llvm/pr-subscribers-clang-analysis Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> Add missing `UseFact` for binary operators and function call arguments to track object usage. These changes allow the analyzer to properly detect cases where an object is used after being invalidated, particularly in container operations like map access. **Pointer vs Iterator Invalidation:** Different containers provide different stability guarantees: - **Pointer/Reference Stability**: Containers like `std::unordered_map` guarantee that pointers and references to elements remain valid even after insertions. This makes operations like `mp[2] = mp[1]` safe in practice. - **No Pointer Stability**: Containers like `std::flat_hash_map` (C++23) do not provide pointer stability on insertion, making such operations unsafe. - **Iterator Stability**: Most containers (including `std::unordered_map`) do not provide iterator stability. Operations that invalidate iterators (like `erase`) make subsequent use of those iterators unsafe. The current implementation conservatively warns on all such cases. --- Full diff: https://github.com/llvm/llvm-project/pull/180446.diff 3 Files Affected: - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+6) - (modified) clang/test/Sema/Inputs/lifetime-analysis.h (+11-5) - (modified) clang/test/Sema/warn-lifetime-safety-invalidations.cpp (+16-3) ``````````diff diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index b69f69ddbae34..40661289b2f2b 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -366,6 +366,9 @@ void FactsGenerator::VisitBinaryOperator(const BinaryOperator *BO) { // result should have the same loans as the pointer operand. if (BO->isCompoundAssignmentOp()) return; + if (auto *RHSOrigins = getOriginsList(*BO->getRHS())) + CurrentBlockFacts.push_back( + FactMgr.createFact<UseFact>(BO->getRHS(), RHSOrigins)); if (BO->isAssignmentOp()) handleAssignment(BO->getLHS(), BO->getRHS()); // TODO: Handle assignments involving dereference like `*p = q`. @@ -582,6 +585,9 @@ void FactsGenerator::handleFunctionCall(const Expr *Call, FD = getDeclWithMergedLifetimeBoundAttrs(FD); if (!FD) return; + for (const Expr *Arg : Args) + if (OriginList *ArgList = getOriginsList(*Arg)) + CurrentBlockFacts.push_back(FactMgr.createFact<UseFact>(Arg, ArgList)); handleInvalidatingCall(Call, FD, Args); handleMovedArgsInCall(FD, Args); diff --git a/clang/test/Sema/Inputs/lifetime-analysis.h b/clang/test/Sema/Inputs/lifetime-analysis.h index f30db1a29b149..880e4650f21d0 100644 --- a/clang/test/Sema/Inputs/lifetime-analysis.h +++ b/clang/test/Sema/Inputs/lifetime-analysis.h @@ -75,11 +75,6 @@ struct vector { void clear(); }; -template<class Key,class T> -struct unordered_map { - T& operator[](const Key& key); -}; - template<class T> void swap( T& a, T& b ); @@ -89,6 +84,17 @@ struct pair { B second; }; +template<class Key,class T> +struct unordered_map { + using iterator = __gnu_cxx::basic_iterator<std::pair<const Key, T>>; + T& operator[](const Key& key); + iterator begin(); + iterator end(); + iterator find(const Key& key); + void erase(iterator); +}; + + template<typename T> struct basic_string_view { basic_string_view(); diff --git a/clang/test/Sema/warn-lifetime-safety-invalidations.cpp b/clang/test/Sema/warn-lifetime-safety-invalidations.cpp index c9ce0c35c53d2..414541eb031c5 100644 --- a/clang/test/Sema/warn-lifetime-safety-invalidations.cpp +++ b/clang/test/Sema/warn-lifetime-safety-invalidations.cpp @@ -260,9 +260,22 @@ void PointerToVectorElement() { } void SelfInvalidatingMap() { - std::unordered_map<int, int> mp; - mp[1] = 1; - mp[2] = mp[1]; // FIXME: Detect this. We are mising a UseFact for the assignment params. + std::unordered_map<int, std::string> mp; + // TODO: We do not have a way to differentiate between pointer stability and iterator stability! + // std::unordered_map and other containers provide pointer/reference stability. Therefore the + // following is safe in practice. + // On the other hand, std::flat_hash_map (since C++23) does not provide pointer stability on + // insertion and following is unsafe for this container. + mp[1] = "42"; + mp[2] = mp[1]; // expected-warning {{object whose reference is captured is later invalidated}} \ + // expected-note {{invalidated here}} \ + // expected-note {{later used here}} + + // None of these containers provide iterator stability. So following is unsafe: + auto it = mp.find(3); // expected-warning {{object whose reference is captured is later invalidated}} + mp.erase(mp.find(4)); // expected-note {{invalidated here}} + if (it != mp.end()) // expected-note {{later used here}} + *it; } } // namespace ElementReferences `````````` </details> https://github.com/llvm/llvm-project/pull/180446 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
