[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa1ffba8d5286: [C++20] [Coroutines] Conform the updates for 
CWG issue 2585 (authored by ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126187/new/

https://reviews.llvm.org/D126187

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/CodeGenCoroutines/coro-alloc-2.cpp

Index: clang/test/CodeGenCoroutines/coro-alloc-2.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-alloc-2.cpp
@@ -0,0 +1,30 @@
+// Tests that we wouldn't generate an allocation call in global scope with (std::size_t, p0, ..., pn)
+// RUN: %clang_cc1 %s -std=c++20 -S -triple x86_64-unknown-linux-gnu -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
+#include "Inputs/coroutine.h"
+
+namespace std {
+typedef decltype(sizeof(int)) size_t;
+}
+
+struct Allocator {};
+
+struct resumable {
+  struct promise_type {
+
+resumable get_return_object() { return {}; }
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+void unhandled_exception() {}
+void return_void(){};
+  };
+};
+
+void *operator new(std::size_t, void *);
+
+resumable f1(void *) {
+  co_return;
+}
+
+// CHECK: coro.alloc:
+// CHECK-NEXT: [[SIZE:%.+]] = call [[BITWIDTH:.+]] @llvm.coro.size.[[BITWIDTH]]()
+// CHECK-NEXT: call {{.*}} ptr @_Znwm([[BITWIDTH]] noundef [[SIZE]])
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1239,6 +1239,41 @@
   return true;
 }
 
+// Collect placement arguments for allocation function of coroutine FD.
+// Return true if we collect placement arguments succesfully. Return false,
+// otherwise.
+static bool collectPlacementArgs(Sema , FunctionDecl , SourceLocation Loc,
+ SmallVectorImpl ) {
+  if (auto *MD = dyn_cast()) {
+if (MD->isInstance() && !isLambdaCallOperator(MD)) {
+  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
+  if (ThisExpr.isInvalid())
+return false;
+  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
+  if (ThisExpr.isInvalid())
+return false;
+  PlacementArgs.push_back(ThisExpr.get());
+}
+  }
+
+  for (auto *PD : FD.parameters()) {
+if (PD->getType()->isDependentType())
+  continue;
+
+// Build a reference to the parameter.
+auto PDLoc = PD->getLocation();
+ExprResult PDRefExpr =
+S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
+   ExprValueKind::VK_LValue, PDLoc);
+if (PDRefExpr.isInvalid())
+  return false;
+
+PlacementArgs.push_back(PDRefExpr.get());
+  }
+
+  return true;
+}
+
 bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
   // Form and check allocation and deallocation calls.
   assert(!IsPromiseDependentType &&
@@ -1255,13 +1290,7 @@
   // allocated, followed by the coroutine function's arguments. If a matching
   // allocation function exists, use it. Otherwise, use an allocation function
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9
   //   An implementation may need to allocate additional storage for a
   //   coroutine.
@@ -1288,31 +1317,12 @@
   // and p_i denotes the i-th function parameter otherwise. For a non-static
   // member function, q_1 is an lvalue that denotes *this; any other q_i is an
   // lvalue that denotes the parameter copy corresponding to p_i.
-  if (auto *MD = dyn_cast()) {
-if (MD->isInstance() && !isLambdaCallOperator(MD)) {
-  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
-  if (ThisExpr.isInvalid())
-return false;
-  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
-  if (ThisExpr.isInvalid())
-return false;
-  PlacementArgs.push_back(ThisExpr.get());
-}
-  }
-  for (auto *PD : FD.parameters()) {
-if (PD->getType()->isDependentType())
-  continue;
 
-// Build a reference to the parameter.
-auto PDLoc = PD->getLocation();
-ExprResult PDRefExpr =
-S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
-   ExprValueKind::VK_LValue, PDLoc);
-if (PDRefExpr.isInvalid())
-  return false;
-
-PlacementArgs.push_back(PDRefExpr.get());
-  }
+  FunctionDecl *OperatorNew = nullptr;
+  FunctionDecl *OperatorDelete = nullptr;
+  FunctionDecl *UnusedResult = nullptr;
+  bool PassAlignment = false;
+  SmallVector PlacementArgs;
 
   bool 

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> I think I have a preference to move it to CodeGenCXX anyway however, since 
> we're actually testing the code-generated output (this is not novel, we DO 
> often use CodeGen tests to make sure proper overloads/etc get called).

It makes sense. Done.

Thanks for reviewing!




Comment at: clang/docs/ReleaseNotes.rst:152
   `Issue 42372 `_.
 - Clang shouldn't lookup allocation function in global scope for coroutines
   in case it found the allocation function name in the promise_type body.

