[Coroutines] Mark coroutine as nounwind if all the calls outside the try-catch wouldn't throw.

[ChuanqiXu]

Summary

A coroutine would generate following codes:

allocate memories;
if (allocation fails)
  return promise-type::get_return_object_on_allocation_failure();
promise-type promise promise-constructor-arguments ;
auto GRO = promise.get_return_object();
auto init_awaiter = promise.initial_suspend();
init_awaiter.await_ready();
init_awaiter.await_suspend(promise);
try {
    init_awaiter.await_resume();
    function-body
} catch (...) {
    promise.unhandled_exception();
}
co_await promise.final_suspend();

promise_type::final_suspend is guranteed to be noexcept.
So we could mark the coroutine as no-throw if all the followings are
no-throw:

• Promise Constructor.
• Parameter Copy/Move Constructor.
• Memory allocator
• promise-type::get_return_object_on_allocation_failure().
• promise.get_return_object().
• promise.initial_suspend(), init_awaiter.await_ready() and init_awaiter.await_suspend().
• promise.unhandled_exception()

Test Plan: check-llvm, cppcoro, folly

[junparser]

clang/lib/AST/StmtCXX.cpp

144

Without unhandled_exception, we current do not emit try-catch block to catch the exception, so it should return false here.

clang/lib/CodeGen/CGCoroutine.cpp

548

initial_suspend is outside of the try-catch in our implementation now. I'm not sure whether something has been changed here. please double check

[junparser]

we may also conside promise.get_return_object(), even parameter copy/move constructors?

[ChuanqiXu]

Address the comments.

[ChuanqiXu]

In D108277#2951823, @junparser wrote:

we may also conside promise.get_return_object(), even parameter copy/move constructors?

get_return_object() is covered in the try-catch. I forgot the parameter constructors. Will do.

clang/lib/CodeGen/CGCoroutine.cpp

548

Now the promise:: initial_suspend() and corresponding await_ready and await_suspend is outside of the try-catch. I had fixed it.

[ChuanqiXu]

Handle Param Moves.

[ChuanqiXu]

Sorry that I found get_return_object and get_return_object_on_allocation_failure are also outside of the try..catch block. Handled them in this revision.

[junparser]

what about allocation function?

BTW，with so many function to check, can we do this in ir level?

[ChuanqiXu]

Handle allocation function.

[ChuanqiXu]

In D108277#2959321, @junparser wrote:

what about allocation function?

BTW，with so many function to check, can we do this in ir level?

Sorry for forgetting the allocation function. After handle it, I think the list should be complete.

Maybe it is possible to do this in middle end. But it looks harder than this approach. I would add it to my to-do list.

[lxfind]

Thanks for working on this!
A few questions:

• Is this primarily an optimization, or does it also fix a bug?
• Does LLVM mid-end not have a nothrow attribute analysis pass? Doing this manually in the front-end seems a bit fragile (concerned about missing something)
• Do we expect those isXXXNoThrow methods to be called else where? If not, is there a reason we choose to put them in 6 separate functions, instead of one function?

[ChuanqiXu]

In D108277#2960350, @lxfind wrote:

Thanks for working on this!
A few questions:

• Is this primarily an optimization, or does it also fix a bug?

Yes, it is purely an optimization. On the one hand, this could convert invoke coro; into call coro;. On the other hand, it would reduce the exceptional path, which is helpful for other analysis.

• Does LLVM mid-end not have a nothrow attribute analysis pass? Doing this manually in the front-end seems a bit fragile (concerned about missing something)

From my perspective, the LLVM mid-end doesn't have a nothrow analysis pass as I expected in this patch. For example:

void bar();
void foo() {
    try {
        bar();
    } catch(...) {

    }
}
void baz() {
    try {
        foo();
    } catch(...) {

    }
}

foo wouldn't be marked as no-throw even if we use O3. BTW, if we mark bar() as noexcept, foo() and bad() would get optimized correctly.

