[coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[GorNishanov]

Summary

Sema::CheckCompletedCoroutineBody was growing unwieldy with building all of the substatements. Also, constructors for CoroutineBodyStmt had way too many parameters.

Instead, CoroutineBodyStmt now defines CtorArgs structure with all of the required construction parameters.
CheckCompleteCoroutineBody delegates construction of individual substatements to short functions one per each substatement.

Also, added a drive-by fix of initializing CoroutinePromise to nullptr in ScopeInfo.h.
And addressed the FIXME that wanted to tail allocate extra room at the end of the CoroutineBodyStmt to hold parameter move expressions. (The comment was longer that the code that implemented tail allocation).

[GorNishanov]

@rsmith, Looking good?

[rsmith]

Generally looks good, but we have a better way of modeling types with a trailing variable-length array that you should use.

include/clang/AST/StmtCXX.h

299

Please use llvm::TrailingObjects to store the trailing variable-length SubStmts array.

lib/Sema/SemaCoroutine.cpp

714–722

Reindent comments.

[GorNishanov]

No changes. Merge with top of the tree (to simplify comparing with the updated version that is coming up in a second).

[GorNishanov]

Feedback implemented!

[GorNishanov]

Looks even better now!

[GorNishanov]

LGTM? Pretty please :)

[GorNishanov]

Gentle and melodic ping.

[EricWF]

This currently segfaults on my machine. Here is the full output of running SemaCXX/coroutines.cpp.
https://gist.github.com/EricWF/81dc332e21c3e5c6bdc024cda87b846f

I'm not sure exactly what's going on, but it should be fixed before committing.

[EricWF]

This LGTM after applying the fixes.

lib/Sema/SemaCoroutine.cpp

696

I figured out what's going on. PromiseRecordDecl doesn't get initialized to null when IsPromiseDependentType is false. Initializing PromiseRecordDecl fixes the problem.

[GorNishanov]

Initialized member variable to zero.

[GorNishanov]

Initialized PromiseRecordDecl to nullptr

[GorNishanov]

Preparing to land. Thank you very much for review!

lib/Sema/SemaCoroutine.cpp

696

Good catch!

include/clang/AST/StmtCXX.h

