[Clang] CGCoroutines skip emitting try block for value returning noexcept init await_resume calls

[yuxuanchen1997]

Previously we were not properly skipping the generation of the try { } block around the init_suspend.await_resume() if the await_resume is not returning void. The reason being that the resume expression was wrapped in a CXXBindTemporaryExpr and the first dyn_cast failed, silently ignoring the noexcept. This only mattered for init_suspend because it had its own try block.

This patch changes to first extract the sub expression when we see a CXXBindTemporaryExpr. Then perform the same logic to check for noexcept.

Another version of this patch also wanted to assert the second step by cast<CXXMemberCallExpr> and as far as I understand it should be a valid assumption. I can change to that if upstream prefers.

[llvmbot]

@llvm/pr-subscribers-coroutines
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Yuxuan Chen (yuxuanchen1997)

Changes

Previously we were not properly skipping the generation of the try { } block around the init_suspend.await_resume() if the await_resume is not returning void. The reason being that the resume expression was wrapped in a CXXBindTemporaryExpr and the first dyn_cast failed, silently ignoring the noexcept. This only mattered for init_suspend because it had its own try block.

This patch changes that to extract the sub expression when we see a CXXBindTemporaryExpr. Another version of this patch also wanted to assert by cast<CXXMemberCallExpr> and as far as I understand it should be a valid assumption. I can change to that if upstream prefers.

---

Full diff: https://github.com/llvm/llvm-project/pull/73160.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGCoroutine.cpp (+11-3)
• (modified) clang/test/CodeGenCoroutines/coro-init-await-nontrivial-return.cpp (+47-3)

diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp
index aaf122c0f83bc47..724d471cc9d78b6 100644
--- a/clang/lib/CodeGen/CGCoroutine.cpp
+++ b/clang/lib/CodeGen/CGCoroutine.cpp
@@ -12,9 +12,10 @@
 
 #include "CGCleanup.h"
 #include "CodeGenFunction.h"
-#include "llvm/ADT/ScopeExit.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtVisitor.h"
+#include "llvm/ADT/ScopeExit.h"
 
 using namespace clang;
 using namespace CodeGen;
@@ -129,7 +130,14 @@ static SmallString<32> buildSuspendPrefixStr(CGCoroData &Coro, AwaitKind Kind) {
   return Prefix;
 }
 