• Do we expect those isXXXNoThrow methods to be called else where? If not, is there a reason we choose to put them in 6 separate functions, instead of one function?

The reason why I use 6 calls instead of 1 is that I think it is more clear. The 6 calls are easy to understand once the reader reads the comment. And if we use one function, I am afraid that the reader need one more jump to read the definition.

[rjmccall]

From your description, it sounds like the optimizer doesn't make any effort to compute nounwind bottom-up on the call graph. That seems like something that, if it can be fixed, would be a lot simpler and have a lot more general applicability than what you're doing here.

clang/lib/AST/Expr.cpp

3774 ↗

(On Diff #368061)

If this is ultimately supposed to be a "does this expression throw" condition, doing that condition correctly is a lot more complicated than this. Some of the complexity in Sema::canThrow wouldn't be necessary here because it allows for dependent expressions, but a lot of it is.

[ChuanqiXu]

Address comments:

• Call one function in CGCoroutine.cpp::EmitCoroutineBody instead of 6 calls.
• Use Sema::canThrow to replace self-implemented Expr::hasMayThrowCall

[ChuanqiXu]

In D108277#2961770, @rjmccall wrote:

From your description, it sounds like the optimizer doesn't make any effort to compute nounwind bottom-up on the call graph. That seems like something that, if it can be fixed, would be a lot simpler and have a lot more general applicability than what you're doing here.

Yes. It is clear that if a pass to calculate the nounwind attribute in the middle end would be much more general than this one, which applies to coroutine only. And I am looking at the documents now : ). And I understand that it is a much higher bar to implement a pass and turn it on by default.
And I think this patch is still meaningful even after we had a pass to calculate nounwind attribute. Since this approach is faster. Thanks for the SemaCoroutine which would calculate the structure of coroutine body statement. We could check the corresponding node of the coroutine body statement directly.

[ChuanqiXu]

Remove unused header.

[junparser]

LGTM, @rjmccall @lxfind any other comments?

[rjmccall]

clang/include/clang/AST/StmtCXX.h

336

Please add a CoroutineStmtBitfields for this and NumParams. You can follow the pattern of e.g. LabelStmtBitfields.

446

You've written "class" instead of "method" here. Also, this isn't really a "helper" anything, it's just a query on the statement.

The name should be clear that you're really just talking about whether the initial "ramp" portion of the coroutine can throw.

Can a coroutine have a function-try-block as their body, and if so, what happens to exceptions from the generated ramp portion of the coroutine?

[ChuanqiXu]

• Refactor CoroutineBodyStmt::MayThrow and CoroutineBodyStmt::NumParams into Stmt::CoroutineBodyStmtBitfields.
• Rephrase the comment for CoroutineBodyStmt::mayThrow.

---

This revision also touched ASTWriter and ASTReader. But I didn't find the place to test for ASTStmtWriter and ASTStmtReader.

[ChuanqiXu]

clang/include/clang/AST/StmtCXX.h

336

Thanks for suggestion!

446

The name should be clear that you're really just talking about whether the initial "ramp" portion of the coroutine can throw.

This may not be precise. Since the condition should be whether the ramp part and the promise_type::unhandled_exception() can throw. So I think mayThrow tells whether or not the coroutine can throw.

Can a coroutine have a function-try-block as their body, and if so, what happens to exceptions from the generated ramp portion of the coroutine?

If that happens, the exceptions would be thrown to the caller of the coroutine instead of the catch block in the coroutine nor the corresponding promise_type::unhandled_exception().

[rjmccall]

We also have to worry about throwing destructors for all of the introduced variables, right?

I'm still not sold on the idea that we should be doing this in the frontend. I mean, your argument could really apply to any function, not just coroutines.

Alternatively, if we do want the frontend to compute a simple, local nounwind, we could probably do it in IRGen, not by inspecting the AST but by just remembering at the end of the function if we ever called EmitCallOrInvoke in a context that doesn't catch all exceptions.

