[coroutines] Fix building of new/delete expressions when get_return_object_on_allocation_failure() is present.

[EricWF]

Summary

This patch implements [dcl.fct.def.coroutine]p8:

The unqualified-id get_return_object_on_allocation_failure is looked up in the scope of
class P by class member access lookup (3.4.5). If a declaration is found, ..., and if a
global allocation function is selected, the ::operator new(size_t, nothrow_t) form shall be used.
[...]
The allocation function used in this case must have a non-throwing noexcept-specification.

[EricWF]

• general cleanup.

[EricWF]

• Remove incorrectly added changes.

Note that this patch is built on top of D31487 and not trunk.

[EricWF]

• Actually remove incorrectly added changes.

[GorNishanov]

LGTM

[EricWF]

• Merge with master
• Fix coro-alloc.cpp test so that it declares new.
• Add FIXME comment regarding assertion about an MaybeODRUsedExpr that hasn't been correctly handled. (the VarDecl for std::nothrow is broken).

[EricWF]

This patch currently triggers an assertion in SemaDecl.cpp:12290 assert(MaybeODRUseExprs.empty() && "Leftover expressions for odr-use checking");

I'll have to come up with a fix for that before this is committed. Unfortunately the easy fix of calling MakeVarDeclReferenced or similar doesn't work. I suspect this is due to the current active scope when ActOnFinishFunctionBody is called.

[EricWF]

• Add call to MarkDeclRefReferenced even though it doesn't prevent the assertion.

[EricWF]

lib/Sema/SemaCoroutine.cpp

680

@rsmith is this the correct way to build a DeclRefExpr to an existing global?

[GorNishanov]

lib/Sema/CoroutineBuilder.h

1 ↗

(On Diff #95074)

This file is called:

CoroutineStmtBuilder.h

in trunk

14 ↗

(On Diff #95074)

#ifndef LLVM_CLANG_LIB_SEMA_COROUTINESTMTBUILDER_H
#define LLVM_CLANG_LIB_SEMA_COROUTINESTMTBUILDER_H

[EricWF]

lib/Sema/CoroutineBuilder.h

1 ↗

(On Diff #95074)

Woops, this file should have been deleted.

[EricWF]

• Fix MaybeODRUseExprs assertion.
• Remove mistakenly present CoroutineBuilder.h file.

I'll land this later today if there are no objections.

[EricWF]

Re-opening since the last commit had to be reverted.

[EricWF]

• Update so it merges against master.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_coroutine_invalid_func_context : Error<
   "'%1' cannot be used in %select{a constructor|a destructor"
   "|a copy assignment operator|a move assignment operator|the 'main' function"
   "|a constexpr function|a function with a deduced return type"
   "|a varargs function}0">;
 def err_implied_coroutine_type_not_found : Error<
   "%0 type was not found; include <experimental/coroutine> before defining "
   "a coroutine">;
+def err_implicit_coroutine_std_nothrow_type_not_found : Error<
+  "std::nothrow was not found; include <new> before defining a coroutine which "
+  "uses get_return_object_on_allocation_failure()">;
+def err_malformed_std_nothrow : Error<
+  "std::nothrow must be a valid variable declaration">;
 def err_malformed_std_coroutine_handle : Error<
   "std::experimental::coroutine_handle must be a class template">;
 def err_coroutine_handle_missing_member : Error<
   "std::experimental::coroutine_handle missing a member named '%0'">;
 def err_malformed_std_coroutine_traits : Error<
   "'std::experimental::coroutine_traits' must be a class template">;
 def err_implied_std_coroutine_traits_promise_type_not_found : Error<
   "this function cannot be a coroutine: %q0 has no member named 'promise_type'">;
 def err_implied_std_coroutine_traits_promise_type_not_class : Error<
   "this function cannot be a coroutine: %0 is not a class">;
 def err_coroutine_promise_type_incomplete : Error<
   "this function cannot be a coroutine: %0 is an incomplete type">;
 def err_coroutine_type_missing_specialization : Error<
   "this function cannot be a coroutine: missing definition of "
   "specialization %q0">;
 def err_coroutine_promise_return_ill_formed : Error<
   "%0 declares both 'return_value' and 'return_void'">;
 def note_coroutine_promise_implicit_await_transform_required_here : Note<
   "call to 'await_transform' implicitly required by 'co_await' here">;
-def note_coroutine_promise_call_implicitly_required : Note<
+def note_coroutine_promise_suspend_implicitly_required : Note<
   "call to '%select{initial_suspend|final_suspend}0' implicitly "
   "required by the %select{initial suspend point|final suspend point}0">;
 def err_coroutine_promise_unhandled_exception_required : Error<
   "%0 is required to declare the member 'unhandled_exception()'">;
 def warn_coroutine_promise_unhandled_exception_required_with_exceptions : Warning<
   "%0 is required to declare the member 'unhandled_exception()' when exceptions are enabled">,
   InGroup<CoroutineMissingUnhandledException>;
 def err_coroutine_promise_get_return_object_on_allocation_failure : Error<
   "%0: 'get_return_object_on_allocation_failure()' must be a static member function">;
+def err_coroutine_promise_new_requires_nothrow : Error<
+  "%0 is required to have a non-throwing noexcept specification when the promise "
+   "type declares 'get_return_object_on_allocation_failure()'">;
+def note_coroutine_promise_call_implicitly_required : Note<
+  "call to %0 implicitly required by coroutine function here">;
 }
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Instrumentation Issue" in {

lib/Sema/SemaCoroutine.cpp

if (Suspend.isInvalid())
       return StmtError();
     Suspend = buildOperatorCoawaitCall(S, SC, Loc, Suspend.get());
     if (Suspend.isInvalid())
       return StmtError();
     Suspend = S.BuildResolvedCoawaitExpr(Loc, Suspend.get(),
                                          /*IsImplicit*/ true);
     Suspend = S.ActOnFinishFullExpr(Suspend.get());
     if (Suspend.isInvalid()) {
-      S.Diag(Loc, diag::note_coroutine_promise_call_implicitly_required)
+      S.Diag(Loc, diag::note_coroutine_promise_suspend_implicitly_required)
           << ((Name == "initial_suspend") ? 0 : 1);
       S.Diag(KWLoc, diag::note_declared_coroutine_here) << Keyword;
       return StmtError();
     }
     return cast<Stmt>(Suspend.get());
   };
 
   StmtResult InitSuspend = buildSuspends("initial_suspend");
if (PC.isInvalid())
     return StmtError();
 
   Expr *PCE = ActOnFinishFullExpr(PC.get()).get();
 
   Stmt *Res = new (Context) CoreturnStmt(Loc, E, PCE, IsImplicit);
   return Res;
 }
 