public:
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == MSDependentExistsStmtClass;
   }
 };
 
 /// \brief Represents the body of a coroutine. This wraps the normal function
 /// body and holds the additional semantic context required to set up and tear
 /// down the coroutine frame.
 class CoroutineBodyStmt : public Stmt {
   enum SubStmt {
     Body,          ///< The body of the coroutine.
     Promise,       ///< The promise statement.
     InitSuspend,   ///< The initial suspend statement, run before the body.
     FinalSuspend,  ///< The final suspend statement, run after the body.
     OnException,   ///< Handler for exceptions thrown in the body.
     OnFallthrough, ///< Handler for control flow falling off the body.
     Allocate,      ///< Coroutine frame memory allocation.
     Deallocate,    ///< Coroutine frame memory deallocation.
     ReturnValue,   ///< Return value for thunk function.
     FirstParamMove ///< First offset for move construction of parameter copies.
   };
+  unsigned NumParams;
   Stmt *SubStmts[SubStmt::FirstParamMove];
 
   friend class ASTStmtReader;
 public:
-  CoroutineBodyStmt(Stmt *Body, Stmt *Promise, Stmt *InitSuspend,
-                    Stmt *FinalSuspend, Stmt *OnException, Stmt *OnFallthrough,
-                    Expr *Allocate, Stmt *Deallocate,
-                    Expr *ReturnValue, ArrayRef<Expr *> ParamMoves)
-      : Stmt(CoroutineBodyStmtClass) {
-    SubStmts[CoroutineBodyStmt::Body] = Body;
-    SubStmts[CoroutineBodyStmt::Promise] = Promise;
-    SubStmts[CoroutineBodyStmt::InitSuspend] = InitSuspend;
-    SubStmts[CoroutineBodyStmt::FinalSuspend] = FinalSuspend;
-    SubStmts[CoroutineBodyStmt::OnException] = OnException;
-    SubStmts[CoroutineBodyStmt::OnFallthrough] = OnFallthrough;
-    SubStmts[CoroutineBodyStmt::Allocate] = Allocate;
-    SubStmts[CoroutineBodyStmt::Deallocate] = Deallocate;
-    SubStmts[CoroutineBodyStmt::ReturnValue] = ReturnValue;
-    // FIXME: Tail-allocate space for parameter move expressions and store them.
-    assert(ParamMoves.empty() && "not implemented yet");
-  }
+
+  struct CtorArgs {
+    Stmt *Body = nullptr;
+    Stmt *Promise = nullptr;
+    Expr *InitialSuspend = nullptr;
+    Expr *FinalSuspend = nullptr;
+    Stmt *OnException = nullptr;
+    Stmt *OnFallthrough = nullptr;
+    Expr *Allocate = nullptr;
+    Expr *Deallocate = nullptr;
+    Stmt *ReturnValue = nullptr;
+    ArrayRef<Stmt *> ParamMoves;
+  };
+
+private:
+
+  CoroutineBodyStmt(CtorArgs const& Args);  
+
+public:
+  static CoroutineBodyStmt *Create(const ASTContext &C, CtorArgs const &Args);
 
   /// \brief Retrieve the body of the coroutine as written. This will be either
   /// a CompoundStmt or a TryStmt.
   Stmt *getBody() const {
     return SubStmts[SubStmt::Body];
   }
 
   Stmt *getPromiseDeclStmt() const { return SubStmts[SubStmt::Promise]; }
public:
   }
 
   Expr *getAllocate() const { return cast<Expr>(SubStmts[SubStmt::Allocate]); }
   Stmt *getDeallocate() const { return SubStmts[SubStmt::Deallocate]; }
 
   Expr *getReturnValueInit() const {
     return cast<Expr>(SubStmts[SubStmt::ReturnValue]);
   }
+  ArrayRef<Stmt const *> getParamMoves() const {
+    return {SubStmts + SubStmt::FirstParamMove, NumParams};
+  }
 
   SourceLocation getLocStart() const LLVM_READONLY {
     return getBody()->getLocStart();
   }
   SourceLocation getLocEnd() const LLVM_READONLY {
     return getBody()->getLocEnd();
   }
 
   child_range children() {
-    return child_range(SubStmts, SubStmts + SubStmt::FirstParamMove);
+    return child_range(SubStmts,
+                       SubStmts + SubStmt::FirstParamMove + NumParams);
   }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == CoroutineBodyStmtClass;
   }
 };
 
 /// \brief Represents a 'co_return' statement in the C++ Coroutines TS.

include/clang/Sema/ScopeInfo.h

public:
   SmallVector<SwitchStmt*, 8> SwitchStack;
 
   /// \brief The list of return statements that occur within the function or
   /// block, if there is any chance of applying the named return value
   /// optimization, or if we need to infer a return type.
   SmallVector<ReturnStmt*, 4> Returns;
 
   /// \brief The promise object for this coroutine, if any.
-  VarDecl *CoroutinePromise;
+  VarDecl *CoroutinePromise = nullptr;
 
   /// \brief The list of coroutine control flow constructs (co_await, co_yield,
   /// co_return) that occur within the function or block. Empty if and only if
   /// this function or block is not (yet known to be) a coroutine.
   SmallVector<Stmt*, 4> CoroutineStmts;
 
   /// \brief The stack of currently active compound stamement scopes in the
   /// function.

lib/AST/StmtCXX.cpp

VarDecl *CXXForRangeStmt::getLoopVariable() {
   Decl *LV = cast<DeclStmt>(getLoopVarStmt())->getSingleDecl();
   assert(LV && "No loop variable in CXXForRangeStmt");
   return cast<VarDecl>(LV);
 }
 
 const VarDecl *CXXForRangeStmt::getLoopVariable() const {
   return const_cast<CXXForRangeStmt *>(this)->getLoopVariable();
 }