clang/include/clang/AST/StmtCXX.h

446

This may not be precise. Since the condition should be whether the ramp part and the promise_type::unhandled_exception() can throw. So I think mayThrow tells whether or not the coroutine can throw.

Oh, I see your point, sorry about that.

clang/lib/CodeGen/CGCoroutine.cpp

572

This whole comment is out of place now, I think.

[ChuanqiXu]

In D108277#2968686, @rjmccall wrote:

We also have to worry about throwing destructors for all of the introduced variables, right?

I think what you mean is ~promise()? Did I understand right? It looks like that we indeed to forget that before.

I'm still not sold on the idea that we should be doing this in the frontend. I mean, your argument could really apply to any function, not just coroutines.

Alternatively, if we do want the frontend to compute a simple, local nounwind, we could probably do it in IRGen, not by inspecting the AST but by just remembering at the end of the function if we ever called EmitCallOrInvoke in a context that doesn't catch all exceptions.

Thanks for suggestion! The alternative makes sense to me. I would like to see actually how hard it is to implement this in middle end. And if it is really so hard, I would like to try your solution. Thanks again.

[ChuanqiXu]

Handled by @MaskRay at: https://reviews.llvm.org/D108905

clang/include/clang/AST/StmtCXX.h

enum SubStmt {
     Deallocate,    ///< Coroutine frame memory deallocation.
     ReturnValue,   ///< Return value for thunk function: p.get_return_object().
     ResultDecl,    ///< Declaration holding the result of get_return_object.
     ReturnStmt,    ///< Return statement for the thunk function.
     ReturnStmtOnAllocFailure, ///< Return statement if allocation failed.
     FirstParamMove ///< First offset for move construction of parameter copies.
   };
   unsigned NumParams;
 
   friend class ASTStmtReader;
   friend class ASTReader;
   friend TrailingObjects;
 
   Stmt **getStoredStmts() { return getTrailingObjects<Stmt *>(); }
 
   Stmt *const *getStoredStmts() const { return getTrailingObjects<Stmt *>(); }
 
public:
   }
 
   Stmt *getPromiseDeclStmt() const {
     return getStoredStmts()[SubStmt::Promise];
   }
   VarDecl *getPromiseDecl() const {
     return cast<VarDecl>(cast<DeclStmt>(getPromiseDeclStmt())->getSingleDecl());
   }
+  /// Return true if the constructor of the promise wouldn't throw,
+  /// return false other wise.
+  bool isPromiseCtorNothrow() const;
+  /// Return true if the promise_type::unhandled_exception wouldn't throw,
+  /// return false other wise.
+  bool isOnExceptionNothrow() const;
 
   Stmt *getInitSuspendStmt() const {
     return getStoredStmts()[SubStmt::InitSuspend];
   }
   Stmt *getFinalSuspendStmt() const {
     return getStoredStmts()[SubStmt::FinalSuspend];
   }
 
public:
   }
 
   const_child_range children() const {
     return const_child_range(getStoredStmts(), getStoredStmts() +
                                                    SubStmt::FirstParamMove +
                                                    NumParams);
   }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == CoroutineBodyStmtClass;
   }
 };
 
 /// Represents a 'co_return' statement in the C++ Coroutines TS.
 ///
 /// This statament models the initialization of the coroutine promise
 /// (encapsulating the eventual notional return value) from an expression