erichkeane wrote:
> I realize it isn't part of this patch, but this release note reads 
> awkwardly... How about:
> 
> 
> 
> > Clang will now find and emit a call to an allocation function in a 
> > promise_type body for coroutines.  Additionally, to implement CWG2585, a 
> > coroutine will no longer generate a call to a global allocation function 
> > with the signature (std::size_t, p0, ..., pn).
> > This fixes Issue `Issue 54881 
> > `_.
> 
> 
The suggested wording lacks a condition that we generate a call to allocation 
function in promise_type only if there is an allocation function name in the 
scope of promise_type. I try to add a condition based on your suggestions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126187/new/

https://reviews.llvm.org/D126187

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 431861.
ChuanqiXu added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126187/new/

https://reviews.llvm.org/D126187

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/CodeGenCoroutines/coro-alloc-2.cpp

Index: clang/test/CodeGenCoroutines/coro-alloc-2.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-alloc-2.cpp
@@ -0,0 +1,30 @@
+// Tests that we wouldn't generate an allocation call in global scope with (std::size_t, p0, ..., pn)
+// RUN: %clang_cc1 %s -std=c++20 -S -triple x86_64-unknown-linux-gnu -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
+#include "Inputs/coroutine.h"
+
+namespace std {
+typedef decltype(sizeof(int)) size_t;
+}
+
+struct Allocator {};
+
+struct resumable {
+  struct promise_type {
+
+resumable get_return_object() { return {}; }
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+void unhandled_exception() {}
+void return_void(){};
+  };
+};
+
+void *operator new(std::size_t, void *);
+
+resumable f1(void *) {
+  co_return;
+}
+
+// CHECK: coro.alloc:
+// CHECK-NEXT: [[SIZE:%.+]] = call [[BITWIDTH:.+]] @llvm.coro.size.[[BITWIDTH]]()
+// CHECK-NEXT: call {{.*}} ptr @_Znwm([[BITWIDTH]] noundef [[SIZE]])
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1239,6 +1239,41 @@
   return true;
 }
 
+// Collect placement arguments for allocation function of coroutine FD.
+// Return true if we collect placement arguments succesfully. Return false,
+// otherwise.
+static bool collectPlacementArgs(Sema , FunctionDecl , SourceLocation Loc,
+ SmallVectorImpl ) {
+  if (auto *MD = dyn_cast()) {
+if (MD->isInstance() && !isLambdaCallOperator(MD)) {
+  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
+  if (ThisExpr.isInvalid())
+return false;
+  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
+  if (ThisExpr.isInvalid())
+return false;
+  PlacementArgs.push_back(ThisExpr.get());
+}
+  }
+
+  for (auto *PD : FD.parameters()) {
+if (PD->getType()->isDependentType())
+  continue;
+
+// Build a reference to the parameter.
+auto PDLoc = PD->getLocation();
+ExprResult PDRefExpr =
+S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
+   ExprValueKind::VK_LValue, PDLoc);
+if (PDRefExpr.isInvalid())
+  return false;
+
+PlacementArgs.push_back(PDRefExpr.get());
+  }
+
+  return true;
+}
+
 bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
   // Form and check allocation and deallocation calls.
   assert(!IsPromiseDependentType &&
@@ -1255,13 +1290,7 @@
   // allocated, followed by the coroutine function's arguments. If a matching
   // allocation function exists, use it. Otherwise, use an allocation function
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9
   //   An implementation may need to allocate additional storage for a
   //   coroutine.
@@ -1288,31 +1317,12 @@
   // and p_i denotes the i-th function parameter otherwise. For a non-static
   // member function, q_1 is an lvalue that denotes *this; any other q_i is an
   // lvalue that denotes the parameter copy corresponding to p_i.
-  if (auto *MD = dyn_cast()) {
-if (MD->isInstance() && !isLambdaCallOperator(MD)) {
-  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
-  if (ThisExpr.isInvalid())
-return false;
-  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
-  if (ThisExpr.isInvalid())
-return false;
-  PlacementArgs.push_back(ThisExpr.get());
-}
-  }
-  for (auto *PD : FD.parameters()) {
-if (PD->getType()->isDependentType())
-  continue;
 
-// Build a reference to the parameter.
-auto PDLoc = PD->getLocation();
-ExprResult PDRefExpr =
-S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
-   ExprValueKind::VK_LValue, PDLoc);
-if (PDRefExpr.isInvalid())
-  return false;
-
-PlacementArgs.push_back(PDRefExpr.get());
-  }
+  FunctionDecl *OperatorNew = nullptr;
+  FunctionDecl *OperatorDelete = nullptr;
+  FunctionDecl *UnusedResult = nullptr;
+  bool PassAlignment = false;
+  SmallVector PlacementArgs;
 
   bool PromiseContainNew = [this, ]() -> bool {
 DeclarationName NewName =
@@ -1330,8 +1340,10 @@
 //   The allocation function's name is looked up by searching for it in the
 // scope of the promise 

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

I have a change to the release note that I'd like to see improved in SOME way, 
but I trust you to correct/reword as you wish.  I'm still not particularly 
happy with the mechanism of the test, but I cannot come up with a way to cause 
the Semantic Analyzer to cause this.

I think I have a preference to move it to CodeGenCXX anyway however, since 
we're actually testing the code-generated output (this is not novel, we DO 
often use CodeGen tests to make sure proper overloads/etc get called).




