[C++20][Coroutines] Lambda-coroutine with operator new in promise_type #84193
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-coroutines @llvm/pr-subscribers-clang Author: Andreas Fertig (andreasfertig) ChangesFix #84064 According to http://eel.is/c++draft/dcl.fct.def.coroutine#9 the first parameter for overload resolution of Before this patch, Clang handled class types correctly but ignored lambdas. This patch adds support for lambda coroutines with a The patch does consider C++23 Full diff: https://github.com/llvm/llvm-project/pull/84193.diff 5 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 25c4c58ad4ae43..f6d81a15487a3e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -6898,10 +6898,18 @@ class Sema final {
BinaryOperatorKind Operator);
//// ActOnCXXThis - Parse 'this' pointer.
- ExprResult ActOnCXXThis(SourceLocation loc);
+ ///
+ /// \param SkipLambdaCaptureCheck Whether to skip the 'this' check for a
+ /// lambda because 'this' is the lambda's 'this'-pointer.
+ ExprResult ActOnCXXThis(SourceLocation loc,
+ bool SkipLambdaCaptureCheck = false);
/// Build a CXXThisExpr and mark it referenced in the current context.
- Expr *BuildCXXThisExpr(SourceLocation Loc, QualType Type, bool IsImplicit);
+ ///
+ /// \param SkipLambdaCaptureCheck Whether to skip the 'this' check for a
+ /// lambda because 'this' is the lambda's 'this'-pointer.
+ Expr *BuildCXXThisExpr(SourceLocation Loc, QualType Type, bool IsImplicit,
+ bool SkipLambdaCaptureCheck = false);
void MarkThisReferenced(CXXThisExpr *This);
/// Try to retrieve the type of the 'this' pointer.
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index a969b9383563b2..d5655309d21f28 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -25,6 +25,7 @@
#include "clang/Sema/Initialization.h"
#include "clang/Sema/Overload.h"
#include "clang/Sema/ScopeInfo.h"
+#include "clang/Sema/Sema.h"
#include "clang/Sema/SemaInternal.h"
#include "llvm/ADT/SmallSet.h"
@@ -1378,8 +1379,21 @@ bool CoroutineStmtBuilder::makeReturnOnAllocFailure() {
static bool collectPlacementArgs(Sema &S, FunctionDecl &FD, SourceLocation Loc,
SmallVectorImpl<Expr *> &PlacementArgs) {
if (auto *MD = dyn_cast<CXXMethodDecl>(&FD)) {
- if (MD->isImplicitObjectMemberFunction() && !isLambdaCallOperator(MD)) {
- ExprResult ThisExpr = S.ActOnCXXThis(Loc);
+ if (MD->isImplicitObjectMemberFunction()) {
+ ExprResult ThisExpr{};
+
+ if (isLambdaCallOperator(MD) && !MD->isStatic()) {
+ Qualifiers ThisQuals = MD->getMethodQualifiers();
+ CXXRecordDecl *Record = MD->getParent();
+
+ Sema::CXXThisScopeRAII ThisScope(S, Record, ThisQuals,
+ Record != nullptr);
+
+ ThisExpr = S.ActOnCXXThis(Loc, /*SkipLambdaCaptureCheck*/ true);
+ } else {
+ ThisExpr = S.ActOnCXXThis(Loc);
+ }
+
if (ThisExpr.isInvalid())
return false;
ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index c34a40fa7c81ac..499c4943c23b01 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1414,7 +1414,7 @@ bool Sema::CheckCXXThisCapture(SourceLocation Loc, const bool Explicit,
return false;
}
-ExprResult Sema::ActOnCXXThis(SourceLocation Loc) {
+ExprResult Sema::ActOnCXXThis(SourceLocation Loc, bool SkipLambdaCaptureCheck) {
/// C++ 9.3.2: In the body of a non-static member function, the keyword this
/// is a non-lvalue expression whose value is the address of the object for
/// which the function is called.
@@ -1434,13 +1434,18 @@ ExprResult Sema::ActOnCXXThis(SourceLocation Loc) {
return Diag(Loc, diag::err_invalid_this_use) << 0;
}
- return BuildCXXThisExpr(Loc, ThisTy, /*IsImplicit=*/false);
+ return BuildCXXThisExpr(Loc, ThisTy, /*IsImplicit=*/false,
+ SkipLambdaCaptureCheck);
}
-Expr *Sema::BuildCXXThisExpr(SourceLocation Loc, QualType Type,
- bool IsImplicit) {
+Expr *Sema::BuildCXXThisExpr(SourceLocation Loc, QualType Type, bool IsImplicit,
+ bool SkipLambdaCaptureCheck) {
auto *This = CXXThisExpr::Create(Context, Loc, Type, IsImplicit);
- MarkThisReferenced(This);
+
+ if (!SkipLambdaCaptureCheck) {
+ MarkThisReferenced(This);
+ }
+
return This;
}
diff --git a/clang/test/SemaCXX/gh84064-1.cpp b/clang/test/SemaCXX/gh84064-1.cpp
new file mode 100644
index 00000000000000..dc7c475041094a
--- /dev/null
+++ b/clang/test/SemaCXX/gh84064-1.cpp
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -I%S/Inputs -std=c++20 %s
+
+// expected-no-diagnostics
+
+#include "std-coroutine.h"
+
+using size_t = decltype(sizeof(0));
+
+struct Generator {
+ struct promise_type {
+ int _val{};
+
+ Generator get_return_object() noexcept
+ {
+ return {};
+ }
+
+ std::suspend_never initial_suspend() noexcept
+ {
+ return {};
+ }
+
+ std::suspend_always final_suspend() noexcept
+ {
+ return {};
+ }
+
+ void return_void() noexcept {}
+ void unhandled_exception() noexcept {}
+
+ template<typename This, typename... TheRest>
+ static void*
+ operator new(size_t size,
+ This&,
+ TheRest&&...) noexcept
+ {
+ return nullptr;
+ }
+
+ static void operator delete(void*, size_t)
+ {
+ }
+ };
+};
+
+int main()
+{
+ auto lamb = []() -> Generator {
+ co_return;
+ };
+
+ static_assert(sizeof(decltype(lamb)) == 1);
+}
+
diff --git a/clang/test/SemaCXX/gh84064-2.cpp b/clang/test/SemaCXX/gh84064-2.cpp
new file mode 100644
index 00000000000000..457de43eab6d9e
--- /dev/null
+++ b/clang/test/SemaCXX/gh84064-2.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -I%S/Inputs -std=c++23 %s
+
+// expected-no-diagnostics
+
+#include "std-coroutine.h"
+
+using size_t = decltype(sizeof(0));
+
+struct GeneratorStatic {
+ struct promise_type {
+ int _val{};
+
+ GeneratorStatic get_return_object() noexcept
+ {
+ return {};
+ }
+
+ std::suspend_never initial_suspend() noexcept
+ {
+ return {};
+ }
+
+ std::suspend_always final_suspend() noexcept
+ {
+ return {};
+ }
+
+ void return_void() noexcept {}
+ void unhandled_exception() noexcept {}
+
+ template<typename... TheRest>
+ static void*
+ operator new(size_t size,
+ TheRest&&...) noexcept
+ {
+ return nullptr;
+ }
+
+ static void operator delete(void*, size_t)
+ {
+ }
+ };
+};
+
+
+int main()
+{
+ auto lambCpp23 = []() static -> GeneratorStatic {
+ co_return;
+ };
+
+ static_assert(sizeof(decltype(lambCpp23)) == 1);
+}
|
clang/include/clang/Sema/Sema.h
Outdated
| /// \param SkipLambdaCaptureCheck Whether to skip the 'this' check for a | ||
| /// lambda because 'this' is the lambda's 'this'-pointer. | ||
| ExprResult ActOnCXXThis(SourceLocation loc, | ||
| bool SkipLambdaCaptureCheck = false); |
There was a problem hiding this comment.
Not thrilled with the name of this variable, I would like to see it bikeshed to be more clear what it means from a 'callers' perspective rather than an implementation perspective.
There was a problem hiding this comment.
Sure, do you have any suggestions?
From what I understood, the previous implementation always checked whether capturing this was valid. My patch adds support for lambdas in coroutines with custom new. It needs a this'- pointer but not a captured one of the surrounding class; the lambdas own this'- pointer. Skipping the capture check sounded appropriate.
There was a problem hiding this comment.
Maybe something like, IsNonStaticLambda? But I guess this only applies in a coroutine situation, so perhaps that is imperfect too. I might have to think it through.
Perhaps @ChuanqiXu9 can come up with something.
There was a problem hiding this comment.
Maybe IsLambdasOwnThis?
There was a problem hiding this comment.
I can't parse that... can you describe that name a little more?
There was a problem hiding this comment.
Sure, have a look at this code (https://cppinsights.io/s/3212c7be)
class Test {
int a;
public:
Test(int x)
: a{x}
{
int other{};
auto l1 = [=] { return a + 2 + other; };
}
};
The lambda l1 has two things that we refer to as this-pointer. It's very own, as every non-static member function has. Used to access other like this->other inside the lambdas body. That this-pointer isn't captured. Hence, no check.
Then the captured this-pointer from the enclosing scope of Test. In C++ Insights called __this. While being used it looks like this->__this->a.
The check is in place for the latter. For the use case of calling operator new in the case of a coroutine, we are talking about the first one, which isn't captured.
While writing about this, I haven't checked whether a promise_type with a constructor worked before or does so now. I can check that maybe tomorrow.
There was a problem hiding this comment.
Excellent. Good. I suggest you to update this to the inline comments.
There was a problem hiding this comment.
If there is any problem with the promise_type with a constructor, let's make it in other patches.
There was a problem hiding this comment.
How about ThisRefersToClosureObject ?
There was a problem hiding this comment.
How about
ThisRefersToClosureObject?
Sounds good! Thanks!
andreasfertig
force-pushed
the
fixGH84064
branch
from
8f9336e to
680c99b
Compare
Fix llvm#84064 According to http://eel.is/c++draft/dcl.fct.def.coroutine#9 the first parameter for overload resolution of `operator new` is `size_t` followed by the arguments of the coroutine function. http://eel.is/c++draft/dcl.fct.def.coroutine#4 states that the first argument is the lvalue of `*this` if the coroutine is a member function. Before this patch, Clang handled class types correctly but ignored lambdas. This patch adds support for lambda coroutines with a `promise_type` that implements a custom `operator new`. The patch does consider C++23 `static operator()`, which already worked as there is no `this` parameter.
andreasfertig
force-pushed
the
fixGH84064
branch
from
680c99b to
9f8a741
Compare
|
The clang-format check fails due to a change in |
Nope, don't worry about that one. That bot is being weird, feel free to merge without it. |
|
@andreasfertig Will you need us to merge that for you? |
@cor3ntin: It looks like I need you, yes. I can only close the PR :-/ |
|
@andreasfertig Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
This commit breaks our coroutines library async_simple https://github.com/alibaba/async_simple . and here is a (relative) minimal reproducer: https://godbolt.org/z/sG5jzcGEz The reproducer comes from an implementation for async_simple::Generator (https://github.com/alibaba/async_simple/blob/main/async_simple/coro/Generator.h), which is basically the same implementation from the paper of I'll revert this patch to avoid the regression now. Sorry for not testing it sufficiently. |
|
Interesting! I'll see what I can do. |
|
Hello all, I did more investigation and found another shortcoming. In some cases, my initial implementation picked the wrong I pushed a new branch (https://github.com/andreasfertig/llvm-project/tree/fixGH84064take2), which also fixes this behavior and requires fewer changes. Regarding the crash, that was improper testing on my side. All tests let Clang crash, even the tests I checked in as part of the PR. The difference is that my tests used only This is where I'm stuck. The call stack for all cases is: The crash is within llvm-project/clang/lib/CodeGen/CGCall.cpp Line 5298 in 8bb9443
|
|
Hi Andreas, thanks for looking into this. I am still confused about whether or not your new branch can fix the crash or not. For the question about the crash itself, I don't have any insight though, I feel like this is a defect in the code generator. I didn't understand why mark a variable as referenced or not by the frontend may affect the code generation. |
|
Hello @ChuanqiXu9,
It's my pleasure! The new branch doesn't fix the crash. If I understand why it is crashing, this branch is a better change compared to the previous merged code.
I'm also struggling with that. As far as I could see, the coroutine part does nothing special for classes. I suspect that some other coroutine-unrelated part does something with the lambda. I also checked whether Do you know who I could ping to get assistance with the CodeGen part? |
Maybe @efriedma-quic @rjmccall |
Fix #84064
According to http://eel.is/c++draft/dcl.fct.def.coroutine#9 the first parameter for overload resolution of
operator newissize_tfollowed by the arguments of the coroutine function.http://eel.is/c++draft/dcl.fct.def.coroutine#4 states that the first argument is the lvalue of
*thisif the coroutine is a member function.Before this patch, Clang handled class types correctly but ignored lambdas. This patch adds support for lambda coroutines with a
promise_typethat implements a customoperator new.The patch does consider C++23
static operator(), which already worked as there is nothisparameter.