[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)
Utkarsh Saxena via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 11 07:16:58 PST 2024
https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/77066
>From 3e0d0ab6c4fc6cba68285816a95e423bc18e8e55 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Fri, 5 Jan 2024 10:11:20 +0100
Subject: [PATCH 1/5] [coroutines] Detect lifetime issues with coroutine lambda
captures
---
clang/lib/Sema/SemaInit.cpp | 20 +++++--
clang/test/SemaCXX/coro-lifetimebound.cpp | 64 +++++++++++++++++++++--
2 files changed, 76 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 60c0e3e74204ec..c100bf11454786 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -12,6 +12,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/DeclObjC.h"
+#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ExprObjC.h"
#include "clang/AST/ExprOpenMP.h"
@@ -33,6 +34,7 @@
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Casting.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/raw_ostream.h"
@@ -7575,15 +7577,27 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
Path.pop_back();
};
- if (ObjectArg && implicitObjectParamIsLifetimeBound(Callee))
- VisitLifetimeBoundArg(Callee, ObjectArg);
-
bool CheckCoroCall = false;
if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
CheckCoroCall = RD->hasAttr<CoroLifetimeBoundAttr>() &&
RD->hasAttr<CoroReturnTypeAttr>() &&
!Callee->hasAttr<CoroDisableLifetimeBoundAttr>();
}
+
+ if (ObjectArg) {
+ bool CheckCoroObjArg = CheckCoroCall;
+ // Ignore `__promise.get_return_object()` as it not lifetimebound.
+ if (Callee->getDeclName().isIdentifier() &&
+ Callee->getName() == "get_return_object")
+ CheckCoroObjArg = false;
+ // Coroutine lambda objects with empty capture list are not lifetimebound.
+ if (auto *LE = dyn_cast<LambdaExpr>(ObjectArg->IgnoreImplicit());
+ LE && LE->captures().empty())
+ CheckCoroObjArg = false;
+ if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg)
+ VisitLifetimeBoundArg(Callee, ObjectArg);
+ }
+
for (unsigned I = 0,
N = std::min<unsigned>(Callee->getNumParams(), Args.size());
I != N; ++I) {
diff --git a/clang/test/SemaCXX/coro-lifetimebound.cpp b/clang/test/SemaCXX/coro-lifetimebound.cpp
index 3fc7ca70a14a12..319134450e4b6f 100644
--- a/clang/test/SemaCXX/coro-lifetimebound.cpp
+++ b/clang/test/SemaCXX/coro-lifetimebound.cpp
@@ -64,6 +64,10 @@ Co<int> bar_coro(const int &b, int c) {
: bar_coro(0, 1); // expected-warning {{returning address of local temporary object}}
}
+// =============================================================================
+// Lambdas
+// =============================================================================
+namespace lambdas {
void lambdas() {
auto unsafe_lambda = [] [[clang::coro_wrapper]] (int b) {
return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}}
@@ -84,15 +88,47 @@ void lambdas() {
co_return x + co_await foo_coro(b);
};
}
+
+Co<int> lambda_captures() {
+ int a = 1;
+ // Temporary lambda object dies.
+ auto lamb = [a](int x, const int& y) -> Co<int> { // expected-warning {{temporary whose address is used as value of local variable 'lamb'}}
+ co_return x + y + a;
+ }(1, a);
+ // Object dies but it has no capture.
+ auto no_capture = []() -> Co<int> { co_return 1; }();
+ auto bad_no_capture = [](const int& a) -> Co<int> { co_return a; }(1); // expected-warning {{temporary}}
+ // Temporary lambda object with lifetime extension under co_await.
+ int res = co_await [a](int x, const int& y) -> Co<int> {
+ co_return x + y + a;
+ }(1, a);
+ co_return 1;
+}
+} // namespace lambdas
+
// =============================================================================
-// Safe usage when parameters are value
+// Member coroutines
// =============================================================================
-namespace by_value {
-Co<int> value_coro(int b) { co_return co_await foo_coro(b); }
-[[clang::coro_wrapper]] Co<int> wrapper1(int b) { return value_coro(b); }
-[[clang::coro_wrapper]] Co<int> wrapper2(const int& b) { return value_coro(b); }
+namespace member_coroutines{
+struct S {
+ Co<int> member(const int& a) { co_return a; }
+};
+
+Co<int> use() {
+ S s;
+ int a = 1;
+ auto test1 = s.member(1); // expected-warning {{temporary whose address is used as value of local variable}}
+ auto test2 = s.member(a);
+ auto test3 = S{}.member(a); // expected-warning {{temporary whose address is used as value of local variable}}
+ co_return 1;
}
+[[clang::coro_wrapper]] Co<int> wrapper(const int& a) {
+ S s;
+ return s.member(a); // expected-warning {{address of stack memory}}
+}
+} // member_coroutines
+
// =============================================================================
// Lifetime bound but not a Coroutine Return Type: No analysis.
// =============================================================================
@@ -129,4 +165,22 @@ Co<int> foo_wrapper(const int& x) { return foo(x); }
// The call to foo_wrapper is wrapper is safe.
return foo_wrapper(1);
}
+
+struct S{
+[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
+Co<int> member(const int& x) { return foo(x); }
+};
+
+Co<int> use() {
+ S s;
+ int a = 1;
+ auto test1 = s.member(1); // param is not flagged.
+ auto test2 = S{}.member(a); // 'this' is not flagged.
+ co_return 1;
+}
+
+[[clang::coro_wrapper]] Co<int> return_stack_addr(const int& a) {
+ S s;
+ return s.member(a); // return of stack addr is not flagged.
+}
} // namespace disable_lifetimebound
>From 0d8d78e30dff896880e674240c8bf47b13c527f5 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Fri, 5 Jan 2024 11:39:41 +0100
Subject: [PATCH 2/5] add more test
---
clang/lib/Sema/SemaInit.cpp | 4 ++--
clang/test/SemaCXX/coro-lifetimebound.cpp | 17 +++++++++++++++--
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index c100bf11454786..1aa6be826699bb 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -7586,8 +7586,8 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
if (ObjectArg) {
bool CheckCoroObjArg = CheckCoroCall;
- // Ignore `__promise.get_return_object()` as it not lifetimebound.
- if (Callee->getDeclName().isIdentifier() &&
+ // Ignore `__promise.get_return_object()` as it is not lifetimebound.
+ if (CheckCoroObjArg && Callee->getDeclName().isIdentifier() &&
Callee->getName() == "get_return_object")
CheckCoroObjArg = false;
// Coroutine lambda objects with empty capture list are not lifetimebound.
diff --git a/clang/test/SemaCXX/coro-lifetimebound.cpp b/clang/test/SemaCXX/coro-lifetimebound.cpp
index 319134450e4b6f..b61013ec057381 100644
--- a/clang/test/SemaCXX/coro-lifetimebound.cpp
+++ b/clang/test/SemaCXX/coro-lifetimebound.cpp
@@ -102,6 +102,10 @@ Co<int> lambda_captures() {
int res = co_await [a](int x, const int& y) -> Co<int> {
co_return x + y + a;
}(1, a);
+ // Lambda object on stack should be fine.
+ auto lamb2 = [a]() -> Co<int> { co_return a; };
+ auto on_stack = lamb2();
+ auto res2 = co_await on_stack;
co_return 1;
}
} // namespace lambdas
@@ -111,7 +115,7 @@ Co<int> lambda_captures() {
// =============================================================================
namespace member_coroutines{
struct S {
- Co<int> member(const int& a) { co_return a; }
+ Co<int> member(const int& a) { co_return a; }
};
Co<int> use() {
@@ -129,6 +133,15 @@ Co<int> use() {
}
} // member_coroutines
+// =============================================================================
+// Safe usage when parameters are value
+// =============================================================================
+namespace by_value {
+Co<int> value_coro(int b) { co_return co_await foo_coro(b); }
+[[clang::coro_wrapper]] Co<int> wrapper1(int b) { return value_coro(b); }
+[[clang::coro_wrapper]] Co<int> wrapper2(const int& b) { return value_coro(b); }
+} // namespace by_value
+
// =============================================================================
// Lifetime bound but not a Coroutine Return Type: No analysis.
// =============================================================================
@@ -158,7 +171,7 @@ CoNoCRT<int> bar(int a) {
namespace disable_lifetimebound {
Co<int> foo(int x) { co_return x; }
-[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
+[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
Co<int> foo_wrapper(const int& x) { return foo(x); }
[[clang::coro_wrapper]] Co<int> caller() {
>From e4b801784962946eedf8b5e7c229871588b1621b Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Wed, 10 Jan 2024 13:06:16 +0000
Subject: [PATCH 3/5] Refactor isGetReturnObject()
---
clang/include/clang/Sema/Sema.h | 6 ++++++
clang/lib/Sema/SemaDecl.cpp | 3 +--
clang/lib/Sema/SemaInit.cpp | 3 +--
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 8f44adef38159e..a17a9444d201b6 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11219,6 +11219,12 @@ class Sema final {
bool buildCoroutineParameterMoves(SourceLocation Loc);
VarDecl *buildCoroutinePromise(SourceLocation Loc);
void CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body);
+ // Heuristaclly tells if the function is promise::get_return_object().
+ static bool isGetReturnObject(const FunctionDecl *FD) {
+ return isa_and_nonnull<CXXMethodDecl>(FD) &&
+ FD->getDeclName().isIdentifier() &&
+ FD->getName().equals("get_return_object") && FD->param_empty();
+ }
// As a clang extension, enforces that a non-coroutine function must be marked
// with [[clang::coro_wrapper]] if it returns a type marked with
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 8e46c4984d93dc..2f93e803d11c69 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -15846,8 +15846,7 @@ void Sema::CheckCoroutineWrapper(FunctionDecl *FD) {
if (!RD || !RD->getUnderlyingDecl()->hasAttr<CoroReturnTypeAttr>())
return;
// Allow `get_return_object()`.
- if (FD->getDeclName().isIdentifier() &&
- FD->getName().equals("get_return_object") && FD->param_empty())
+ if (isGetReturnObject(FD))
return;
if (!FD->hasAttr<CoroWrapperAttr>())
Diag(FD->getLocation(), diag::err_coroutine_return_type) << RD;
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 1aa6be826699bb..848184ff4b724e 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -7587,8 +7587,7 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
if (ObjectArg) {
bool CheckCoroObjArg = CheckCoroCall;
// Ignore `__promise.get_return_object()` as it is not lifetimebound.
- if (CheckCoroObjArg && Callee->getDeclName().isIdentifier() &&
- Callee->getName() == "get_return_object")
+ if (CheckCoroObjArg && Sema::isGetReturnObject(Callee))
CheckCoroObjArg = false;
// Coroutine lambda objects with empty capture list are not lifetimebound.
if (auto *LE = dyn_cast<LambdaExpr>(ObjectArg->IgnoreImplicit());
>From 4c05d54e07d4f650bf5694a3ddc365078b854627 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Thu, 11 Jan 2024 15:03:41 +0000
Subject: [PATCH 4/5] implicitly add coro_wrapper and
coro_disable_lifetimebound to get_return_object
---
clang/include/clang/Sema/Sema.h | 6 ------
clang/lib/Sema/SemaCoroutine.cpp | 22 ++++++++++++++++++++++
clang/lib/Sema/SemaDecl.cpp | 7 +++++--
clang/lib/Sema/SemaInit.cpp | 3 ---
clang/test/SemaCXX/coro-lifetimebound.cpp | 2 +-
5 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a17a9444d201b6..8f44adef38159e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11219,12 +11219,6 @@ class Sema final {
bool buildCoroutineParameterMoves(SourceLocation Loc);
VarDecl *buildCoroutinePromise(SourceLocation Loc);
void CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body);
- // Heuristaclly tells if the function is promise::get_return_object().
- static bool isGetReturnObject(const FunctionDecl *FD) {
- return isa_and_nonnull<CXXMethodDecl>(FD) &&
- FD->getDeclName().isIdentifier() &&
- FD->getName().equals("get_return_object") && FD->param_empty();
- }
// As a clang extension, enforces that a non-coroutine function must be marked
// with [[clang::coro_wrapper]] if it returns a type marked with
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index bee80db8d166a6..0b03ca4ea1fa9c 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -297,6 +297,26 @@ struct ReadySuspendResumeResult {
bool IsInvalid;
};
+// Adds [[clang::coro_wrapper]] and [[clang::coro_disable_lifetimebound]]
+// attributes to `get_return_object`.
+static void handleGetReturnObject(Sema &S, MemberExpr *ME) {
+ if (!ME || !ME->getMemberDecl() || !ME->getMemberDecl()->getAsFunction())
+ return;
+ auto *MD = ME->getMemberDecl()->getAsFunction();
+ auto* RetType = MD->getReturnType()->getAsRecordDecl();
+ if (!RetType || !RetType->hasAttr<CoroReturnTypeAttr>())
+ return;
+ // `get_return_object` should be allowed to return coro_return_type.
+ if (!MD->hasAttr<CoroWrapperAttr>())
+ MD->addAttr(
+ CoroWrapperAttr::CreateImplicit(S.getASTContext(), MD->getLocation()));
+ // Object arg of `__promise.get_return_object()` is not lifetimebound.
+ if (RetType->hasAttr<CoroLifetimeBoundAttr>() &&
+ !MD->hasAttr<CoroDisableLifetimeBoundAttr>())
+ MD->addAttr(CoroDisableLifetimeBoundAttr::CreateImplicit(
+ S.getASTContext(), MD->getLocation()));
+}
+
static ExprResult buildMemberCall(Sema &S, Expr *Base, SourceLocation Loc,
StringRef Name, MultiExprArg Args) {
DeclarationNameInfo NameInfo(&S.PP.getIdentifierTable().get(Name), Loc);
@@ -319,6 +339,8 @@ static ExprResult buildMemberCall(Sema &S, Expr *Base, SourceLocation Loc,
return ExprError();
}
+ if (Name.equals("get_return_object"))
+ handleGetReturnObject(S, dyn_cast<MemberExpr>(Result.get()));
auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
}
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 2f93e803d11c69..efd3232261123b 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -15845,8 +15845,11 @@ void Sema::CheckCoroutineWrapper(FunctionDecl *FD) {
RecordDecl *RD = FD->getReturnType()->getAsRecordDecl();
if (!RD || !RD->getUnderlyingDecl()->hasAttr<CoroReturnTypeAttr>())
return;
- // Allow `get_return_object()`.
- if (isGetReturnObject(FD))
+ // Allow some_promise_type::get_return_object().
+ // Since we are still in the promise definition, we can only do this
+ // heuristically as the promise may not be yet associated to a coroutine.
+ if (isa<CXXMethodDecl>(FD) && FD->getDeclName().isIdentifier() &&
+ FD->getName().equals("get_return_object") && FD->param_empty())
return;
if (!FD->hasAttr<CoroWrapperAttr>())
Diag(FD->getLocation(), diag::err_coroutine_return_type) << RD;
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 848184ff4b724e..05e71a0b4960ab 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -7586,9 +7586,6 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
if (ObjectArg) {
bool CheckCoroObjArg = CheckCoroCall;
- // Ignore `__promise.get_return_object()` as it is not lifetimebound.
- if (CheckCoroObjArg && Sema::isGetReturnObject(Callee))
- CheckCoroObjArg = false;
// Coroutine lambda objects with empty capture list are not lifetimebound.
if (auto *LE = dyn_cast<LambdaExpr>(ObjectArg->IgnoreImplicit());
LE && LE->captures().empty())
diff --git a/clang/test/SemaCXX/coro-lifetimebound.cpp b/clang/test/SemaCXX/coro-lifetimebound.cpp
index b61013ec057381..9e96a296562a05 100644
--- a/clang/test/SemaCXX/coro-lifetimebound.cpp
+++ b/clang/test/SemaCXX/coro-lifetimebound.cpp
@@ -180,7 +180,7 @@ Co<int> foo_wrapper(const int& x) { return foo(x); }
}
struct S{
-[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
+[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
Co<int> member(const int& x) { return foo(x); }
};
>From 9858f46932f8cf8775c11496b2fe069410c513f4 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Thu, 11 Jan 2024 15:16:46 +0000
Subject: [PATCH 5/5] format
---
clang/lib/Sema/SemaCoroutine.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 0b03ca4ea1fa9c..ef08e1ee750d17 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -303,7 +303,7 @@ static void handleGetReturnObject(Sema &S, MemberExpr *ME) {
if (!ME || !ME->getMemberDecl() || !ME->getMemberDecl()->getAsFunction())
return;
auto *MD = ME->getMemberDecl()->getAsFunction();
- auto* RetType = MD->getReturnType()->getAsRecordDecl();
+ auto *RetType = MD->getReturnType()->getAsRecordDecl();
if (!RetType || !RetType->hasAttr<CoroReturnTypeAttr>())
return;
// `get_return_object` should be allowed to return coro_return_type.
More information about the cfe-commits
mailing list