[coroutines] Fix rebuilding of implicit and dependent coroutine statements.

[EricWF]

Summary

Certain implicitly generated coroutine statements, such as the calls to 'return_value()' or return_void() or get_return_object_on_allocation_failure(), cannot be built until the promise type is no longer dependent. This means they are not built until after the coroutine body statement has been transformed.

This patch fixes an issue where these statements would never be built for coroutine templates.

It also fixes a small issue where diagnostics about get_return_object_on_allocation_failure() were incorrectly suppressed.

[EricWF]

• Remove unneeded asserts.

[GorNishanov]

lib/Sema/CoroutineBuilder.h

53 ↗

(On Diff #93441)

makeReturnObject is built as $promise.get_return_object() should it be put into buildDependentStatements?

[EricWF]

• Fix insane definition of hasDependentPromiseType()

[EricWF]

• Remove incorrect comments.

[EricWF]

lib/Sema/CoroutineBuilder.h

53 ↗

(On Diff #93441)

I don't think so because the statement is required, so we can build it right away and transform it later. The "dependent" statements are ones built different depending on the results of name lookup.

[GorNishanov]

lib/Sema/CoroutineBuilder.h

1 ↗

(On Diff #93510)

I would name the file to match the class it declares, i.e CoroutineStmtBuilder.h, since it is the only class defined in this header.

9 ↗

(On Diff #93510)

I do not think that this description matches the content of the file.

37 ↗

(On Diff #93510)

I would move the constructor and two build methods implementations into SemaCoroutine.cpp.

lib/Sema/TreeTransform.h

6908 ↗

(On Diff #93510)

if(Expr *OnFallthrough = ...) {
   ...getDerived().TransformStmt(OnFallthrough);

6915 ↗

(On Diff #93510)

if(Expr *OnException = ...) {
   ...getDerived().TransformStmt(OnException);

6922 ↗

(On Diff #93510)

if(Expr *OnAllocFailure = ...) {
   ...getDerived().TransformStmt(OnAllocFailure);

[GorNishanov]

LGTM with nits addressed

[EricWF]

• Address inline comments.

[EricWF]

I have to revert this commit due to test failures. I think I uploaded an incorrect patch or bad merge because the tests passed on my machine.

[EricWF]

• Upload new, hopefully correct, diff.

[EricWF]

re-opening because I had to revert the changes again. The tests are passing on my local machine but they're failing on the bots. I'm not exactly sure what's going on. Perhaps its UB caused by an uninitialized value? More investigation needed.

[EricWF]

• Update against trunk.

[EricWF]

Woops, I've been updating the wrong patch. These updates should have been for D31562.

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