clang/lib/AST/StmtCXX.cpp

 //===--- StmtCXX.cpp - Classes for representing C++ statements ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the subclesses of Stmt class declared in StmtCXX.h
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/StmtCXX.h"
+#include "clang/AST/ExprCXX.h"
 
 #include "clang/AST/ASTContext.h"
 
 using namespace clang;
 
 QualType CXXCatchStmt::getCaughtType() const {
   if (ExceptionDecl)
     return ExceptionDecl->getType();
CoroutineBodyStmt::CoroutineBodyStmt(CoroutineBodyStmt::CtorArgs const &Args)
   SubStmts[CoroutineBodyStmt::ReturnValue] = Args.ReturnValue;
   SubStmts[CoroutineBodyStmt::ResultDecl] = Args.ResultDecl;
   SubStmts[CoroutineBodyStmt::ReturnStmt] = Args.ReturnStmt;
   SubStmts[CoroutineBodyStmt::ReturnStmtOnAllocFailure] =
       Args.ReturnStmtOnAllocFailure;
   std::copy(Args.ParamMoves.begin(), Args.ParamMoves.end(),
             const_cast<Stmt **>(getParamMoves().data()));
 }
+
+bool CoroutineBodyStmt::isPromiseCtorNothrow() const {
+  Expr *CtorExpr = getPromiseDecl()->getInit();
+  if (isa<ExprWithCleanups>(CtorExpr))
+    CtorExpr = cast<ExprWithCleanups>(CtorExpr)->getSubExpr();
+  if (!isa<CXXConstructExpr>(CtorExpr))
+    // Unimaged pattern. Return false conservatively.
+    return false;
+
+  auto *Ctor = cast<CXXConstructExpr>(CtorExpr)->getConstructor();
+  return isNoexceptExceptionSpec(Ctor->getExceptionSpecType());
+}
+bool CoroutineBodyStmt::isOnExceptionNothrow() const {
+  Stmt *ExceptionHandler = getExceptionHandler();
+  if (!ExceptionHandler)
+    return true;
+  Expr *OnExceptionCall = cast<Expr>(ExceptionHandler);
+  if (isa<ExprWithCleanups>(OnExceptionCall))
+    OnExceptionCall = cast<ExprWithCleanups>(OnExceptionCall)->getSubExpr();
+  if (!isa<CallExpr>(OnExceptionCall))
+    // Unimaged pattern. Return false conservatively.
+    return false;
+  auto *UnhandledException = cast<CallExpr>(OnExceptionCall)->getDirectCallee();
+  assert(UnhandledException &&
+         "The Call to promise_type::unhandled_exception is not direct call.\n");
+  return isNoexceptExceptionSpec(UnhandledException->getExceptionSpecType());
+}
 No newline at end of file

clang/lib/CodeGen/CGCoroutine.cpp

static void emitBodyAndFallthrough(CodeGenFunction &CGF,
   CGF.EmitStmt(Body);
   const bool CanFallthrough = CGF.Builder.GetInsertBlock();
   if (CanFallthrough)
     if (Stmt *OnFallthrough = S.getFallthroughHandler())
       CGF.EmitStmt(OnFallthrough);
 }
 
 void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
+  // A coroutine would generate following codes:
+  // ```
+  //    promise-type promise promise-constructor-arguments ;
+  //    try {
+  //        co_await promise.initial_suspend();
+  //        function-body
+  //    } catch (...) {
+  //        promise.unhandled_exception();
+  //    }
+  //    co_await promise.final_suspend();
+  //  ```
+  //  promise_type::final_suspend is guranteed to be noexcept. So if the
+  //  constructor of promise_type and unhandled_exception() are guranteed
+  //  to be no throw, we could mark the coroutine as no throw.
+  if (S.isPromiseCtorNothrow() && S.isOnExceptionNothrow())
+    CurFn->addFnAttr(llvm::Attribute::NoUnwind);
+
   auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
   auto &TI = CGM.getContext().getTargetInfo();
   unsigned NewAlign = TI.getNewAlign() / TI.getCharWidth();
 
   auto *EntryBB = Builder.GetInsertBlock();
   auto *AllocBB = createBasicBlock("coro.alloc");
   auto *InitBB = createBasicBlock("coro.init");
   auto *FinalBB = createBasicBlock("coro.final");
   auto *RetBB = createBasicBlock("coro.ret");
 
   auto *CoroId = Builder.CreateCall(
       CGM.getIntrinsic(llvm::Intrinsic::coro_id),
       {Builder.getInt32(NewAlign), NullPtr, NullPtr, NullPtr});
   createCoroData(*this, CurCoro, CoroId);
   CurCoro.Data->SuspendBB = RetBB;
   assert(ShouldEmitLifetimeMarkers &&
          "Must emit lifetime intrinsics for coroutines");
 
   // Backend is allowed to elide memory allocations, to help it, emit
   // auto mem = coro.alloc() ? 0 : ... allocation code ...;

