Author: Zeyi Xu Date: 2026-05-25T23:14:15+08:00 New Revision: 3e6582fba63f1b2cade4eb3222736d50e1121b7b
URL: https://github.com/llvm/llvm-project/commit/3e6582fba63f1b2cade4eb3222736d50e1121b7b DIFF: https://github.com/llvm/llvm-project/commit/3e6582fba63f1b2cade4eb3222736d50e1121b7b.diff LOG: Reland "[LifetimeSafety] Detect iterator invalidation through container aliases" (#197873) This relands #195231, which was reverted in commit 7c9717848851f3a71908becab4312ddc2d8482b8. The original crash from the reproducer no longer reproduces after #196680, #197220, and #197604. I verified the original `repro.cpp`: it no longer hits the lifetime-safety assertion now. Also added regression tests for the crash: ```cpp struct SinkInteriorBorrow { const char *dest_; // expected-note {{this field dangles}} SinkInteriorBorrow(std::string *dest, int n) : dest_(dest->data()) { // expected-warning {{parameter which escapes to a field is later invalidated}} if (n > 0) dest->clear(); // expected-note {{invalidated here}} } }; ``` Closes https://github.com/llvm/llvm-project/issues/193044 Added: Modified: clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp clang/test/Sema/warn-lifetime-safety-invalidations.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 8a7a4b79a1584..582a8fd1645a8 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -812,9 +812,11 @@ void FactsGenerator::handleInvalidatingCall(const Expr *Call, if (!isInvalidationMethod(*MD)) return; - // Heuristics to turn-down false positives. - auto *DRE = dyn_cast<DeclRefExpr>(Args[0]); - if (!DRE || DRE->getDecl()->getType()->isReferenceType()) + + // Heuristics to turn-down false positives. Skip member field expressions for + // now. This is not a perfect filter and will still surface some false + // positives (e.g. `auto& r = s.v`). + if (!isa<DeclRefExpr>(Args[0]->IgnoreImpCasts())) return; OriginList *ThisList = getOriginsList(*Args[0]); diff --git a/clang/test/Sema/warn-lifetime-safety-invalidations.cpp b/clang/test/Sema/warn-lifetime-safety-invalidations.cpp index ee4b76b14c0f4..35077b547fc95 100644 --- a/clang/test/Sema/warn-lifetime-safety-invalidations.cpp +++ b/clang/test/Sema/warn-lifetime-safety-invalidations.cpp @@ -236,13 +236,12 @@ void IteratorUsedAfterErase(std::vector<int> v) { } } -// FIXME: Detect this. We currently skip invalidation through ref/pointers to containers. -void IteratorUsedAfterPushBackParam(std::vector<int>& v) { +void IteratorUsedAfterPushBackParam(std::vector<int>& v) { // expected-warning {{parameter is later invalidated}} auto it = std::begin(v); if (it != std::end(v) && *it == 3) { - v.push_back(4); + v.push_back(4); // expected-note {{invalidated here}} } - ++it; + ++it; // expected-note {{later used here}} } void IteratorUsedAfterPushBack(std::vector<int> v) { @@ -321,6 +320,55 @@ void IteratorUsedAfterStdBeginAddAssign() { } } // namespace SimpleInvalidIterators +namespace InvalidatingThroughContainerAliases { +void IteratorInvalidatedThroughLocalReferenceAlias() { + std::vector<int> vv; + std::vector<int> &v = vv; + auto it = vv.begin(); // expected-warning {{object whose reference is captured is later invalidated}} + v.push_back(42); // expected-note {{invalidated here}} + (void)it; // expected-note {{later used here}} +} + +void IteratorInvalidatedThroughPointerParameter(std::vector<int> *v) { // expected-warning {{parameter is later invalidated}} + auto it = v->begin(); + v->push_back(42); // expected-note {{invalidated here}} + (void)it; // expected-note {{later used here}} +} + +void ParenthesizedContainerInvalidatesIterator() { + // FIXME: Support invalidation through non-DRE lvalue expressions. + std::vector<int> v; + auto it = v.begin(); + (v).push_back(42); + (void)it; +} + +} // namespace InvalidatingThroughContainerAliases + +namespace ContainerObjectAliases { +// FIXME: Distinguish owner-borrow from content-borrow. +void PointerParameterObjectUseIsOk(std::vector<int> *v) { // expected-warning {{parameter is later invalidated}} + v->push_back(42); // expected-note {{invalidated here}} + (void)v; // expected-note {{later used here}} +} + +// FIXME: Distinguish owner-borrow from content-borrow. +void LocalPointerAliasObjectUseIsOk() { + std::vector<int> vv; + std::vector<int> *v = &vv; // expected-warning {{object whose reference is captured is later invalidated}} + v->push_back(42); // expected-note {{invalidated here}} + (void)*v; // expected-note {{later used here}} +} + +// FIXME: Distinguish owner-borrow from content-borrow. +void LocalReferenceAliasObjectUseIsOk() { + std::vector<int> vv; + std::vector<int> &v = vv; // expected-warning {{object whose reference is captured is later invalidated}} + v.push_back(42); // expected-note {{invalidated here}} + (void)v; // expected-note {{later used here}} +} +} // namespace ContainerObjectAliases + namespace ElementReferences { // Testing raw pointers and references to elements, not just iterators. @@ -356,7 +404,7 @@ void SelfInvalidatingMap() { // insertion and following is unsafe for this container. mp[1] = "42"; mp[2] // expected-note {{invalidated here}} - = + = mp[1]; // expected-warning {{object whose reference is captured is later invalidated}} expected-note {{later used here}} } @@ -364,7 +412,7 @@ void InvalidateErase() { std::flat_map<int, std::string> mp; // 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}} + mp.erase(mp.find(4)); // expected-note {{invalidated here}} if (it != mp.end()) // expected-note {{later used here}} *it; } @@ -407,42 +455,60 @@ void Invalidate1Use1IsInvalid() { s.strings1.push_back("1"); *it; } -void Invalidate1Use2IsOk() { +void Invalidate2Use1IsOk() { S s; auto it = s.strings1.begin(); s.strings2.push_back("1"); *it; -}void Invalidate1Use2ViaRefIsOk() { +} +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; +} +void ConditionalFieldInvalidatesIterator(bool flag) { + // FIXME: Support conditional invalidation through field expressions. S s; - auto it = s.strings2.begin(); - auto& strings2 = s.strings2; - strings2.push_back("1"); + auto it = s.strings1.begin(); + (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 {{object whose reference is captured is later invalidated}} + auto& strings1 = s.strings1; + strings1.push_back("1"); // expected-note {{invalidated here}} + *it; // expected-note {{later used here}} +} void Invalidate1UseSIsOk() { S s; S* p = &s; 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; - p->push_back("1"); - (void)*p; + std::vector<std::string>* p = &s; // expected-warning {{object whose reference is captured is later invalidated}} + p->push_back("1"); // expected-note {{invalidated here}} + (void)*p; // expected-note {{later used here}} } void IteratorFromPointerToContainerIsInvalidated() { - // FIXME: Detect this. std::vector<std::string> s; - std::vector<std::string>* p = &s; + std::vector<std::string>* p = &s; // expected-warning {{object whose reference is captured is later invalidated}} auto it = p->begin(); - p->push_back("1"); - *it; + p->push_back("1"); // expected-note {{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) - path = std::string(); + for (std::string& path : subdirs) // expected-warning {{object whose reference is captured is later invalidated}} expected-note {{later used here}} + path = std::string(); // expected-note {{invalidated here}} } } // namespace ContainersAsFields @@ -450,6 +516,25 @@ void ChangingRegionOwnedByContainerIsOk() { namespace InvalidatedField { std::string StableString; +// FIXME: Distinguish owner-borrow from interior-borrow. +struct SinkOwnerBorrow { + std::string *dest_; // expected-note {{this field dangles}} + + SinkOwnerBorrow(std::string *dest, int n) : dest_(dest) { // expected-warning {{parameter which escapes to a field is later invalidated}} + if (n > 0) + dest->clear(); // expected-note {{invalidated here}} + } +}; + +struct SinkInteriorBorrow { + const char *dest_; // expected-note {{this field dangles}} + + SinkInteriorBorrow(std::string *dest, int n) : dest_(dest->data()) { // expected-warning {{parameter which escapes to a field is later invalidated}} + if (n > 0) + dest->clear(); // expected-note {{invalidated here}} + } +}; + struct S { std::string_view FieldFromLocalVector; // expected-note {{this field dangles}} std::string_view FieldFromByValueParamVector; // expected-note {{this field dangles}} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
