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

[ChuanqiXu]

Summary

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.

[erichkeane]

Also needs some release notes.

clang/lib/Sema/SemaCoroutine.cpp

1293

Extra comment line.

1355

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.

clang/test/SemaCXX/coroutine-allocs2.cpp

2 ↗

(On Diff #431303)

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?

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/lib/Sema/SemaCoroutine.cpp

1293

Oh, this is intended. I feel the format looks better with the blank line.

1355

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.

clang/test/SemaCXX/coroutine-allocs2.cpp

2 ↗

(On Diff #431303)

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.)

[erichkeane]

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).

clang/docs/ReleaseNotes.rst

151–152

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 <https://github.com/llvm/llvm-project/issues/54881>_.

clang/lib/Sema/SemaCoroutine.cpp

1293

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.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

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!

clang/docs/ReleaseNotes.rst

151–152

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.

clang/docs/ReleaseNotes.rst

- Fixed a crash when the ``__bf16`` type is used such that its size or
   fixes `Issue 50171 <https://github.com/llvm/llvm-project/issues/50171>`_.
 - Fixed a false positive diagnostic about an unevaluated expression having no
   side effects when the expression is of VLA type and is an operand of the
   ``sizeof`` operator. Fixes `Issue 48010 <https://github.com/llvm/llvm-project/issues/48010>`_.
 - Fixed a false positive diagnostic about scoped enumerations being a C++11
   extension in C mode. A scoped enumeration's enumerators cannot be named in C
   because there is no way to fully qualify the enumerator name, so this
   "extension" was unintentional and useless. This fixes
   `Issue 42372 <https://github.com/llvm/llvm-project/issues/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.
+- Clang will now find and emit a call to an allocation function in a 
+  promise_type body for coroutines if there is any allocation function 
+  declaration in the scope of promise_type. 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 <https://github.com/llvm/llvm-project/issues/54881>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the

clang/lib/Sema/SemaCoroutine.cpp

S.Diag(Fn.FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
         << Fn.getFirstCoroutineStmtKeyword();
     return false;
   }
 
   this->ReturnStmtOnAllocFailure = ReturnStmt.get();
   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 &S, FunctionDecl &FD, SourceLocation Loc,
+                                 SmallVectorImpl<Expr *> &PlacementArgs) {
+  if (auto *MD = dyn_cast<CXXMethodDecl>(&FD)) {
+    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 &&
          "cannot make statement while the promise type is dependent");
   QualType PromiseType = Fn.CoroutinePromise->getType();
 
   if (S.RequireCompleteType(Loc, PromiseType, diag::err_incomplete_type))
     return false;
 
   const bool RequiresNoThrowAlloc = ReturnStmtOnAllocFailure != nullptr;
 
   // According to [dcl.fct.def.coroutine]p9, Lookup allocation functions using a
   // parameter list composed of the requested size of the coroutine state being
   // 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<Expr *, 1> PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9
   //   An implementation may need to allocate additional storage for a
   //   coroutine.
   // This storage is known as the coroutine state and is obtained by calling a
   // non-array allocation function ([basic.stc.dynamic.allocation]). The
   // allocation function's name is looked up by searching for it in the scope of
   // the promise type.
   // - If any declarations are found, overload resolution is performed on a
bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
   // sequence of types of the non-object function parameters, preceded by the
   // type of the object parameter ([dcl.fct]) if the coroutine is a non-static
   // member function. [dcl.fct.def.coroutine]p4 In the following, p_i is an
   // lvalue of type P_i, where p1 denotes the object parameter and p_i+1 denotes
   // the i-th non-object function parameter for a non-static member function,
   // 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<CXXMethodDecl>(&FD)) {
-    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<Expr *, 1> PlacementArgs;
 
   bool PromiseContainNew = [this, &PromiseType]() -> bool {
     DeclarationName NewName =
         S.getASTContext().DeclarationNames.getCXXOperatorName(OO_New);
     LookupResult R(S, NewName, Loc, Sema::LookupOrdinaryName);
 
     if (PromiseType->isRecordType())
       S.LookupQualifiedName(R, PromiseType->getAsCXXRecordDecl());
 
     return !R.empty() && !R.isAmbiguous();
   }();
 
   auto LookupAllocationFunction = [&]() {
     // [dcl.fct.def.coroutine]p9
     //   The allocation function's name is looked up by searching for it in the
     // scope of the promise type.
     // - If any declarations are found, ...
-    // - Otherwise, a search is performed in the global scope.
-    Sema::AllocationFunctionScope NewScope = PromiseContainNew ? Sema::AFS_Class : Sema::AFS_Global;
+    // - If no declarations are found in the scope of the promise type, a search
+    // is performed in the global scope.
+    Sema::AllocationFunctionScope NewScope =
+        PromiseContainNew ? Sema::AFS_Class : Sema::AFS_Global;
     S.FindAllocationFunctions(Loc, SourceRange(),
                               NewScope,
                               /*DeleteScope*/ Sema::AFS_Both, PromiseType,
                               /*isArray*/ false, PassAlignment, PlacementArgs,
                               OperatorNew, UnusedResult, /*Diagnose*/ false);
   };
 
+  // We don't expect to call to global operator new with (size, p0, …, pn).
+  // So if we choose to lookup the allocation function in global scope, we
+  // shouldn't lookup placement arguments.
+  if (PromiseContainNew && !collectPlacementArgs(S, FD, Loc, PlacementArgs))
+    return false;
+
   LookupAllocationFunction();
 
   // [dcl.fct.def.coroutine]p9
   //   If no viable function is found ([over.match.viable]), overload resolution
   // is performed again on a function call created by passing just the amount of
   // space required as an argument of type std::size_t.
-  if (!OperatorNew && !PlacementArgs.empty()) {
+  if (!OperatorNew && !PlacementArgs.empty() && PromiseContainNew) {
     PlacementArgs.clear();
     LookupAllocationFunction();
   }
 
   bool IsGlobalOverload =
       OperatorNew && !isa<CXXRecordDecl>(OperatorNew->getDeclContext());
   // If we didn't find a class-local new declaration and non-throwing new
   // was is required then we need to lookup the non-throwing global operator

clang/test/CodeGenCoroutines/coro-alloc-2.cpp

+// 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]])