Author: Haojian Wu Date: 2025-01-22T14:11:16+01:00 New Revision: bd56950b9cd5b6b07e1ccb9b02c5d8a7125a23b6
URL: https://github.com/llvm/llvm-project/commit/bd56950b9cd5b6b07e1ccb9b02c5d8a7125a23b6 DIFF: https://github.com/llvm/llvm-project/commit/bd56950b9cd5b6b07e1ccb9b02c5d8a7125a23b6.diff LOG: [clang] Refine the temporay object member access filtering for GSL pointer (#122088) We currently have ad-hoc filtering logic for temporary object member access in `VisitGSLPointerArg`. This logic filters out more cases than it should, leading to false negatives. Furthermore, this location lacks sufficient context to implement a more accurate solution. This patch refines the filtering logic by moving it to the central filtering location, `analyzePathForGSLPointer`, consolidating the logic and avoiding scattered filtering across multiple places. As a result, the special handling for conditional operators (#120233) is no longer necessary. This change also resolves #120543. Added: Modified: clang/lib/Sema/CheckExprLifetime.cpp clang/test/Sema/Inputs/lifetime-analysis.h clang/test/Sema/warn-lifetime-analysis-nocfg.cpp Removed: ################################################################################ diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 27e6b5b2cb3930..8963cad86dbcae 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -200,6 +200,7 @@ struct IndirectLocalPathEntry { LifetimeBoundCall, TemporaryCopy, LambdaCaptureInit, + MemberExpr, GslReferenceInit, GslPointerInit, GslPointerAssignment, @@ -593,19 +594,6 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, Path.pop_back(); }; auto VisitGSLPointerArg = [&](const FunctionDecl *Callee, Expr *Arg) { - // We are not interested in the temporary base objects of gsl Pointers: - // Temp().ptr; // Here ptr might not dangle. - if (isa<MemberExpr>(Arg->IgnoreImpCasts())) - return; - // Avoid false positives when the object is constructed from a conditional - // operator argument. A common case is: - // // 'ptr' might not be owned by the Owner object. - // std::string_view s = cond() ? Owner().ptr : sv; - if (const auto *Cond = - dyn_cast<AbstractConditionalOperator>(Arg->IgnoreImpCasts()); - Cond && isPointerLikeType(Cond->getType())) - return; - auto ReturnType = Callee->getReturnType(); // Once we initialized a value with a non gsl-owner reference, it can no @@ -726,6 +714,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, Init = ILE->getInit(0); } + if (MemberExpr *ME = dyn_cast<MemberExpr>(Init->IgnoreImpCasts())) + Path.push_back( + {IndirectLocalPathEntry::MemberExpr, ME, ME->getMemberDecl()}); // Step over any subobject adjustments; we may have a materialized // temporary inside them. Init = const_cast<Expr *>(Init->skipRValueSubobjectAdjustments()); @@ -1117,10 +1108,12 @@ enum PathLifetimeKind { static PathLifetimeKind shouldLifetimeExtendThroughPath(const IndirectLocalPath &Path) { for (auto Elem : Path) { - if (Elem.Kind == IndirectLocalPathEntry::DefaultInit) - return PathLifetimeKind::Extend; - if (Elem.Kind != IndirectLocalPathEntry::LambdaCaptureInit) - return PathLifetimeKind::NoExtend; + if (Elem.Kind == IndirectLocalPathEntry::MemberExpr || + Elem.Kind == IndirectLocalPathEntry::LambdaCaptureInit) + continue; + return Elem.Kind == IndirectLocalPathEntry::DefaultInit + ? PathLifetimeKind::Extend + : PathLifetimeKind::NoExtend; } return PathLifetimeKind::Extend; } @@ -1138,6 +1131,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I, case IndirectLocalPathEntry::GslPointerInit: case IndirectLocalPathEntry::GslPointerAssignment: case IndirectLocalPathEntry::ParenAggInit: + case IndirectLocalPathEntry::MemberExpr: // These exist primarily to mark the path as not permitting or // supporting lifetime extension. break; @@ -1167,6 +1161,7 @@ static bool pathOnlyHandlesGslPointer(const IndirectLocalPath &Path) { case IndirectLocalPathEntry::VarInit: case IndirectLocalPathEntry::AddressOf: case IndirectLocalPathEntry::LifetimeBoundCall: + case IndirectLocalPathEntry::MemberExpr: continue; case IndirectLocalPathEntry::GslPointerInit: case IndirectLocalPathEntry::GslReferenceInit: @@ -1193,13 +1188,34 @@ enum AnalysisResult { // Analyze cases where a GSLPointer is initialized or assigned from a // temporary owner object. static AnalysisResult analyzePathForGSLPointer(const IndirectLocalPath &Path, - Local L) { + Local L, LifetimeKind LK) { if (!pathOnlyHandlesGslPointer(Path)) return NotGSLPointer; // At this point, Path represents a series of operations involving a // GSLPointer, either in the process of initialization or assignment. + // Process temporary base objects for MemberExpr cases, e.g. Temp().field. + for (const auto &E : Path) { + if (E.Kind == IndirectLocalPathEntry::MemberExpr) { + // Avoid interfering with the local base object. + if (pathContainsInit(Path)) + return Abandon; + + // We are not interested in the temporary base objects of gsl Pointers: + // auto p1 = Temp().ptr; // Here p1 might not dangle. + // However, we want to diagnose for gsl owner fields: + // auto p2 = Temp().owner; // Here p2 is dangling. + if (const auto *FD = llvm::dyn_cast_or_null<FieldDecl>(E.D); + FD && !FD->getType()->isReferenceType() && + isRecordWithAttr<OwnerAttr>(FD->getType()) && + LK != LK_MemInitializer) { + return Report; + } + return Abandon; + } + } + // Note: A LifetimeBoundCall can appear interleaved in this sequence. // For example: // const std::string& Ref(const std::string& a [[clang::lifetimebound]]); @@ -1297,7 +1313,7 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity, auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L); bool IsGslPtrValueFromGslTempOwner = true; - switch (analyzePathForGSLPointer(Path, L)) { + switch (analyzePathForGSLPointer(Path, L, LK)) { case Abandon: return false; case Skip: @@ -1429,6 +1445,7 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity, auto *DRE = dyn_cast<DeclRefExpr>(L); // Suppress false positives for code like the one below: // Ctor(unique_ptr<T> up) : pointer(up.get()), owner(move(up)) {} + // FIXME: move this logic to analyzePathForGSLPointer. if (DRE && isRecordWithAttr<OwnerAttr>(DRE->getType())) return false; @@ -1527,6 +1544,7 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity, case IndirectLocalPathEntry::LifetimeBoundCall: case IndirectLocalPathEntry::TemporaryCopy: + case IndirectLocalPathEntry::MemberExpr: case IndirectLocalPathEntry::GslPointerInit: case IndirectLocalPathEntry::GslReferenceInit: case IndirectLocalPathEntry::GslPointerAssignment: diff --git a/clang/test/Sema/Inputs/lifetime-analysis.h b/clang/test/Sema/Inputs/lifetime-analysis.h index f888e6ab94bb6a..d318033ff0cc45 100644 --- a/clang/test/Sema/Inputs/lifetime-analysis.h +++ b/clang/test/Sema/Inputs/lifetime-analysis.h @@ -52,7 +52,7 @@ struct vector { void push_back(const T&); void push_back(T&&); - + const T& back() const; void insert(iterator, T&&); }; diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 4c19367bb7f3dd..04bb1330ded4c2 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -806,3 +806,49 @@ std::string_view test2(int c, std::string_view sv) { } } // namespace GH120206 + +namespace GH120543 { +struct S { + std::string_view sv; + std::string s; +}; +struct Q { + const S* get() const [[clang::lifetimebound]]; +}; + +std::string_view foo(std::string_view sv [[clang::lifetimebound]]); + +void test1() { + std::string_view k1 = S().sv; // OK + std::string_view k2 = S().s; // expected-warning {{object backing the pointer will}} + + std::string_view k3 = Q().get()->sv; // OK + std::string_view k4 = Q().get()->s; // expected-warning {{object backing the pointer will}} + + std::string_view lb1 = foo(S().s); // expected-warning {{object backing the pointer will}} + std::string_view lb2 = foo(Q().get()->s); // expected-warning {{object backing the pointer will}} +} + +struct Bar {}; +struct Foo { + std::vector<Bar> v; +}; +Foo getFoo(); +void test2() { + const Foo& foo = getFoo(); + const Bar& bar = foo.v.back(); // OK +} + +struct Foo2 { + std::unique_ptr<Bar> bar; +}; + +struct Test { + Test(Foo2 foo) : bar(foo.bar.get()), // OK + storage(std::move(foo.bar)) {}; + + Bar* bar; + std::unique_ptr<Bar> storage; +}; + +} // namespace GH120543 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