+/// Look up the std::nothrow object.
+static Expr *buildStdNoThrowDeclRef(Sema &S, SourceLocation Loc) {
+  NamespaceDecl *Std = S.getStdNamespace();
+  assert(Std && "Should already be diagnosed");
+
+  LookupResult Result(S, &S.PP.getIdentifierTable().get("nothrow"), Loc,
+                      Sema::LookupOrdinaryName);
+  if (!S.LookupQualifiedName(Result, Std)) {
+    // FIXME: <experimental/coroutine> should have been included already.
+    // If we require it to include <new> then this diagnostic is no longer
+    // needed.
+    S.Diag(Loc, diag::err_implicit_coroutine_std_nothrow_type_not_found);
+    return nullptr;
+  }
+
+  // FIXME: Mark the variable as ODR used. This currently does not work
+  // likely due to the scope at in which this function is called.
+  auto *VD = Result.getAsSingle<VarDecl>();
+  if (!VD) {
+    Result.suppressDiagnostics();
+    // We found something weird. Complain about the first thing we found.
+    NamedDecl *Found = *Result.begin();
+    S.Diag(Found->getLocation(), diag::err_malformed_std_nothrow);
+    return nullptr;
+  }
+
+  ExprResult DR = S.BuildDeclRefExpr(VD, VD->getType(), VK_LValue, Loc);
+  if (DR.isInvalid())
+    return nullptr;
+
+  return DR.get();
+}
+
 // Find an appropriate delete for the promise.
 static FunctionDecl *findDeleteForPromise(Sema &S, SourceLocation Loc,
                                           QualType PromiseType) {
   FunctionDecl *OperatorDelete = nullptr;
 
   DeclarationName DeleteName =
       S.Context.DeclarationNames.getCXXOperatorName(OO_Delete);
 
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;
 
-  // FIXME: Add nothrow_t placement arg for global alloc
-  //        if ReturnStmtOnAllocFailure != nullptr.
+  const bool RequiresNoThrowAlloc = ReturnStmtOnAllocFailure != nullptr;
+
   // FIXME: Add support for stateful allocators.
 
   FunctionDecl *OperatorNew = nullptr;
   FunctionDecl *OperatorDelete = nullptr;
   FunctionDecl *UnusedResult = nullptr;
   bool PassAlignment = false;
+  MultiExprArg PlacementArgs = None;
 
   S.FindAllocationFunctions(Loc, SourceRange(),
                             /*UseGlobal*/ false, PromiseType,
-                            /*isArray*/ false, PassAlignment,
-                            /*PlacementArgs*/ None, OperatorNew, UnusedResult);
+                            /*isArray*/ false, PassAlignment, PlacementArgs,
+                            OperatorNew, UnusedResult);
 
