llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-temporal-safety @llvm/pr-subscribers-clang Author: Zeyi Xu (zeyi2) <details> <summary>Changes</summary> Teach lifetime safety invalidation diagnostics to handle origins that escape through fields or global/static storage before the referenced object is invalidated. Previously they were skipped. Closes https://github.com/llvm/llvm-project/issues/195706 Closes https://github.com/llvm/llvm-project/issues/193044 --- Full diff: https://github.com/llvm/llvm-project/pull/196680.diff 6 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h (+6) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+8) - (modified) clang/lib/Analysis/LifetimeSafety/Checker.cpp (+19-1) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+4-3) - (modified) clang/lib/Sema/SemaLifetimeSafety.h (+35) - (modified) clang/test/Sema/warn-lifetime-safety-invalidations.cpp (+204-21) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h index d20ac87a7c8d9..2332ce36646ed 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h @@ -88,6 +88,12 @@ class LifetimeSafetySemaHelper { virtual void reportUseAfterInvalidation(const ParmVarDecl *PVD, const Expr *UseExpr, const Expr *InvalidationExpr) {} + virtual void reportInvalidatedField(const Expr *IssueExpr, + const FieldDecl *Field, + const Expr *InvalidationExpr) {} + virtual void reportInvalidatedGlobal(const Expr *IssueExpr, + const VarDecl *DanglingGlobal, + const Expr *InvalidationExpr) {} using EscapingTarget = llvm::PointerUnion<const Expr *, const FieldDecl *, const VarDecl *>; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index c69b2ce3648f8..07846cd761ee3 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -10985,6 +10985,14 @@ def warn_lifetime_safety_invalidation : Warning<"%select{object whose reference is captured|parameter}0 is later invalidated">, InGroup<LifetimeSafetyInvalidation>, DefaultIgnore; +def warn_lifetime_safety_invalidated_field + : Warning<"object whose reference is stored in a field is later invalidated">, + InGroup<LifetimeSafetyInvalidation>, + DefaultIgnore; +def warn_lifetime_safety_invalidated_global + : Warning<"object whose reference is stored in global or static storage is later invalidated">, + InGroup<LifetimeSafetyInvalidation>, + DefaultIgnore; def warn_lifetime_safety_dangling_field : Warning<"address of stack memory escapes to a field">, diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp index 4ae90cf751ec3..fb292caf5c5de 100644 --- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp @@ -259,7 +259,25 @@ class LifetimeChecker { MovedExpr, ExpiryLoc); } else if (const auto *OEF = CausingFact.dyn_cast<const OriginEscapesFact *>()) { - if (const auto *RetEscape = dyn_cast<ReturnEscapeFact>(OEF)) + if (Warning.InvalidatedByExpr) { + if (const auto *FieldEscape = dyn_cast<FieldEscapeFact>(OEF)) + // Field escape later invalidated. + SemaHelper->reportInvalidatedField(IssueExpr, + FieldEscape->getFieldDecl(), + Warning.InvalidatedByExpr); + else if (const auto *GlobalEscape = dyn_cast<GlobalEscapeFact>(OEF)) + // Global escape later invalidated. + SemaHelper->reportInvalidatedGlobal(IssueExpr, + GlobalEscape->getGlobal(), + Warning.InvalidatedByExpr); + else if (isa<ReturnEscapeFact>(OEF)) + // Return escape. + SemaHelper->reportUseAfterReturn( + IssueExpr, cast<ReturnEscapeFact>(OEF)->getReturnExpr(), + MovedExpr, ExpiryLoc); + else + llvm_unreachable("Unhandled OriginEscapesFact type"); + } else if (const auto *RetEscape = dyn_cast<ReturnEscapeFact>(OEF)) // Return stack address. SemaHelper->reportUseAfterReturn( IssueExpr, RetEscape->getReturnExpr(), MovedExpr, ExpiryLoc); diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 0a06548d881d1..0e6b032914b05 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -810,9 +810,10 @@ 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]->IgnoreParenImpCasts())) return; OriginList *ThisList = getOriginsList(*Args[0]); diff --git a/clang/lib/Sema/SemaLifetimeSafety.h b/clang/lib/Sema/SemaLifetimeSafety.h index 92e7b5cf14ae5..5925550991701 100644 --- a/clang/lib/Sema/SemaLifetimeSafety.h +++ b/clang/lib/Sema/SemaLifetimeSafety.h @@ -142,6 +142,41 @@ class LifetimeSafetySemaHelperImpl : public LifetimeSafetySemaHelper { << UseExpr->getSourceRange(); } + void reportInvalidatedField(const Expr *IssueExpr, + const FieldDecl *DanglingField, + const Expr *InvalidationExpr) override { + const Expr *WarningExpr = IssueExpr ? IssueExpr : InvalidationExpr; + S.Diag(WarningExpr->getExprLoc(), + diag::warn_lifetime_safety_invalidated_field) + << WarningExpr->getSourceRange(); + S.Diag(InvalidationExpr->getExprLoc(), + diag::note_lifetime_safety_invalidated_here) + << InvalidationExpr->getSourceRange(); + S.Diag(DanglingField->getLocation(), + diag::note_lifetime_safety_dangling_field_here) + << DanglingField->getEndLoc(); + } + + void reportInvalidatedGlobal(const Expr *IssueExpr, + const VarDecl *DanglingGlobal, + const Expr *InvalidationExpr) override { + const Expr *WarningExpr = IssueExpr ? IssueExpr : InvalidationExpr; + S.Diag(WarningExpr->getExprLoc(), + diag::warn_lifetime_safety_invalidated_global) + << WarningExpr->getSourceRange(); + S.Diag(InvalidationExpr->getExprLoc(), + diag::note_lifetime_safety_invalidated_here) + << InvalidationExpr->getSourceRange(); + if (DanglingGlobal->isStaticLocal() || DanglingGlobal->isStaticDataMember()) + S.Diag(DanglingGlobal->getLocation(), + diag::note_lifetime_safety_dangling_static_here) + << DanglingGlobal->getEndLoc(); + else + S.Diag(DanglingGlobal->getLocation(), + diag::note_lifetime_safety_dangling_global_here) + << DanglingGlobal->getEndLoc(); + } + void suggestLifetimeboundToParmVar(SuggestionScope Scope, const ParmVarDecl *ParmToAnnotate, EscapingTarget Target) override { diff --git a/clang/test/Sema/warn-lifetime-safety-invalidations.cpp b/clang/test/Sema/warn-lifetime-safety-invalidations.cpp index df9f7288144b1..bb4560c001181 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,46 @@ 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}} +} +} // 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 +395,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 +403,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,17 +446,20 @@ 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() { +} + +// FIXME: Requires field-sensitive AccessPaths to fix. +void Invalidate1Use2ViaRefIsOk() { S s; - auto it = s.strings2.begin(); - auto& strings2 = s.strings2; - strings2.push_back("1"); - *it; + 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; @@ -425,28 +467,169 @@ 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; - 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 +namespace InvalidatedGlobalAndField { +std::string StableString; +std::string_view GlobalView; // expected-note {{this global dangles}} +std::string_view GlobalViewFromLocal; // expected-note {{this global dangles}} +std::string_view GlobalViewFromReferenceAlias; // expected-note {{this global dangles}} +std::string_view GlobalViewFromUnmodifiedField; // expected-note {{this global dangles}} +std::string_view GlobalViewReassigned; +std::string_view GlobalViewUnrelatedContainer; +std::string *GlobalDest; // expected-note {{this global dangles}} + +struct TwoVectors { + std::vector<std::string> Strings1; + std::vector<std::string> Strings2; +}; + +struct StringOwner { + std::string S; + std::string T; +}; + +void InvalidatedGlobalPointerParam(std::vector<std::string> *strings) { + GlobalView = *strings->begin(); + strings->push_back("1"); // expected-warning {{object whose reference is stored in global or static storage is later invalidated}} + // expected-note@-1 {{invalidated here}} +} + +void InvalidatedGlobalLocalContainer() { + std::vector<std::string> strings; + GlobalViewFromLocal = *strings.begin(); // expected-warning {{object whose reference is stored in global or static storage is later invalidated}} + strings.push_back("1"); // expected-note {{invalidated here}} +} + +void InvalidatedGlobalReferenceAlias(std::vector<std::string> &strings) { + std::vector<std::string> &alias = strings; + GlobalViewFromReferenceAlias = *strings.begin(); + alias.push_back("1"); // expected-warning {{object whose reference is stored in global or static storage is later invalidated}} + // expected-note@-1 {{invalidated here}} +} + +void InvalidatedStaticLocal(std::vector<std::string> *strings) { + static std::string_view StaticView; // expected-note {{this static storage dangles}} + StaticView = *strings->begin(); + strings->push_back("1"); // expected-warning {{object whose reference is stored in global or static storage is later invalidated}} + // expected-note@-1 {{invalidated here}} +} + +void GlobalReassignedBeforeInvalidation(std::vector<std::string> *strings) { + GlobalViewReassigned = *strings->begin(); + GlobalViewReassigned = StableString; + strings->push_back("1"); +} + +void GlobalUnrelatedContainerInvalidated(std::vector<std::string> *strings1, + std::vector<std::string> *strings2) { + GlobalViewUnrelatedContainer = *strings1->begin(); + strings2->push_back("1"); +} + +// FIXME: Requires field-sensitive AccessPaths to fix. +void GlobalDifferentFieldInvalidatedIsOk(TwoVectors &s) { + GlobalViewFromUnmodifiedField = *s.Strings2.begin(); + auto &strings1 = s.Strings1; + strings1.push_back("1"); // expected-warning {{object whose reference is stored in global or static storage is later invalidated}} + // expected-note@-1 {{invalidated here}} +} + +// FIXME: Distinguish owner-borrow from content-borrow. +void GlobalPointerOwnerBorrowIsOk(std::string *dest) { + GlobalDest = dest; + dest->clear(); // expected-warning {{object whose reference is stored in global or static storage is later invalidated}} + // expected-note@-1 {{invalidated here}} +} + +struct S { + std::string_view Field; // expected-note {{this field dangles}} + std::string_view FieldFromLocal; // expected-note {{this field dangles}} + std::string_view FieldFromReferenceAlias; // expected-note {{this field dangles}} + std::string_view FieldReassigned; + std::string_view FieldUnrelatedContainer; + const char *FieldCharPtr; // expected-note {{this field dangles}} + static std::string_view StaticField; // expected-note {{this static storage dangles}} + + void InvalidatedFieldPointerParam(std::vector<std::string> *strings) { + Field = *strings->begin(); + strings->push_back("1"); // expected-warning {{object whose reference is stored in a field is later invalidated}} + // expected-note@-1 {{invalidated here}} + } + + void InvalidatedFieldLocalContainer() { + std::vector<std::string> strings; + FieldFromLocal = *strings.begin(); // expected-warning {{object whose reference is stored in a field is later invalidated}} + strings.push_back("1"); // expected-note {{invalidated here}} + } + + void InvalidatedFieldReferenceAlias(std::vector<std::string> &strings) { + std::vector<std::string> &alias = strings; + FieldFromReferenceAlias = *strings.begin(); + alias.push_back("1"); // expected-warning {{object whose reference is stored in a field is later invalidated}} + // expected-note@-1 {{invalidated here}} + } + + void FieldReassignedBeforeInvalidation(std::vector<std::string> *strings) { + FieldReassigned = *strings->begin(); + FieldReassigned = StableString; + strings->push_back("1"); + } + + void FieldUnrelatedContainerInvalidated(std::vector<std::string> *strings1, + std::vector<std::string> *strings2) { + FieldUnrelatedContainer = *strings1->begin(); + strings2->push_back("1"); + } + + // FIXME: Requires field-sensitive AccessPaths to fix. + void MemberDestructorDifferentFieldIsOk(StringOwner &owner) { + FieldCharPtr = owner.S.data(); + owner.T.~basic_string(); // expected-warning {{object whose reference is stored in a field is later invalidated}} + // expected-note@-1 {{invalidated here}} + } +}; + +void InvalidatedStaticDataMember(std::vector<std::string> *strings) { + S::StaticField = *strings->begin(); + strings->push_back("1"); // expected-warning {{object whose reference is stored in global or static storage is later invalidated}} + // expected-note@-1 {{invalidated here}} +} + +struct Sink { + std::string *Dest; // expected-note {{this field dangles}} + + // FIXME: Distinguish owner-borrow from content-borrow. + Sink(std::string *dest, int n) : Dest(dest) { + if (n > 0) + dest->clear(); // expected-warning {{object whose reference is stored in a field is later invalidated}} + // expected-note@-1 {{invalidated here}} + } +}; +} // namespace InvalidatedGlobalAndField + namespace AssociativeContainers { void SetInsertDoesNotInvalidate() { std::set<int> s; `````````` </details> https://github.com/llvm/llvm-project/pull/196680 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
