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

[lxfind]

Summary

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.

[junparser]

why we should not do this with normal await call?

[lxfind]

In D89066#2324115, @junparser wrote:

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

[junparser]

In D89066#2324151, @lxfind wrote:

In D89066#2324115, @junparser wrote:

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

[lxfind]

In D89066#2324291, @junparser wrote:

In D89066#2324151, @lxfind wrote:

In D89066#2324115, @junparser wrote:

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

Unfortunately I don't. But this is not related to ASAN. Basically, this is causing destructing of objects that should still be alive. I suspect that it's because ExprWithCleanups always clean up temps that belongs to the full expression, not just the sub-expression in it.

[wenlei]

LGTM. I think we can get this in first to address the fall out from https://reviews.llvm.org/D87470, while investigating ASAN failure.

clang/lib/Sema/SemaCoroutine.cpp

401–411

Can you add comment explaining why we don't cleanup for all await, probably a TODO?

[lxfind]

Add more comments and TODO

[lxfind]

There seems to be build failures in the buildbot, but I don't understand why it's happening.. (unable to repro locally and the patterns seem correct)
http://lab.llvm.org:8011/#/builders/12/builds/92/steps/7/logs/FAIL__Clang__coro-semmetric-transfer_cpp

[lxfind]

Test failures are being fixed in https://reviews.llvm.org/D89269.

[junparser]

In D89066#2325358, @lxfind wrote:

In D89066#2324291, @junparser wrote:

In D89066#2324151, @lxfind wrote:

In D89066#2324115, @junparser wrote:

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

Unfortunately I don't. But this is not related to ASAN. Basically, this is causing destructing of objects that should still be alive. I suspect that it's because ExprWithCleanups always clean up temps that belongs to the full expression, not just the sub-expression in it.

should we also need to add ExprWithCleanups when expand co_await to await_ready & await_suspend and await_resume expression which may fix this issue？

clang/lib/Sema/SemaCoroutine.cpp

static ExprResult buildMemberCall(Sema &S, Expr *Base, SourceLocation Loc,
   return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, Loc, nullptr);
 }
 
 // See if return type is coroutine-handle and if so, invoke builtin coro-resume
 // on its address. This is to enable experimental support for coroutine-handle
 // 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();
   if (!T->isClassType() && !T->isStructureType())
     return nullptr;
 
   // FIXME: Add convertability check to coroutine_handle<>. Possibly via
   // EvaluateBinaryTypeTrait(BTT_IsConvertible, ...) which is at the moment
   // a private function in SemaExprCXX.cpp
 
   ExprResult AddressExpr = buildMemberCall(S, E, Loc, "address", None);
   if (AddressExpr.isInvalid())
     return nullptr;
 
   Expr *JustAddress = AddressExpr.get();
 
   // Check that the type of AddressExpr is void*
   if (!JustAddress->getType().getTypePtr()->isVoidPointerType())
     S.Diag(cast<CallExpr>(JustAddress)->getCalleeDecl()->getLocation(),
            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.
+  // 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);
 }
 
 /// 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);
 
   // Assume invalid until we see otherwise.
   ReadySuspendResumeResult Calls = {{}, Operand, /*IsInvalid=*/true};
 
   ExprResult CoroHandleRes = buildCoroutineHandle(S, CoroPromise->getType(), Loc);
   if (CoroHandleRes.isInvalid())
static ReadySuspendResumeResult buildCoawaitCalls(Sema &S, VarDecl *CoroPromise,
   if (!AwaitSuspend->getType()->isDependentType()) {
     // [expr.await]p3 [...]
     //   - await-suspend is the expression e.await_suspend(h), which shall be
     //     a prvalue of type void, bool, or std::coroutine_handle<Z> for some
     //     type Z.
     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
       if (RetType->isReferenceType() ||
           (!RetType->isBooleanType() && !RetType->isVoidType())) {
         S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),
                diag::err_await_suspend_invalid_return_type)
             << RetType;
if (E->getValueKind() == VK_RValue)
     E = CreateMaterializeTemporaryExpr(E->getType(), E, true);
 
   // The location of the `co_await` token cannot be used when constructing
   // the member call expressions since it's before the location of `Expr`, which
   // is used as the start of the member call expression.
   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();
 
   Expr *Res =
       new (Context) CoawaitExpr(Loc, E, RSS.Results[0], RSS.Results[1],
                                 RSS.Results[2], RSS.OpaqueValue, IsImplicit);
 
   return Res;
ExprResult Sema::BuildCoyieldExpr(SourceLocation Loc, Expr *E) {
   }
 
   // If the expression is a temporary, materialize it as an lvalue so that we
   // can use it multiple times.
   if (E->getValueKind() == VK_RValue)
     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();
 
   Expr *Res =
       new (Context) CoyieldExpr(Loc, E, RSS.Results[0], RSS.Results[1],
                                 RSS.Results[2], RSS.OpaqueValue);
 
   return Res;

clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp

-// 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"
 
 namespace coro = std::experimental::coroutines_v1;
 
 struct detached_task {
   struct promise_type {
     detached_task get_return_object() noexcept {