Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++20][Coroutines] lambda-coroutine with promise_type ctor. #84519

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andreasfertig
Copy link
Contributor

This is a follow-up of #84064. It turned out that a coroutine-lambda with a promise_type and a user-defined constructor ignores the this pointer. Per http://eel.is/c++draft/dcl.fct.def.coroutine#4, in such a case, the first parameter to the constructor is an lvalue of *this.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" coroutines C++20 coroutines labels Mar 8, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 8, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-coroutines

Author: Andreas Fertig (andreasfertig)

Changes

This is a follow-up of #84064. It turned out that a coroutine-lambda with a promise_type and a user-defined constructor ignores the this pointer. Per http://eel.is/c++draft/dcl.fct.def.coroutine#4, in such a case, the first parameter to the constructor is an lvalue of *this.


Full diff: https://github.com/llvm/llvm-project/pull/84519.diff

3 Files Affected:

  • (modified) clang/lib/Sema/SemaCoroutine.cpp (+15-2)
  • (added) clang/test/SemaCXX/coroutine-promise-ctor-lambda.cpp (+71)
  • (added) clang/test/SemaCXX/cxx23-coroutine-promise-ctor-static-callop-lambda.cpp (+47)
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 301a5ff72a3b2a..79da92083a2be7 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -596,8 +596,21 @@ VarDecl *Sema::buildCoroutinePromise(SourceLocation Loc) {
 
   // Add implicit object parameter.
   if (auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
-    if (MD->isImplicitObjectMemberFunction() && !isLambdaCallOperator(MD)) {
-      ExprResult ThisExpr = ActOnCXXThis(Loc);
+    if (MD->isImplicitObjectMemberFunction()) {
+      ExprResult ThisExpr{};
+
+      if (isLambdaCallOperator(MD) && !MD->isStatic()) {
+        Qualifiers ThisQuals = MD->getMethodQualifiers();
+        CXXRecordDecl *Record = MD->getParent();
+
+        Sema::CXXThisScopeRAII ThisScope(*this, Record, ThisQuals,
+                                         Record != nullptr);
+
+        ThisExpr = ActOnCXXThis(Loc, /*ThisRefersToClosureObject=*/true);
+      } else {
+        ThisExpr = ActOnCXXThis(Loc);
+      }
+
       if (ThisExpr.isInvalid())
         return nullptr;
       ThisExpr = CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
diff --git a/clang/test/SemaCXX/coroutine-promise-ctor-lambda.cpp b/clang/test/SemaCXX/coroutine-promise-ctor-lambda.cpp
new file mode 100644
index 00000000000000..92e9a006c3a8d9
--- /dev/null
+++ b/clang/test/SemaCXX/coroutine-promise-ctor-lambda.cpp
@@ -0,0 +1,71 @@
+// 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>
+    promise_type(This&,
+                 TheRest&&...)
+    {
+    }
+  };
+};
+
+struct CapturingThisTest
+{
+    int x{};
+
+    void AsPointer()
+    {
+      auto lamb = [=,this]() -> Generator {
+        int y = x;
+        co_return;
+      };
+
+      static_assert(sizeof(decltype(lamb)) == sizeof(void*));
+    }
+
+    void AsStarThis()
+    {
+      auto lamb = [*this]() -> Generator {
+        int y = x;
+        co_return;
+      };
+
+      static_assert(sizeof(decltype(lamb)) == sizeof(int));
+    }
+};
+
+int main()
+{
+  auto lamb = []() -> Generator {
+    co_return;
+  };
+
+  static_assert(sizeof(decltype(lamb)) == 1);
+}
diff --git a/clang/test/SemaCXX/cxx23-coroutine-promise-ctor-static-callop-lambda.cpp b/clang/test/SemaCXX/cxx23-coroutine-promise-ctor-static-callop-lambda.cpp
new file mode 100644
index 00000000000000..0e9e63bce86b87
--- /dev/null
+++ b/clang/test/SemaCXX/cxx23-coroutine-promise-ctor-static-callop-lambda.cpp
@@ -0,0 +1,47 @@
+// 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 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... TheRest>
+    promise_type(TheRest&&...)
+    {
+    }
+  };
+};
+
+
+int main()
+{
+  auto lamb = []() static -> Generator {
+    co_return;
+  };
+
+  static_assert(sizeof(decltype(lamb)) == 1);
+}
+

@andreasfertig
Copy link
Contributor Author

andreasfertig commented Mar 8, 2024

@ChuanqiXu9 @erichkeane @cor3ntin here is a dedicated patch to support a promise_type with a user-provided constructor in a coroutine used with a lambda.

@Sirraide Sirraide removed the clang Clang issues not falling into any other category label Mar 8, 2024
Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick

clang/lib/Sema/SemaCoroutine.cpp Outdated Show resolved Hide resolved
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Mar 9, 2024
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a changelog entry for that?

clang/lib/Sema/SemaCoroutine.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This is a follow-up of llvm#84064. It turned out that a coroutine-lambda
with a `promise_type` and a user-defined constructor ignores the `this`
pointer. Per http://eel.is/c++draft/dcl.fct.def.coroutine#4, in such a
case, the first parameter to the constructor is an lvalue of `*this`.
@andreasfertig
Copy link
Contributor Author

Can we add a changelog entry for that?

Done!

@andreasfertig andreasfertig marked this pull request as draft March 11, 2024 16:08
Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff c54e0524eeffcf2c5428cd2bc0449a1989e82cd8 7d06524095a48cd6ea5fde089d857cb2b0aa6efb -- clang/test/SemaCXX/coroutine-promise-ctor-lambda.cpp clang/test/SemaCXX/cxx23-coroutine-promise-ctor-static-callop-lambda.cpp clang/lib/Sema/SemaCoroutine.cpp clang/test/SemaCXX/gh84064-1.cpp
View the diff from clang-format here.
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index d8359367b9..29e4a33199 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -1673,7 +1673,6 @@ bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
   if (NewExpr.isInvalid())
     return false;
 
-
   // Make delete call.
 
   QualType OpDeleteQualType = OperatorDelete->getType();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category coroutines C++20 coroutines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants