[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

Tue Mar 16 12:15:37 PDT 2021

lxfind added a comment.

In D98638#2628082 <https://reviews.llvm.org/D98638#2628082>, @ChuanqiXu wrote:

> It looks like there are two things this patch wants to do:
>
> 1. Don't put the temporary generated by symmetric-transfer on the coroutine frame.
> 2. Offer a mechanism to force some values (it is easy to extend Alloca to Value) to put in the stack instead of the coroutine frame.
>
> I am a little confused about the first problem. Would it cause the program to crash? (e.g., we access the fields of coroutine frame after the frame gets destroyed). Or it just wastes some storage?
> And I want to ask about the change of the AST nodes and SemaCoroutine. Can we know if a CoroutineSuspendExpr stands for a symmetric-transfer? If yes, it seems we can only do changes in CodeGen part.

It will result in a crash, because we will be accessing memory that's already freed.  If you run:

  bin/clang -fcoroutines-ts -std=c++14 -stdlib=libc++ ../clang/test/CodeGenCoroutines/coro-symmetric-transfer-01.cpp -o - -emit-llvm -S -Xclang -disable-llvm-passes

You can see that in the `final.suspend` basic block, there are IRs like this:

    %call19 = call i8* @_ZN13detached_task12promise_type13final_awaiter13await_suspendENSt12experimental13coroutines_v116coroutine_handleIS0_EE(%"struct.detached_task::promise_type::final_awaiter"* nonnull dereferenceable(1) %ref.tm
  p10, i8* %22) #2
    %coerce.dive20 = getelementptr inbounds %"struct.std::experimental::coroutines_v1::coroutine_handle.0", %"struct.std::experimental::coroutines_v1::coroutine_handle.0"* %coerce, i32 0, i32 0
    store i8* %call19, i8** %coerce.dive20, align 8
    %call21 = call i8* @_ZNKSt12experimental13coroutines_v116coroutine_handleIvE7addressEv(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"* nonnull dereferenceable(8) %coerce) #2
    call void @llvm.coro.resume(i8* %call21)

The temporary variable %coerce will be put on the frame because it's used by the call to `address` function and LLVM thinks it may escape. But the call to await_suspend() (the first line) in reality could destroy the current coroutine frame. Hence after the call to await_suspend, it will be accessing the frame, leading to memory corruption.

> Then I agree to introduce new intrinsic to hint the middle end to put some values on the stack. And the design of `@llvm.coro.forcestack.begin()` and `@llvm.coro.forcestack.end()` is a little strange to me. It says they mark a region where only data from the local stack can be accessed. But it looks error-prone since it is hard for the front-end to decide whether all the access of the region should be put on the stack. I think we could introduce only one intrinisic `@llvm.coro.forcestack(Value* v)`, we can use the argument to mark the value need to be put on the stack.

This is a good idea. Let me play with it. Thanks!

> And about the problem you mentioned in D96922 <https://reviews.llvm.org/D96922>: "The lifetime of  %coro.gro" starts early and %coro.gro" would be used after `coro.end` (Possibly the destructor?) which would cause the program to access destroyed coroutine frame". It looks like the mechanism could solve this problem by a call to `@llvm.coro.forcestack(%coro.gro)`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638