[clang] [clang] Refine the temporay object member access filtering for GSL pointer (PR #122088)
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 20 06:59:28 PST 2025
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.wu at 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.wu at 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.wu at 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.wu at 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
More information about the cfe-commits
mailing list