Comment at: clang/docs/ReleaseNotes.rst:152
   `Issue 42372 `_.
 - Clang shouldn't lookup allocation function in global scope for coroutines
   in case it found the allocation function name in the promise_type body.

I realize it isn't part of this patch, but this release note reads awkwardly... 
How about:



> Clang will now find and emit a call to an allocation function in a 
> promise_type body for coroutines.  Additionally, to implement CWG2585, a 
> coroutine will no longer generate a call to a global allocation function with 
> the signature (std::size_t, p0, ..., pn).
> This fixes Issue `Issue 54881 
> `_.





Comment at: clang/lib/Sema/SemaCoroutine.cpp:1293
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9

ChuanqiXu wrote:
> erichkeane wrote:
> > Extra comment line.
> Oh, this is intended. I feel the format looks better with the blank line.
Ah, the deletes made it not clear that this continued into the comment on 1294, 
so I thought this was a blank before code.  Thanks for the clarification.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126187/new/

https://reviews.llvm.org/D126187

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1293
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9

erichkeane wrote:
> Extra comment line.
Oh, this is intended. I feel the format looks better with the blank line.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1355
+  // We don't expect to call to global operator new with (size, p0, …, pn).
+  if (PromiseContainNew && !collectPlacementArgs(S, FD, Loc, PlacementArgs))
+return false;

erichkeane wrote:
> Can you explain how this works?  I'm not seeing what part of the collection 
> of placement args would prohibit the call you're talking about.
The new version of CWG issue 2585 says: "We shouldn't generate an allocation 
call in global scope with (std::size_t, p0, ..., pn)". Also it says that we 
wouldn't lookup into global scope if we find `operator new` in promise_type. It 
implies that we should only generate an allocation call in promise_type scope   
with (std::size_t, p0, ..., pn). So we should only collect placement arguments 
if we saw `operator new` in promise_type scope.

In other words, if we don't add the check, it would collect placement arguments 
all the way so it would be possible to generate an allocation call in global 
scope with (std::size_t, p0, ..., pn), which violates the update of CWG issue 
2585.



Comment at: clang/test/SemaCXX/coroutine-allocs2.cpp:2
+// Tests that we wouldn't generate an allocation call in global scope with 
(std::size_t, p0, ..., pn)
+// Although this test generates codes, it aims to test the semantics. So it is 
put here.
+// RUN: %clang_cc1 %s -std=c++20 -S -triple x86_64-unknown-linux-gnu 
-emit-llvm -disable-llvm-passes %s -o - | FileCheck %s

erichkeane wrote:
> I'm not a fan at all of having a FIleCheck-type-test in Sema, and this DOES 
> validate semantics.  Is there any way to get this to emit an error instead?  
> Perhaps declare the generated operator-new as 'deleted' and show that it 
> chooses THAT one instead by an error?
I tried to mark the allocation function as delete. But it doesn't work... And 
FileCheck is the only way I imaged. Another possible way might be to use 
FileCheck for dumped AST. But I feel it is worse since the ABI is more stable 
than the AST. (The format of AST is not guaranteed stable I thought.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126187/new/

https://reviews.llvm.org/D126187

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 431558.
ChuanqiXu added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126187/new/

https://reviews.llvm.org/D126187

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-allocs2.cpp

Index: clang/test/SemaCXX/coroutine-allocs2.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/coroutine-allocs2.cpp
@@ -0,0 +1,31 @@
+// Tests that we wouldn't generate an allocation call in global scope with (std::size_t, p0, ..., pn)
+// Although this test generates code, it aims to test the semantics. So it is put here.
+// RUN: %clang_cc1 %s -std=c++20 -S -triple x86_64-unknown-linux-gnu -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef decltype(sizeof(int)) size_t;
+}
+
+struct Allocator {};
+
+struct resumable {
+  struct promise_type {
+
+resumable get_return_object() { return {}; }
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+void unhandled_exception() {}
+void return_void(){};
+  };
+};
+
+void *operator new(std::size_t, void *);
+
+resumable f1(void *) {
+  co_return;
+}
+
+// CHECK: coro.alloc:
+// CHECK-NEXT: [[SIZE:%.+]] = call [[BITWIDTH:.+]] @llvm.coro.size.[[BITWIDTH]]()
+// CHECK-NEXT: call {{.*}} ptr @_Znwm([[BITWIDTH]] noundef [[SIZE]])
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1239,6 +1239,41 @@
   return true;
 }
 
