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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to