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

[Xun Li via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdce8f2bb25ea: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter (authored by lxfind).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89066/new/

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,17 @@
            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 await_suspend call on the awaiter, the coroutine may have
+  // been destroyed. In that case, we can not store anything to the frame
+  // from this point on. Hence here we wrap it immediately with a cleanup. This
+  // could have applied to all await_suspend calls. However doing so causes
+  // alive objects being destructed for reasons that need further
+  // investigations. Here we walk-around it temporarily by only doing it after
+  // the suspend call on the final awaiter (indicated by IsImplicit) where it's
+  // most common to happen.
+  // TODO: Properly clean up the temps generated by await_suspend calls.
+  if (IsImplicit)
+    JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
   return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
                           JustAddress);
 }
@@ -409,7 +416,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 +466,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 +879,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 +934,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.297657.patch
Type: text/x-patch
Size: 4124 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201012/36869606/attachment-0001.bin>