-  OperatorDelete = findDeleteForPromise(S, Loc, PromiseType);
+  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
+  // instead.
+  if (RequiresNoThrowAlloc && (!OperatorNew || IsGlobalOverload)) {
+    auto *StdNoThrow = buildStdNoThrowDeclRef(S, Loc);
+    if (!StdNoThrow)
+      return false;
+    PlacementArgs = MultiExprArg(StdNoThrow);
+    OperatorNew = nullptr;
+    S.FindAllocationFunctions(Loc, SourceRange(),
+                              /*UseGlobal*/ true, PromiseType,
+                              /*isArray*/ false, PassAlignment, PlacementArgs,
+                              OperatorNew, UnusedResult);
+  }
 
-  if (!OperatorDelete || !OperatorNew)
+  if (OperatorNew && RequiresNoThrowAlloc) {
+    const auto *FT = OperatorNew->getType()->getAs<FunctionProtoType>();
+    if (!FT->isNothrow(S.Context, /*ResultIfDependent*/ false)) {
+      S.Diag(OperatorNew->getLocation(),
+             diag::err_coroutine_promise_new_requires_nothrow)
+          << OperatorNew;
+      S.Diag(Loc, diag::note_coroutine_promise_call_implicitly_required)
+          << OperatorNew;
+      return false;
+    }
+  }
+
+  if ((OperatorDelete = findDeleteForPromise(S, Loc, PromiseType)) == nullptr)
     return false;
 
   Expr *FramePtr =
       buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_frame, {});
 
   Expr *FrameSize =
       buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_size, {});
 
   // Make new call.
 
   ExprResult NewRef =
       S.BuildDeclRefExpr(OperatorNew, OperatorNew->getType(), VK_LValue, Loc);
   if (NewRef.isInvalid())
     return false;
 
+  SmallVector<Expr *, 2> NewArgs{FrameSize};
+  for (auto Arg : PlacementArgs)
+    NewArgs.push_back(Arg);
+
   ExprResult NewExpr =
-      S.ActOnCallExpr(S.getCurScope(), NewRef.get(), Loc, FrameSize, Loc);
+      S.ActOnCallExpr(S.getCurScope(), NewRef.get(), Loc, NewArgs, Loc);
+  NewExpr = S.ActOnFinishFullExpr(NewExpr.get());
   if (NewExpr.isInvalid())
     return false;
 
   // Make delete call.
 
   QualType OpDeleteQualType = OperatorDelete->getType();
 
   ExprResult DeleteRef =
bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
   // Check if we need to pass the size.
   const auto *OpDeleteType =
       OpDeleteQualType.getTypePtr()->getAs<FunctionProtoType>();
   if (OpDeleteType->getNumParams() > 1)
     DeleteArgs.push_back(FrameSize);
 
   ExprResult DeleteExpr =
       S.ActOnCallExpr(S.getCurScope(), DeleteRef.get(), Loc, DeleteArgs, Loc);
+  DeleteExpr = S.ActOnFinishFullExpr(DeleteExpr.get());
   if (DeleteExpr.isInvalid())
     return false;
 
   this->Allocate = NewExpr.get();
   this->Deallocate = DeleteExpr.get();
 
   return true;
 }

test/CodeGenCoroutines/coro-alloc.cpp

 template <>
 struct coroutine_handle<void> {
   static coroutine_handle from_address(void *) { return {}; }
   coroutine_handle() = default;
   template <class PromiseType>
   coroutine_handle(coroutine_handle<PromiseType>) {}
 };
 
-}
-}
+} // end namespace experimental
+
+struct nothrow_t {};
+constexpr nothrow_t nothrow = {};
+
+} // end namespace std
+
+// Required when get_return_object_on_allocation_failure() is defined by
+// the promise.
+using SizeT = decltype(sizeof(int));
+void* operator new(SizeT __sz, const std::nothrow_t&) noexcept;
+void  operator delete(void* __p, const std::nothrow_t&) noexcept;
+
 
 struct suspend_always {
   bool await_ready() { return false; }
   void await_suspend(std::experimental::coroutine_handle<>) {}
   void await_resume() {}
 };
 
 struct global_new_delete_tag {};