+// Collect placement arguments for allocation function of coroutine FD.
+// Return true if we collect placement arguments succesfully. Return false,
+// otherwise.
+static bool collectPlacementArgs(Sema , FunctionDecl , SourceLocation Loc,
+ SmallVectorImpl ) {
+  if (auto *MD = dyn_cast()) {
+if (MD->isInstance() && !isLambdaCallOperator(MD)) {
+  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
+  if (ThisExpr.isInvalid())
+return false;
+  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
+  if (ThisExpr.isInvalid())
+return false;
+  PlacementArgs.push_back(ThisExpr.get());
+}
+  }
+
+  for (auto *PD : FD.parameters()) {
+if (PD->getType()->isDependentType())
+  continue;
+
+// Build a reference to the parameter.
+auto PDLoc = PD->getLocation();
+ExprResult PDRefExpr =
+S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
+   ExprValueKind::VK_LValue, PDLoc);
+if (PDRefExpr.isInvalid())
+  return false;
+
+PlacementArgs.push_back(PDRefExpr.get());
+  }
+
+  return true;
+}
+
 bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
   // Form and check allocation and deallocation calls.
   assert(!IsPromiseDependentType &&
@@ -1255,13 +1290,7 @@
   // allocated, followed by the coroutine function's arguments. If a matching
   // allocation function exists, use it. Otherwise, use an allocation function
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9
   //   An implementation may need to allocate additional storage for a
   //   coroutine.
@@ -1288,31 +1317,12 @@
   // and p_i denotes the i-th function parameter otherwise. For a non-static
   // member function, q_1 is an lvalue that denotes *this; any other q_i is an
   // lvalue that denotes the parameter copy corresponding to p_i.
-  if (auto *MD = dyn_cast()) {
-if (MD->isInstance() && !isLambdaCallOperator(MD)) {
-  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
-  if (ThisExpr.isInvalid())
-return false;
-  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
-  if (ThisExpr.isInvalid())
-return false;
-  PlacementArgs.push_back(ThisExpr.get());
-}
-  }
-  for (auto *PD : FD.parameters()) {
-if (PD->getType()->isDependentType())
-  continue;
 
-// Build a reference to the parameter.
-auto PDLoc = PD->getLocation();
-ExprResult PDRefExpr =
-S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
-   ExprValueKind::VK_LValue, PDLoc);
-if (PDRefExpr.isInvalid())
-  return false;
-
-PlacementArgs.push_back(PDRefExpr.get());
-  }
+  FunctionDecl *OperatorNew = nullptr;
+  FunctionDecl *OperatorDelete = nullptr;
+  FunctionDecl *UnusedResult = nullptr;
+  bool PassAlignment = false;
+  SmallVector PlacementArgs;
 
   bool PromiseContainNew = [this, ]() -> bool {
 DeclarationName NewName =
@@ -1330,8 +1340,10 @@
 //   The allocation function's 

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Also needs some release notes.




Comment at: clang/lib/Sema/SemaCoroutine.cpp:1293
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9

Extra comment line.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1355
+  // We don't expect to call to global operator new with (size, p0, …, pn).
+  if (PromiseContainNew && !collectPlacementArgs(S, FD, Loc, PlacementArgs))
+return false;

Can you explain how this works?  I'm not seeing what part of the collection of 
placement args would prohibit the call you're talking about.



Comment at: clang/test/SemaCXX/coroutine-allocs2.cpp:2
+// Tests that we wouldn't generate an allocation call in global scope with 
(std::size_t, p0, ..., pn)
+// Although this test generates codes, it aims to test the semantics. So it is 
put here.
+// RUN: %clang_cc1 %s -std=c++20 -S -triple x86_64-unknown-linux-gnu 
-emit-llvm -disable-llvm-passes %s -o - | FileCheck %s

I'm not a fan at all of having a FIleCheck-type-test in Sema, and this DOES 
validate semantics.  Is there any way to get this to emit an error instead?  
Perhaps declare the generated operator-new as 'deleted' and show that it 
chooses THAT one instead by an error?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126187/new/

https://reviews.llvm.org/D126187

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added a reviewer: erichkeane.
ChuanqiXu added a project: clang.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a subscriber: cfe-commits.

This is the  following update for the update in CWG issue 2585: 
https://cplusplus.github.io/CWG/issues/2585.html

The new updates in CWG issue 2585 say that we shouldn't lookup ::operator 
new(std::size_t, p0, ..., p1) in global scope.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126187

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-allocs2.cpp

Index: clang/test/SemaCXX/coroutine-allocs2.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/coroutine-allocs2.cpp
@@ -0,0 +1,31 @@
+// Tests that we wouldn't generate an allocation call in global scope with (std::size_t, p0, ..., pn)
+// Although this test generates codes, it aims to test the semantics. So it is put here.
+// RUN: %clang_cc1 %s -std=c++20 -S -triple x86_64-unknown-linux-gnu -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef decltype(sizeof(int)) size_t;
+}
+
+struct Allocator {};
+
+struct resumable {
+  struct promise_type {
+
+resumable get_return_object() { return {}; }
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+void unhandled_exception() {}
+void return_void(){};
+  };
+};
+
+void *operator new(std::size_t, void*);
+
+resumable f1(void *) {
+  co_return;
+}
+
+// CHECK: coro.alloc:
+// CHECK-NEXT: [[SIZE:%.+]] = call [[BITWIDTH:.+]] @llvm.coro.size.[[BITWIDTH]]()
+// CHECK-NEXT: call {{.*}} ptr @_Znwm([[BITWIDTH]] noundef [[SIZE]])
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1239,6 +1239,41 @@
   return true;
 }
 