+
+CoroutineBodyStmt *CoroutineBodyStmt::Create(
+    const ASTContext &C, CoroutineBodyStmt::CtorArgs const& Args) {
+  std::size_t Size =
+      sizeof(CoroutineBodyStmt) + Args.ParamMoves.size() * sizeof(Stmt *);
+
+  void *Mem = C.Allocate(Size, alignof(CoroutineBodyStmt));
+  return new (Mem) CoroutineBodyStmt(Args);
+}
+
+CoroutineBodyStmt::CoroutineBodyStmt(CoroutineBodyStmt::CtorArgs const &Args)
+    : Stmt(CoroutineBodyStmtClass), NumParams(Args.ParamMoves.size()) {
+  SubStmts[CoroutineBodyStmt::Body] = Args.Body;
+  SubStmts[CoroutineBodyStmt::Promise] = Args.Promise;
+  SubStmts[CoroutineBodyStmt::InitSuspend] = Args.InitialSuspend;
+  SubStmts[CoroutineBodyStmt::FinalSuspend] = Args.FinalSuspend;
+  SubStmts[CoroutineBodyStmt::OnException] = Args.OnException;
+  SubStmts[CoroutineBodyStmt::OnFallthrough] = Args.OnFallthrough;
+  SubStmts[CoroutineBodyStmt::Allocate] = Args.Allocate;
+  SubStmts[CoroutineBodyStmt::Deallocate] = Args.Deallocate;
+  SubStmts[CoroutineBodyStmt::ReturnValue] = Args.ReturnValue;
+  std::copy(Args.ParamMoves.begin(), Args.ParamMoves.end(),
+            const_cast<Stmt **>(getParamMoves().data()));
+}
 No newline at end of file

lib/Sema/SemaCoroutine.cpp

static FunctionDecl *findDeleteForPromise(Sema &S, SourceLocation Loc,
   return OperatorDelete;
 }
 
 // Builds allocation and deallocation for the coroutine. Returns false on
 // failure.
 static bool buildAllocationAndDeallocation(Sema &S, SourceLocation Loc,
                                            FunctionScopeInfo *Fn,
                                            Expr *&Allocation,
-                                           Stmt *&Deallocation) {
+                                           Expr *&Deallocation) {
   TypeSourceInfo *TInfo = Fn->CoroutinePromise->getTypeSourceInfo();
   QualType PromiseType = TInfo->getType();
   if (PromiseType->isDependentType())
     return true;
 
   if (S.RequireCompleteType(Loc, PromiseType, diag::err_incomplete_type))
     return false;
 
static bool buildAllocationAndDeallocation(Sema &S, SourceLocation Loc,
   if (DeleteExpr.isInvalid())
     return false;
 
   Deallocation = DeleteExpr.get();
 
   return true;
 }
 
+namespace {
+class SubStmtBuilder : public CoroutineBodyStmt::CtorArgs {
+  Sema &S;
+  FunctionDecl &FD;
+  FunctionScopeInfo &Fn;
+  bool IsValid;
+  SourceLocation Loc;
+  QualType RetType;
+  SmallVector<Stmt *, 4> ParamMovesVector;
+  const bool IsPromiseDependentType;
+  CXXRecordDecl *PromiseRecordDecl;
+
+public:
+  SubStmtBuilder(Sema &S, FunctionDecl &FD, FunctionScopeInfo &Fn, Stmt *Body)
+      : S(S), FD(FD), Fn(Fn), Loc(FD.getLocation()),
+        IsPromiseDependentType(
+            !Fn.CoroutinePromise ||
+            Fn.CoroutinePromise->getType()->isDependentType()) {
+    this->Body = Body;
+    if (!IsPromiseDependentType) {
+      PromiseRecordDecl = Fn.CoroutinePromise->getType()->getAsCXXRecordDecl();
+      assert(PromiseRecordDecl && "Type should have already been checked");
+    }
+    this->IsValid = makePromiseStmt() && makeInitialSuspend() &&
+                    makeFinalSuspend() && makeOnException() &&
+                    makeOnFallthrough() && makeNewAndDeleteExpr() &&
+                    makeReturnObject() && makeParamMoves();
+  }
+
+  bool isInvalid() const { return !this->IsValid; }  
+
+  bool makePromiseStmt();
+  bool makeInitialSuspend();
+  bool makeFinalSuspend();
+  bool makeNewAndDeleteExpr();
+  bool makeOnFallthrough();
+  bool makeOnException();
+  bool makeReturnObject();
+  bool makeParamMoves();
+};
+}
+
 void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
   FunctionScopeInfo *Fn = getCurFunction();
   assert(Fn && !Fn->CoroutineStmts.empty() && "not a coroutine");
 
   // Coroutines [stmt.return]p1:
   //   A return statement shall not appear in a coroutine.
   if (Fn->FirstReturnLoc.isValid()) {
     Diag(Fn->FirstReturnLoc, diag::err_return_in_coroutine);
     auto *First = Fn->CoroutineStmts[0];
     Diag(First->getLocStart(), diag::note_declared_coroutine_here)
         << (isa<CoawaitExpr>(First) ? 0 :
             isa<CoyieldExpr>(First) ? 1 : 2);
   }
+  SubStmtBuilder Builder(*this, *FD, *Fn, Body);
+  if (Builder.isInvalid())
+    return FD->setInvalidDecl();
 
-  SourceLocation Loc = FD->getLocation();
+  // Build body for the coroutine wrapper statement.
+  Body = CoroutineBodyStmt::Create(Context, Builder);
+}
 
