[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

[Xun Li via Phabricator via cfe-commits]

lxfind created this revision.
lxfind added reviewers: lewissbaker, wenlei, bruno, junparser, rjmccall.
Herald added subscribers: cfe-commits, modimo, dexonsmith, modocache.
Herald added a project: clang.
lxfind requested review of this revision.

In https://reviews.llvm.org/D87470 I added the change to tighten the lifetime of the expression awaiter.await_suspend().address.
Howver it was incorrect. ExprWithCleanups will call the dtor and end the lifetime for all the temps created in the current full expr.
When this is called on a normal await call, we don't want to do that.
We only want to do this for the call on the final_awaiter, to avoid writing into the frame after the frame is destroyed.
This change fixes it, by checking IsImplicit.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89066

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp


Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===================================================================
--- clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang -std=c++14 -fcoroutines-ts -emit-llvm -S -O1 %s -o -
+// RUN: %clang -std=c++14 -fcoroutines-ts -emit-llvm -S -O1 %s -o - | FileCheck %s
 
 #include "Inputs/coroutine.h"
 
Index: clang/lib/Sema/SemaCoroutine.cpp
===================================================================
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -375,7 +375,7 @@
 // returning await_suspend that results in a guaranteed tail call to the target
 // coroutine.
 static Expr *maybeTailCall(Sema &S, QualType RetType, Expr *E,
-                           SourceLocation Loc) {
+                           SourceLocation Loc, bool IsImplicit) {
   if (RetType->isReferenceType())
     return nullptr;
   Type const *T = RetType.getTypePtr();
@@ -398,10 +398,12 @@
            diag::warn_coroutine_handle_address_invalid_return_type)
         << JustAddress->getType();
 
-  // The coroutine handle used to obtain the address is no longer needed
-  // at this point, clean it up to avoid unnecessarily long lifetime which
-  // could lead to unnecessary spilling.
-  JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
+  // After the suspend call on the final awaiter, the coroutine may have
+  // been destroyed. In that case, we can not store anything to the frame
+  // from this point on. Hence in the case of the final awaiter suspend
+  // call (indicated by IsImplciit), we wrap it immediately with a cleanup.
+  if (IsImplicit)
+    JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
   return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
                           JustAddress);
 }
@@ -409,7 +411,8 @@
 /// Build calls to await_ready, await_suspend, and await_resume for a co_await
 /// expression.
 static ReadySuspendResumeResult buildCoawaitCalls(Sema &S, VarDecl *CoroPromise,
-                                                  SourceLocation Loc, Expr *E) {
+                                                  SourceLocation Loc, Expr *E,
+                                                  bool IsImplicit) {
   OpaqueValueExpr *Operand = new (S.Context)
       OpaqueValueExpr(Loc, E->getType(), VK_LValue, E->getObjectKind(), E);
 
@@ -458,7 +461,8 @@
     QualType RetType = AwaitSuspend->getCallReturnType(S.Context);
 
     // Experimental support for coroutine_handle returning await_suspend.
-    if (Expr *TailCallSuspend = maybeTailCall(S, RetType, AwaitSuspend, Loc))
+    if (Expr *TailCallSuspend =
+            maybeTailCall(S, RetType, AwaitSuspend, Loc, IsImplicit))
       Calls.Results[ACT::ACT_Suspend] = TailCallSuspend;
     else {
       // non-class prvalues always have cv-unqualified types
@@ -870,8 +874,8 @@
   SourceLocation CallLoc = E->getExprLoc();
 
   // Build the await_ready, await_suspend, await_resume calls.
-  ReadySuspendResumeResult RSS =
-      buildCoawaitCalls(*this, Coroutine->CoroutinePromise, CallLoc, E);
+  ReadySuspendResumeResult RSS = buildCoawaitCalls(
+      *this, Coroutine->CoroutinePromise, CallLoc, E, IsImplicit);
   if (RSS.IsInvalid)
     return ExprError();
 
@@ -925,8 +929,8 @@
     E = CreateMaterializeTemporaryExpr(E->getType(), E, true);
 
   // Build the await_ready, await_suspend, await_resume calls.
-  ReadySuspendResumeResult RSS =
-      buildCoawaitCalls(*this, Coroutine->CoroutinePromise, Loc, E);
+  ReadySuspendResumeResult RSS = buildCoawaitCalls(
+      *this, Coroutine->CoroutinePromise, Loc, E, /*IsImplicit*/ false);
   if (RSS.IsInvalid)
     return ExprError();
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89066.297038.patch
Type: text/x-patch
Size: 3785 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201008/90431274/attachment-0001.bin>