-static bool memberCallExpressionCanThrow(const Expr *E) {
+static bool ResumeExprCanThrow(const CoroutineSuspendExpr &S) {
+  const Expr *E = S.getResumeExpr();
+
+  // If the return type of await_resume is not void, get the CXXMemberCallExpr
+  // from its subexpr.
+  if (const auto *BindTempExpr = dyn_cast<CXXBindTemporaryExpr>(E)) {
+    E = BindTempExpr->getSubExpr();
+  }
   if (const auto *CE = dyn_cast<CXXMemberCallExpr>(E))
     if (const auto *Proto =
             CE->getMethodDecl()->getType()->getAs<FunctionProtoType>())
@@ -233,7 +241,7 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
   // is marked as 'noexcept', we avoid generating this additional IR.
   CXXTryStmt *TryStmt = nullptr;
   if (Coro.ExceptionHandler && Kind == AwaitKind::Init &&
-      memberCallExpressionCanThrow(S.getResumeExpr())) {
+      ResumeExprCanThrow(S)) {
     Coro.ResumeEHVar =
         CGF.CreateTempAlloca(Builder.getInt1Ty(), Prefix + Twine("resume.eh"));
     Builder.CreateFlagStore(true, Coro.ResumeEHVar);
diff --git a/clang/test/CodeGenCoroutines/coro-init-await-nontrivial-return.cpp b/clang/test/CodeGenCoroutines/coro-init-await-nontrivial-return.cpp
index c4b8da327f5c140..0e50a616d6ef7c2 100644
--- a/clang/test/CodeGenCoroutines/coro-init-await-nontrivial-return.cpp
+++ b/clang/test/CodeGenCoroutines/coro-init-await-nontrivial-return.cpp
@@ -3,6 +3,7 @@
 
 #include "Inputs/coroutine.h"
 
+namespace can_throw {
 struct NontrivialType {
   ~NontrivialType() {}
 };
@@ -38,9 +39,52 @@ Task coro_create() {
     co_return;
 }
 
-// CHECK-LABEL: define{{.*}} ptr @_Z11coro_createv(
+// CHECK-LABEL: define{{.*}} ptr @_ZN9can_throw11coro_createEv(
 // CHECK: init.ready:
 // CHECK-NEXT: store i1 true, ptr {{.*}}
-// CHECK-NEXT: call void @_ZN4Task23initial_suspend_awaiter12await_resumeEv(
-// CHECK-NEXT: call void @_ZN14NontrivialTypeD1Ev(
+// CHECK-NEXT: call void @_ZN9can_throw4Task23initial_suspend_awaiter12await_resumeEv(
+// CHECK-NEXT: call void @_ZN9can_throw14NontrivialTypeD1Ev(
 // CHECK-NEXT: store i1 false, ptr {{.*}}
+}
+
+namespace no_throw {
+struct NontrivialType {
+  ~NontrivialType() {}
+};
+
+struct Task {
+    struct promise_type;
+    using handle_type = std::coroutine_handle<promise_type>;
+
+    struct initial_suspend_awaiter {
+        bool await_ready() {
+            return false;
+        }
+
+        void await_suspend(handle_type h) {}
+
+        NontrivialType await_resume() noexcept { return {}; }
+    };
+
+    struct promise_type {
+        void return_void() {}
+        void unhandled_exception() {}
+        initial_suspend_awaiter initial_suspend() { return {}; }
+        std::suspend_never final_suspend() noexcept { return {}; }
+        Task get_return_object() {
+            return Task{handle_type::from_promise(*this)};
+        }
+    };
+
+    handle_type handler;
+};
+
+Task coro_create() {
+    co_return;
+}
+
+// CHECK-LABEL: define{{.*}} ptr @_ZN8no_throw11coro_createEv(
+// CHECK: init.ready:
+// CHECK-NEXT: call void @_ZN8no_throw4Task23initial_suspend_awaiter12await_resumeEv(
+// CHECK-NEXT: call void @_ZN8no_throw14NontrivialTypeD1Ev(
+}

[ChuanqiXu9]

Such pattern match doesn't smell good. How about looking into its children recursively if we find E is not CXXMemberCallExpr?

[yuxuanchen1997]

I find this a little overkill for what we need. The old code here didn't intend to handle more cases either because it just expected the AST to be in a specific shape (CXXMemberCallExpr). This fix is merely just to amend its functionality to work on a temporary expression as well should there be a return value to be discarded. It should be clear intent here that we don't care about anything else. (Ideally, I want an assertion here that it's either of the two Expr types, but I don't have 100% confidence that this is true.)

EDIT: Recursively visiting AST nodes to check if entire Stmt can throw is a much larger task. A complete solution that doesn't overapproximate is complex while making the decision how far you'd like to go before bailing out is probably worth a proper trade-off discussion of its own. Different use cases of it likely will disagree where the line should be drawn. In our use case, since we control CoroutineSuspendExpr, it's not appropriate for us to answer this question. (i.e. Do we want to handle CompoundStmt? What about TryStmt?)

[apolloww]

I think we are only targeting the resume part of init_suspend here, so the cases are limited. Can we change the function name to something more specific, like InitResumeExprCanThrow?

[ChuanqiXu9]

Maybe it is not a such big task as you imaged, you can take a look at Sema::checkFinalSuspendNoThrow and we should be able to make a simpler change than that.

The reason why I don't like the current pattern is that, every time I wrote:

if (auto *WantedExpressionType = isa<AExpreType>(E))
     E = WantedExpressionType->subExpr();
...

I found I always missed some situations.

[yuxuanchen1997]

I guess you are correct that this is easy to miss some cases. In my case I missed the fact that the destructor of the BindTempExpr->getTemporary() can throw if marked noexcept(false). I updated the PR to cover this case and including corresponding tests.

Next up I will try to see how many cases I'll need to cover in a recursive check on children() like the one in Sema::checkFinalSuspendNoThrow.

[ChuanqiXu9]

It looks a little bit confusing. Let's try to rename it to InitNoThrowTask.

[yuxuanchen1997]

I applied your suggested change. I think this name is still bikesheddable though.

[yuxuanchen1997]

@ChuanqiXu9, I updated the check like you suggested, and it should look much nicer now. The only caveat is that the children() call isn't going to cover the implicit destructor call in a CXXBindTemporaryExpr. That pattern matching case is here to stay and likely there are other cases that make this analysis unsound (See https://eel.is/c++draft/except.spec#6). In reality none of them can reach here though so it won't miscompile.

[ChuanqiXu9]

LGTM with nit.

[ChuanqiXu9]

Suggested change

	if (FunctionCanThrow(Callee))
	return true;
	if (!FunctionCanThrow(Callee))
	return false;

nit: This reads better. In case we know the called function is nothrow, we don't need to check it further recursively especially await_resume doesn't have arguments except this.

[yuxuanchen1997]

The logic doesn't go this way.

If a function can throw, the task of this (conservative) analysis is to return true and nothing else needs to be done. Otherwise, fall through and recursively visit all children, which may include Stmts that throw.

[ChuanqiXu9]

Please don't land it when you have different opinions.

It should be safe to return false directly if we know the function is nothrow. Then it may be slightly more efficient.