+// Collect placement arguments for allocation function of coroutine FD.
+// Return true if we collect placement arguments succesfully. Return false,
+// otherwise.
+static bool collectPlacementArgs(Sema , FunctionDecl , SourceLocation Loc,
+ SmallVectorImpl ) {
+  if (auto *MD = dyn_cast()) {
+if (MD->isInstance() && !isLambdaCallOperator(MD)) {
+  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
+  if (ThisExpr.isInvalid())
+return false;
+  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
+  if (ThisExpr.isInvalid())
+return false;
+  PlacementArgs.push_back(ThisExpr.get());
+}
+  }
+
+  for (auto *PD : FD.parameters()) {
+if (PD->getType()->isDependentType())
+  continue;
+
+// Build a reference to the parameter.
+auto PDLoc = PD->getLocation();
+ExprResult PDRefExpr =
+S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
+   ExprValueKind::VK_LValue, PDLoc);
+if (PDRefExpr.isInvalid())
+  return false;
+
+PlacementArgs.push_back(PDRefExpr.get());
+  }
+
+  return true;
+}
+
 bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
   // Form and check allocation and deallocation calls.
   assert(!IsPromiseDependentType &&
@@ -1255,13 +1290,7 @@
   // allocated, followed by the coroutine function's arguments. If a matching
   // allocation function exists, use it. Otherwise, use an allocation function
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9
   //   An implementation may need to allocate additional storage for a
   //   coroutine.
@@ -1288,31 +1317,12 @@
   // and p_i denotes the i-th function parameter otherwise. For a non-static
   // member function, q_1 is an lvalue that denotes *this; any other q_i is an
   // lvalue that denotes the parameter copy corresponding to p_i.
-  if (auto *MD = dyn_cast()) {
-if (MD->isInstance() && !isLambdaCallOperator(MD)) {
-  ExprResult ThisExpr = S.ActOnCXXThis(Loc);
-  if (ThisExpr.isInvalid())
-return false;
-  ThisExpr = S.CreateBuiltinUnaryOp(Loc, UO_Deref, ThisExpr.get());
-  if (ThisExpr.isInvalid())
-return false;
-  PlacementArgs.push_back(ThisExpr.get());
-}
-  }
-  for (auto *PD : FD.parameters()) {
-if (PD->getType()->isDependentType())
-  continue;
 
-// Build a reference to the parameter.
-auto PDLoc = PD->getLocation();
-ExprResult PDRefExpr =
-S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
-   ExprValueKind::VK_LValue, PDLoc);
-if (PDRefExpr.isInvalid())
-  return false;
-
-PlacementArgs.push_back(PDRefExpr.get());
-  }
+  FunctionDecl