clang/test/CodeGenCoroutines/coro-nounwind.cpp

+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -triple=x86_64-pc-windows-msvc18.0.0 -emit-llvm %s -o - -fexceptions -fcxx-exceptions -disable-llvm-passes | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -fexceptions -fcxx-exceptions -disable-llvm-passes | FileCheck --check-prefix=CHECK-LINUX %s
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct coro_nothrow_t {
+  struct promise_type {
+    coro_nothrow_t get_return_object() {
+      coro::coroutine_handle<promise_type>{};
+      return {};
+    }
+    coro::suspend_never initial_suspend() { return {}; }
+    coro::suspend_never final_suspend() noexcept { return {}; }
+    void return_void() {}
+    void unhandled_exception() noexcept;
+  };
+};
+
+// CHECK: define dso_local i8 @"?foo@@YA?AUcoro_nothrow_t{{.*}}#[[FOOATTRNUM:[0-9]+]]
+// CHECK-LINUX: define dso_local void @_Z3foov{{.*}}#[[FOOATTRNUM:[0-9]+]]
+
+coro_nothrow_t foo() {
+  co_return;
+}
+
+struct coro_ctor_maythrow_t {
+  struct promise_type {
+    coro_ctor_maythrow_t get_return_object() {
+      coro::coroutine_handle<promise_type>{};
+      return {};
+    }
+    coro::suspend_never initial_suspend() { return {}; }
+    coro::suspend_never final_suspend() noexcept { return {}; }
+    void return_void() {}
+    void unhandled_exception() noexcept;
+    promise_type();
+  };
+};
+
+// CHECK: define dso_local i8 @"?bar@@YA?AUcoro_ctor_maythrow_t{{.*}}#[[BARATTRNUM:[0-9]+]]
+// CHECK-LINUX: define dso_local void @_Z3barv() #[[BARATTRNUM:[0-9]+]]
+coro_ctor_maythrow_t bar() {
+  co_return;
+}
+
+struct coro_onexception_maythrow_t {
+  struct promise_type {
+    coro_onexception_maythrow_t get_return_object() {
+      coro::coroutine_handle<promise_type>{};
+      return {};
+    }
+    coro::suspend_never initial_suspend() { return {}; }
+    coro::suspend_never final_suspend() noexcept { return {}; }
+    void return_void() {}
+    void unhandled_exception();
+  };
+};
+
+// CHECK: define dso_local i8 @"?baz@@YA?AUcoro_onexception_maythrow_t{{.*}}#[[BAZATTRNUM:[0-9]+]]
+// CHECK-LINUX: define dso_local void @_Z3bazv() #[[BAZATTRNUM:[0-9]+]]
+
+// CHECK: attributes #[[FOOATTRNUM]] = {{.*}}nounwind
+// CHECK-NOT: attributes #[[BARATTRNUM]] = {{.*}}nounwind
+// CHECK-NOT: attributes #[[BAZATTRNUM]] = {{.*}}nounwind
+// CHECK-LINUX: attributes #[[FOOATTRNUM]] = {{.*}}nounwind
+// CHECK-LINUX-NOT: attributes #[[BARATTRNUM]] = {{.*}}nounwind
+// CHECK-LINUX-NOT: attributes #[[BAZATTRNUM]] = {{.*}}nounwind
+coro_onexception_maythrow_t baz() {
+  co_return;
+}
 No newline at end of file