llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Balázs Benics (steakhal) <details> <summary>Changes</summary> The field-origin model only recognized a pointer stored as a direct member of `*this` (`this->field`). A pointer nested inside a member subobject was never modeled -- most importantly libc++'s `unique_ptr`, whose pointer lives in a `_LIBCPP_COMPRESSED_PAIR` that expands (on clang) to an anonymous struct. Its loan was lost when the field was read, so e.g. `unique_ptr::reset()` could not see that the deleter consumes the owned pointer. Generalize both halves of the model to `this`-rooted subobject access paths, traversing only *anonymous* struct/union subobjects. Their members are uniquely named within the class, so a FieldDecl-keyed origin cannot conflate two distinct subobjects: - OriginManager::getOrCreateList(Expr) shares origins with the terminal FieldDecl of a `this`-rooted chain of anonymous subobject accesses. Arrow accesses (`this->p->f`) are rejected, since `f` belongs to another object. - FactsGenerator::issueFieldPlaceholderLoans recurses into anonymous subobject fields, issuing a `$this.<field>` placeholder loan for each pointer leaf (e.g. unique_ptr's `__ptr_` inside the compressed-pair anonymous struct). Escape to a field of an anonymous subobject is now detected (dangling-field.cpp). Escape through a *named* subobject remains a documented FIXME: a FieldDecl-keyed origin cannot distinguish two members of the same type. Assisted-By: claude --- Full diff: https://github.com/llvm/llvm-project/pull/232867.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h (+1-1) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+27-10) - (modified) clang/lib/Analysis/LifetimeSafety/Origins.cpp (+25-1) - (modified) clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp (+12) - (modified) clang/test/Sema/LifetimeSafety/dangling-field.cpp (+39-6) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h index f5f84f27b5035..009f050710c35 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h @@ -165,7 +165,7 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> { bool escapesViaReturn(OriginID OID) const; llvm::SmallVector<Fact *> issuePlaceholderLoans(); - void issueFieldPlaceholderLoans(const CXXMethodDecl *MD, + void issueFieldPlaceholderLoans(const CXXRecordDecl *ClassDef, llvm::SmallVectorImpl<Fact *> &Facts); FactManager &FactMgr; AnalysisDeclContext &AC; diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 6a805bb7be2d8..ea3582185063f 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -1211,15 +1211,30 @@ void FactsGenerator::handleUse(const Expr *E) { // class and its bases. It represents the storage the field refers to, as // opposed to the field itself (covered by $this). void FactsGenerator::issueFieldPlaceholderLoans( - const CXXMethodDecl *MD, llvm::SmallVectorImpl<Fact *> &Facts) { - const CXXRecordDecl *ClassDecl = MD->getParent()->getDefinition(); - if (!ClassDecl) - return; + const CXXRecordDecl *ClassDef, llvm::SmallVectorImpl<Fact *> &Facts) { + assert(ClassDef); + assert(ClassDef->isThisDeclarationADefinition()); auto IssueForRecord = [&](const CXXRecordDecl *RD) { for (const FieldDecl *Field : RD->fields()) { - // Skip private fields of bases as they cannot be named in MD. - if (RD != ClassDecl && Field->getAccess() == AS_private) + // Skip private fields of bases as they cannot be named in ClassDef. + if (RD != ClassDef && Field->getAccess() == AS_private) continue; + + if (Field->isAnonymousStructOrUnion()) { + if (const CXXRecordDecl *Sub = Field->getType()->getAsCXXRecordDecl()) { + if (const CXXRecordDecl *SubDef = Sub->getDefinition()) { + // Anonymous records cannot have base classes and they can only have + // public fields. Sema rejects them with an error otherwise. + [[maybe_unused]] auto IsFieldPublic = [](const FieldDecl *F) { + return F->getAccess() == AS_public; + }; + assert(SubDef->getNumBases() == 0); + assert(llvm::all_of(SubDef->fields(), IsFieldPublic)); + issueFieldPlaceholderLoans(SubDef, Facts); + continue; + } + } + } OriginList *FieldList = getOriginsList(*Field); if (!FieldList) continue; @@ -1229,8 +1244,8 @@ void FactsGenerator::issueFieldPlaceholderLoans( } return true; }; - IssueForRecord(ClassDecl); - ClassDecl->forallBases(IssueForRecord); + IssueForRecord(ClassDef); + ClassDef->forallBases(IssueForRecord); } // Creates an IssueFact for a new placeholder loan for each pointer or reference @@ -1248,8 +1263,10 @@ llvm::SmallVector<Fact *> FactsGenerator::issuePlaceholderLoans() { const Loan *L = FactMgr.getLoanMgr().createPlaceholderLoan(MD); PlaceholderLoanFacts.push_back( FactMgr.createFact<IssueFact>(L->getID(), List->getOuterOriginID())); - if (LSOpts.CheckFieldOriginEscape && !isa<CXXConstructorDecl>(MD)) - issueFieldPlaceholderLoans(MD, PlaceholderLoanFacts); + if (LSOpts.CheckFieldOriginEscape && !isa<CXXConstructorDecl>(MD)) { + if (const auto *ClassDef = MD->getParent()->getDefinition()) + issueFieldPlaceholderLoans(ClassDef, PlaceholderLoanFacts); + } } for (const ParmVarDecl *PVD : FD->parameters()) { OriginList *List = getOriginsList(*PVD); diff --git a/clang/lib/Analysis/LifetimeSafety/Origins.cpp b/clang/lib/Analysis/LifetimeSafety/Origins.cpp index 385b203d72845..4aaf2e2d0b730 100644 --- a/clang/lib/Analysis/LifetimeSafety/Origins.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Origins.cpp @@ -256,6 +256,30 @@ OriginList *OriginManager::getOrCreateList(const ValueDecl *D) { return DeclToList[D] = buildListForType(D->getType(), D); } +/// Returns true if the field named by \p ME denotes a subobject of `*this`, +/// reached as `this->f` or through anonymous-struct/union subobjects. +static bool isSubobjectOfThis(const MemberExpr *ME) { + const Expr *Base = ME->getBase()->IgnoreParenImpCasts(); + if (isa<CXXThisExpr>(Base)) + return true; + + // An arrow access dereferences a pointer, so the field belongs to another + // object, not a subobject of `*this`. + if (ME->isArrow()) + return false; + + // Traverse only *anonymous* subobjects: their members are uniquely named + // within the class, so the FieldDecl-keyed origin can't conflate two + // subobjects. A named subobject (`this->a.f`) could repeat the same inner + // type, so it is rejected. + if (const auto *BaseAsME = dyn_cast<MemberExpr>(Base)) { + const auto *BaseField = dyn_cast<FieldDecl>(BaseAsME->getMemberDecl()); + if (BaseField && BaseField->isAnonymousStructOrUnion()) + return isSubobjectOfThis(BaseAsME); + } + return false; +} + OriginList *OriginManager::getOrCreateList(const Expr *E) { if (auto *ParenIgnored = E->IgnoreParens(); ParenIgnored != E) return getOrCreateList(ParenIgnored); @@ -290,7 +314,7 @@ OriginList *OriginManager::getOrCreateList(const Expr *E) { ReferencedDecl = DRE->getDecl(); else if (auto *ME = dyn_cast<MemberExpr>(E)) if (auto *Field = dyn_cast<FieldDecl>(ME->getMemberDecl()); - Field && isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts())) + Field && isSubobjectOfThis(ME)) ReferencedDecl = Field; if (ReferencedDecl) { OriginList *Head = nullptr; diff --git a/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp b/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp index 34f3ad4f45de1..12ed6452ba845 100644 --- a/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp +++ b/clang/test/Sema/LifetimeSafety/annotation-suggestions.cpp @@ -818,6 +818,18 @@ struct FieldTypes { } }; +namespace FieldOriginEscapeThroughAnonStruct { +struct FieldInAnonStruct { + struct { + int *ptr_; // expected-note {{if field 'ptr_' is owned by the class, mark the method [[clang::lifetimebound]]}} + }; + + int *get_ptr() const { // expected-warning {{return value depends on the object referenced by field 'ptr_', not on '*this'}} + return ptr_; // expected-note {{returned here}} + } +}; +} // namespace FieldOriginEscapeThroughAnonStruct + struct Base { int *pub_ptr_; // expected-note {{if field 'pub_ptr_' is owned by the class, mark the method [[clang::lifetimebound]]}} int *get_priv() const { // expected-warning {{return value depends on the object referenced by field 'priv_ptr_', not on '*this'}} diff --git a/clang/test/Sema/LifetimeSafety/dangling-field.cpp b/clang/test/Sema/LifetimeSafety/dangling-field.cpp index 68ed47687b29a..c2611a99384c8 100644 --- a/clang/test/Sema/LifetimeSafety/dangling-field.cpp +++ b/clang/test/Sema/LifetimeSafety/dangling-field.cpp @@ -196,27 +196,60 @@ struct MemberSetters { } }; -// FIXME: Detect escape to field of field. -struct IndirectEscape{ +// FIXME: Escape to a field of a *named* subobject is not detected; only +// anonymous struct/union subobjects are traversed. +struct IndirectEscape { struct { const char *p; } b; - + void foo() { std::string s; b.p = s.data(); } }; -// FIXME: Detect escape to field of field. +// FIXME: Not detected either, and this shows *why* only anonymous subobjects are +// traversed: the named members `a` and `b` share the terminal field `Inner::p`, +// so a FieldDecl-keyed origin could not tell `a.p` apart from `b.p` if it +// traversed named subobjects. +struct IndirectEscapeSameType { + struct Inner { + const char *p; + }; + Inner a; + Inner b; + + void foo() { + std::string s; + a.p = s.data(); + } +}; + +// Escape to a field of an anonymous subobject is detected (members of an +// anonymous struct/union are uniquely named within the enclosing class). struct IndirectEscape2 { struct { - const char *p; + const char *p; // expected-note {{this field dangles}} }; void foo() { std::string s; - p = s.data(); + p = s.data(); // expected-warning {{stack memory associated with local variable 's' escapes to the field 'p' which will dangle}} + } +}; + +// A field reached through a *reference* member is NOT a subobject of `*this` +// (the reference binds to external storage), so it is not attributed to a +// `$this` field: no false diagnostic. +struct IndirectEscapeThroughReference { + struct Inner { const char *p; }; + Inner &r; + IndirectEscapeThroughReference(Inner &i) : r(i) {} + + void foo() { + std::string s; + r.p = s.data(); // no diagnostic: r.p is not a subobject of *this } }; `````````` </details> https://github.com/llvm/llvm-project/pull/232867 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
