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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
