https://github.com/hokein updated https://github.com/llvm/llvm-project/pull/122088
>From 82415b4085f3ab956926909d68b16afff26ceb51 Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Tue, 17 Dec 2024 14:28:00 +0100 Subject: [PATCH 1/4] [clang] Refine the temporay object member access filtering for GSL pointer. --- clang/lib/Sema/CheckExprLifetime.cpp | 42 +++++++++++++------ clang/test/Sema/Inputs/lifetime-analysis.h | 2 +- .../Sema/warn-lifetime-analysis-nocfg.cpp | 28 +++++++++++++ 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 27e6b5b2cb3930..e144d23c6e7bb2 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()); @@ -1119,6 +1110,8 @@ shouldLifetimeExtendThroughPath(const IndirectLocalPath &Path) { for (auto Elem : Path) { if (Elem.Kind == IndirectLocalPathEntry::DefaultInit) return PathLifetimeKind::Extend; + if (Elem.Kind == IndirectLocalPathEntry::MemberExpr) + continue; if (Elem.Kind != IndirectLocalPathEntry::LambdaCaptureInit) return PathLifetimeKind::NoExtend; } @@ -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: @@ -1200,6 +1195,26 @@ static AnalysisResult analyzePathForGSLPointer(const IndirectLocalPath &Path, // 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())) { + 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]]); @@ -1527,6 +1542,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..3b271af9dfa13f 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -806,3 +806,31 @@ 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]]; +}; +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}} +} + +struct Bar {}; +struct Foo { + std::vector<Bar> v; +}; +Foo getFoo(); +void test2() { + const Foo& foo = getFoo(); + const Bar& bar = foo.v.back(); // OK +} + +} // namespace GH120543 >From 765d56630ca4cf4583bfb4c954f00ec3a4da4fb9 Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Thu, 9 Jan 2025 09:03:22 +0100 Subject: [PATCH 2/4] Add a lifetimebound testcase --- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 3b271af9dfa13f..48f52b7fc2788c 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -815,12 +815,18 @@ struct 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 {}; >From 87fea691ea116907f6b93e1703f218bd9a5ada0b Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Mon, 13 Jan 2025 11:52:49 +0100 Subject: [PATCH 3/4] address review comment. --- clang/lib/Sema/CheckExprLifetime.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index e144d23c6e7bb2..b3ee993c17de8c 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -1108,12 +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::MemberExpr) + if (Elem.Kind == IndirectLocalPathEntry::MemberExpr || + Elem.Kind == IndirectLocalPathEntry::LambdaCaptureInit) continue; - if (Elem.Kind != IndirectLocalPathEntry::LambdaCaptureInit) - return PathLifetimeKind::NoExtend; + return Elem.Kind == IndirectLocalPathEntry::DefaultInit + ? PathLifetimeKind::Extend + : PathLifetimeKind::NoExtend; } return PathLifetimeKind::Extend; } >From 3276f4ef1fa3ea9c5b27f4f7eac055c594737d74 Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Mon, 20 Jan 2025 15:58:42 +0100 Subject: [PATCH 4/4] Fix a false postive in member initializer. --- clang/lib/Sema/CheckExprLifetime.cpp | 8 +++++--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp | 12 ++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index b3ee993c17de8c..8963cad86dbcae 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -1188,7 +1188,7 @@ 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; @@ -1208,7 +1208,8 @@ static AnalysisResult analyzePathForGSLPointer(const IndirectLocalPath &Path, // 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())) { + isRecordWithAttr<OwnerAttr>(FD->getType()) && + LK != LK_MemInitializer) { return Report; } return Abandon; @@ -1312,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: @@ -1444,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; diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 48f52b7fc2788c..04bb1330ded4c2 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -839,4 +839,16 @@ void test2() { 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