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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D98638#2630607 <https://reviews.llvm.org/D98638#2630607>, @lxfind wrote:

> In D98638#2628082 <https://reviews.llvm.org/D98638#2628082>, @ChuanqiXu wrote:
>
>> 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?
>
> This is a repro of the crash (in TSAN mode): https://godbolt.org/z/KvPY66

Oh I got it. The program would crash since handle is destroyed in final_awaiter::await_suspend explicitly:

  std::coroutine_handle<> await_suspend(std::coroutine_handle<promise_type> h) noexcept {
         h.destroy();
         return std::noop_coroutine();
  }

And the normal symmetric transfer wouldn't destroy the handle (although it depends on the implementation of await_suspend).
So the problem met in this patch is program maybe crash with symmetric transfer with destroy coroutine handle explicitly (in the final awaiter normally) instead of normal symmetric transfer.

The explicitly destruction for the coroutine handle in the await_suspend of final awaiter is a normal pattern to enable the Coro-elide optimization. There is a discuss before in cafe-dev: http://clang-developers.42468.n3.nabble.com/Miscompilation-heap-use-after-free-in-C-coroutines-td4070320.html. It looks like it is a subsequent problem.

Here what I want to say is we **shouldn't**  handle all the symmetric transfer from the above analysis. And we shouldn't change the ASTNodes and Sema part. We need to solve about the above pattern. It is not easy to give a solution since user could implement symmetric transfer in final awaiter without destroying the handle, which is more common.

My unfinished idea is to emit an intrinsic called @llvm.coro.finalize before we emit the promise_type::final_suspend. Then the @llvm.coro.finalize marks the end of the lifetime for current coroutine frame. And all the analysis in CoroFrame should consider use after @llvm.coro.finalize (We could emit warning for some cases). But this idea is also problematic, it makes the semantics of coroutine intrinsic more chaos. Just image that how a newbie feels when he see @llvm.coro.end, @llvm.coro.destroy and @llvm.coro.finalize. And we can't use @llvm.coro.end @llvm.coro.destroy  since they have other semantics (llvm.coro.destroy means deletion and llvm.coro.end would be used to split the coroutine). Also, the idea of @llvm.coro.finalize seems available to solve the problem about%gro mentioned above.

It seems to be a workaround to use @llvm.coro.forcestack(%result_of_final_await_suspend) . Since I wondering if there are other corner cases as the %gro. My opinion about '@llvm.coro.forcestack' is that we could use it as a patch if we find any holes that is hard to handle immediately. But we also need to find a solution to solve problems more fundamentally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638