struct promise_type {
     static int get_return_object_on_allocation_failure() { return -1; }
   };
 };
 
 // CHECK-LABEL: f4(
 extern "C" int f4(promise_on_alloc_failure_tag) {
   // CHECK: %[[ID:.+]] = call token @llvm.coro.id(i32 16
   // CHECK: %[[SIZE:.+]] = call i64 @llvm.coro.size.i64()
-  // CHECK: %[[MEM:.+]] = call i8* @_Znwm(i64 %[[SIZE]])
+  // CHECK: %[[MEM:.+]] = call i8* @_ZnwmRKSt9nothrow_t(i64 %[[SIZE]], %"struct.std::nothrow_t"* dereferenceable(1) @_ZStL7nothrow)
   // CHECK: %[[OK:.+]] = icmp ne i8* %[[MEM]], null
   // CHECK: br i1 %[[OK]], label %[[OKBB:.+]], label %[[ERRBB:.+]]
 
   // CHECK: [[ERRBB]]:
   // CHECK: %[[RETVAL:.+]] = call i32 @_ZNSt12experimental16coroutine_traitsIJi28promise_on_alloc_failure_tagEE12promise_type39get_return_object_on_allocation_failureEv(
   // CHECK: ret i32 %[[RETVAL]]
   co_return;
 }

test/SemaCXX/coroutines.cpp

 template<> struct std::experimental::coroutine_traits<float>
 { using promise_type = good_promise_2; };
 
 float badly_specialized_coro_handle() { // expected-error {{std::experimental::coroutine_handle missing a member named 'from_address'}}
   //expected-note@-1 {{call to 'initial_suspend' implicitly required by the initial suspend point}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
+namespace std {
+  struct nothrow_t {};
+  constexpr nothrow_t nothrow = {};
+}
+
+using SizeT = decltype(sizeof(int));
+
+void* operator new(SizeT __sz, const std::nothrow_t&) noexcept;
+void  operator delete(void* __p, const std::nothrow_t&) noexcept;
+
+
+
 struct promise_on_alloc_failure_tag {};
 
 template<>
 struct std::experimental::coroutine_traits<int, promise_on_alloc_failure_tag> {
   struct promise_type {
     int get_return_object() {}
     suspend_always initial_suspend() { return {}; }
     suspend_always final_suspend() { return {}; }

 
 template <class T>
 coro<T> dependent_private_alloc_failure_handler(T) {
   // expected-error@-1 {{'get_return_object_on_allocation_failure' is a private member of 'bad_promise_11'}}
   co_return; // FIXME: Add a "declared coroutine here" note.
 }
 template coro<bad_promise_11> dependent_private_alloc_failure_handler(bad_promise_11);
 // expected-note@-1 {{requested here}}
+
+struct bad_promise_12 {
+  coro<bad_promise_12> get_return_object();
+  suspend_always initial_suspend();
+  suspend_always final_suspend();
+  void unhandled_exception();
+  void return_void();
+  static coro<bad_promise_12> get_return_object_on_allocation_failure();
+
+  static void* operator new(SizeT);
+  // expected-error@-1 2 {{'operator new' is required to have a non-throwing noexcept specification when the promise type declares 'get_return_object_on_allocation_failure()'}}
+};
+coro<bad_promise_12> throwing_in_class_new() { // expected-note {{call to 'operator new' implicitly required by coroutine function here}}
+  co_return;
+}
+
+template <class T>
+coro<T> dependent_throwing_in_class_new(T) { // expected-note {{call to 'operator new' implicitly required by coroutine function here}}
+   co_return;
+}
+template coro<bad_promise_12> dependent_throwing_in_class_new(bad_promise_12); // expected-note {{requested here}}
+
+
+struct good_promise_13 {
+  coro<good_promise_13> get_return_object();
+  suspend_always initial_suspend();
+  suspend_always final_suspend();
+  void unhandled_exception();
+  void return_void();
+  static coro<good_promise_13> get_return_object_on_allocation_failure();
+};
+coro<good_promise_13> uses_nothrow_new() {
+  co_return;
+}
+
+template <class T>
+coro<T> dependent_uses_nothrow_new(T) {
+   co_return;
+}
+template coro<good_promise_13> dependent_uses_nothrow_new(good_promise_13);