+bool SubStmtBuilder::makePromiseStmt() {
   // Form a declaration statement for the promise declaration, so that AST
   // visitors can more easily find it.
   StmtResult PromiseStmt =
-      ActOnDeclStmt(ConvertDeclToDeclGroup(Fn->CoroutinePromise), Loc, Loc);
+      S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(Fn.CoroutinePromise), Loc, Loc);
   if (PromiseStmt.isInvalid())
-    return FD->setInvalidDecl();
+    return false;
 
+  this->Promise = PromiseStmt.get();
+  return true;
+}
+
+bool SubStmtBuilder::makeInitialSuspend() {
   // Form and check implicit 'co_await p.initial_suspend();' statement.
   ExprResult InitialSuspend =
-      buildPromiseCall(*this, Fn, Loc, "initial_suspend", None);
+      buildPromiseCall(S, &Fn, Loc, "initial_suspend", None);
   // FIXME: Support operator co_await here.
   if (!InitialSuspend.isInvalid())
-    InitialSuspend = BuildCoawaitExpr(Loc, InitialSuspend.get());
-  InitialSuspend = ActOnFinishFullExpr(InitialSuspend.get());
+    InitialSuspend = S.BuildCoawaitExpr(Loc, InitialSuspend.get());
+  InitialSuspend = S.ActOnFinishFullExpr(InitialSuspend.get());
   if (InitialSuspend.isInvalid())
-    return FD->setInvalidDecl();
+    return false;
+
+  this->InitialSuspend = InitialSuspend.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeFinalSuspend() {
   // Form and check implicit 'co_await p.final_suspend();' statement.
   ExprResult FinalSuspend =
-      buildPromiseCall(*this, Fn, Loc, "final_suspend", None);
+      buildPromiseCall(S, &Fn, Loc, "final_suspend", None);
   // FIXME: Support operator co_await here.
   if (!FinalSuspend.isInvalid())
-    FinalSuspend = BuildCoawaitExpr(Loc, FinalSuspend.get());
-  FinalSuspend = ActOnFinishFullExpr(FinalSuspend.get());
+    FinalSuspend = S.BuildCoawaitExpr(Loc, FinalSuspend.get());
+  FinalSuspend = S.ActOnFinishFullExpr(FinalSuspend.get());
   if (FinalSuspend.isInvalid())
-    return FD->setInvalidDecl();
+    return false;
+
+  this->FinalSuspend = FinalSuspend.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeNewAndDeleteExpr() {
   // Form and check allocation and deallocation calls.
-  Expr *Allocation = nullptr;
-  Stmt *Deallocation = nullptr;
-  if (!buildAllocationAndDeallocation(*this, Loc, Fn, Allocation, Deallocation))
-    return FD->setInvalidDecl();
+  return buildAllocationAndDeallocation(S, Loc, &Fn, this->Allocate,
+                                        this->Deallocate);
+}
+
+bool SubStmtBuilder::makeOnFallthrough() {
+  if (!PromiseRecordDecl)
+    return true;
 
-  // control flowing off the end of the coroutine.
-  // Also try to form 'p.set_exception(std::current_exception());' to handle
-  // uncaught exceptions.
-  ExprResult SetException;
-  StmtResult Fallthrough;
-  if (Fn->CoroutinePromise &&
-      !Fn->CoroutinePromise->getType()->isDependentType()) {
-    CXXRecordDecl *RD = Fn->CoroutinePromise->getType()->getAsCXXRecordDecl();
-    assert(RD && "Type should have already been checked");
-    // [dcl.fct.def.coroutine]/4
-    // The unqualified-ids 'return_void' and 'return_value' are looked up in
-    // the scope of class P. If both are found, the program is ill-formed.
-    DeclarationName RVoidDN = PP.getIdentifierInfo("return_void");
-    LookupResult RVoidResult(*this, RVoidDN, Loc, Sema::LookupMemberName);
-    const bool HasRVoid = LookupQualifiedName(RVoidResult, RD);
-
-    DeclarationName RValueDN = PP.getIdentifierInfo("return_value");
-    LookupResult RValueResult(*this, RValueDN, Loc, Sema::LookupMemberName);
-    const bool HasRValue = LookupQualifiedName(RValueResult, RD);
+  DeclarationName RVoidDN = S.PP.getIdentifierInfo("return_void");
+  LookupResult RVoidResult(S, RVoidDN, Loc, Sema::LookupMemberName);
+  const bool HasRVoid = S.LookupQualifiedName(RVoidResult, PromiseRecordDecl);
+
+  DeclarationName RValueDN = S.PP.getIdentifierInfo("return_value");
+  LookupResult RValueResult(S, RValueDN, Loc, Sema::LookupMemberName);
+  const bool HasRValue = S.LookupQualifiedName(RValueResult, PromiseRecordDecl);
 
-    if (HasRVoid && HasRValue) {
-      // FIXME Improve this diagnostic
-      Diag(FD->getLocation(), diag::err_coroutine_promise_return_ill_formed)
-          << RD;
-      return FD->setInvalidDecl();
-    } else if (HasRVoid) {
-      // If the unqualified-id return_void is found, flowing off the end of a
-      // coroutine is equivalent to a co_return with no operand. Otherwise,
-      // flowing off the end of a coroutine results in undefined behavior.
-      Fallthrough = BuildCoreturnStmt(FD->getLocation(), nullptr);
-      Fallthrough = ActOnFinishFullStmt(Fallthrough.get());
-      if (Fallthrough.isInvalid())
-        return FD->setInvalidDecl();
-    }
+  StmtResult Fallthrough;
+    S.Diag(FD.getLocation(), diag::err_coroutine_promise_return_ill_formed)
+        << PromiseRecordDecl;
+    return false;
+    Fallthrough = S.BuildCoreturnStmt(FD.getLocation(), nullptr);
+    Fallthrough = S.ActOnFinishFullStmt(Fallthrough.get());
+      return false;
+  }
+
+  this->OnFallthrough = Fallthrough.get();
+  return true;
 
-    // [dcl.fct.def.coroutine]/3
-    // The unqualified-id set_exception is found in the scope of P by class
-    // member access lookup (3.4.5).
-    DeclarationName SetExDN = PP.getIdentifierInfo("set_exception");
-    LookupResult SetExResult(*this, SetExDN, Loc, Sema::LookupMemberName);
-    if (LookupQualifiedName(SetExResult, RD)) {
-      // Form the call 'p.set_exception(std::current_exception())'
-      SetException = buildStdCurrentExceptionCall(*this, Loc);
-      if (SetException.isInvalid())
-        return FD->setInvalidDecl();
-      Expr *E = SetException.get();
-      SetException = buildPromiseCall(*this, Fn, Loc, "set_exception", E);
-      SetException = ActOnFinishFullExpr(SetException.get(), Loc);
-      if (SetException.isInvalid())
-        return FD->setInvalidDecl();
-    }
-  }
+bool SubStmtBuilder::makeOnException() {
+// Try to form 'p.set_exception(std::current_exception());' to handle
+// uncaught exceptions.
+// TODO: Post WG21 Issaquah 2016 renamed set_exception to unhandled_exception
+// TODO: and dropped exception_ptr parameter. Make it so.
+
+  if (!PromiseRecordDecl)
+    return true;
+
+    // If exceptions are disabled, don't try to build OnException.
+  if (!S.getLangOpts().CXXExceptions)
+    return true;
+
+  ExprResult SetException;
+
+  DeclarationName SetExDN = S.PP.getIdentifierInfo("set_exception");
+  LookupResult SetExResult(S, SetExDN, Loc, Sema::LookupMemberName);
+  if (S.LookupQualifiedName(SetExResult, PromiseRecordDecl)) {
+    SetException = buildStdCurrentExceptionCall(S, Loc);
+      return false;
+    SetException = buildPromiseCall(S, &Fn, Loc, "set_exception", E);
+    SetException = S.ActOnFinishFullExpr(SetException.get(), Loc);
+      return false;
+
+  this->OnException = SetException.get();
+  return true;
 
+bool SubStmtBuilder::makeReturnObject() {
+
   // Build implicit 'p.get_return_object()' expression and form initialization
   // of return type from it.
   ExprResult ReturnObject =
-      buildPromiseCall(*this, Fn, Loc, "get_return_object", None);
+      buildPromiseCall(S, &Fn, Loc, "get_return_object", None);
   if (ReturnObject.isInvalid())
-    return FD->setInvalidDecl();
-  QualType RetType = FD->getReturnType();
+    return false;
+  QualType RetType = FD.getReturnType();
   if (!RetType->isDependentType()) {
     InitializedEntity Entity =
         InitializedEntity::InitializeResult(Loc, RetType, false);
-    ReturnObject = PerformMoveOrCopyInitialization(Entity, nullptr, RetType,
+    ReturnObject = S.PerformMoveOrCopyInitialization(Entity, nullptr, RetType,
                                                    ReturnObject.get());
     if (ReturnObject.isInvalid())
-      return FD->setInvalidDecl();
+      return false;
   }
-  ReturnObject = ActOnFinishFullExpr(ReturnObject.get(), Loc);
+  ReturnObject = S.ActOnFinishFullExpr(ReturnObject.get(), Loc);
   if (ReturnObject.isInvalid())
-    return FD->setInvalidDecl();
+    return false;
 
-  // FIXME: Perform move-initialization of parameters into frame-local copies.
-  SmallVector<Expr*, 16> ParamMoves;
+  this->ReturnValue = ReturnObject.get();
+  return true;
+}
 
-  // Build body for the coroutine wrapper statement.
-  Body = new (Context) CoroutineBodyStmt(
-      Body, PromiseStmt.get(), InitialSuspend.get(), FinalSuspend.get(),
-      SetException.get(), Fallthrough.get(), Allocation, Deallocation,
-      ReturnObject.get(), ParamMoves);
+bool SubStmtBuilder::makeParamMoves() {
+  // FIXME: Perform move-initialization of parameters into frame-local copies.
+  return true;
 }

test/SemaCXX/coroutines.cpp

-// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s -fcxx-exceptions
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include <experimental/coroutine>}}
   // expected-error@-1 {{use of undeclared identifier 'a'}}
 }
 
 void no_coroutine_traits_bad_arg_yield() {
   co_yield a; // expected-error {{include <experimental/coroutine>}}