[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<Coroutine>;
 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;
+  }
+
+  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);
   if (NewExpr.isInvalid())
     return false;
 
   // Make delete call.
 
   QualType OpDeleteQualType = OperatorDelete->getType();
 
   ExprResult DeleteRef =

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;
+
+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}}