[Coroutine][Sema] Cleanup temporaries as early as possible
The original bug was discovered in T75057860. Clang front-end emits an AST that looks like this for an co_await expression:
|- ExprWithCleanups
|- -CoawaitExpr
|- -MaterializeTemporaryExpr ... Awaiter
...
|- -CXXMemberCallExpr ... .await_ready
...
|- -CallExpr ... __builtin_coro_resume
...
|- -CXXMemberCallExpr ... .await_resume
...
ExprWithCleanups is responsible for cleaning up (including calling dtors) for the temporaries generated in the wrapping expression).
In the above structure, the __builtin_coro_resume part (which corresponds to the code for the suspend case in the co_await with symmetric transfer), the pseudocode looks like this:
__builtin_coro_resume(
awaiter.await_suspend(
from_address(
__builtin_coro_frame())).address());
One of the temporaries that's generated as part of this code is the coroutine handle returned from awaiter.await_suspend() call. The call returns a handle which is a prvalue (since it's a returned value on the fly). In order to call the address() method on it, it needs to be converted into an xvalue. Hence a materialized temp is created to hold it. This temp will need to be cleaned up eventually. Now, since all cleanups happen at the end of the entire co_await expression, which is after the <coro.suspend> suspension point, the compiler will think that such a temp needs to live across suspensions, and need to be put on the coroutine frame, even though it's only used temporarily just to call address() method.
Such a phenomena not only unnecessarily increases the frame size, but can lead to ASAN failures, if the coroutine was already destroyed as part of the await_suspend() call. This is because if the coroutine was already destroyed, the frame no longer exists, and one can not store anything into it. But if the temporary object is considered to need to live on the frame, it will be stored into the frame after await_suspend() returns.
A fix attempt was done in https://reviews.llvm.org/D87470. Unfortunately it is incorrect. The reason is that cleanups in Clang works more like linearly than nested. There is one current state indicating whether it needs cleanup, and an ExprWithCleanups resets that state. This means that an ExprWithCleanups must be capable of cleaning up all temporaries created in the wrapping expression, otherwise there will be dangling temporaries cleaned up at the wrong place.
I eventually found a walk-around (https://reviews.llvm.org/D89066) that doesn't break any existing tests while fixing the issue. But it targets the final co_await only. If we ever have a co_await that's not on the final awaiter and the frame gets destroyed after suspend, we are in trouble. Hence we need a proper fix.
This patch is the proper fix. It does the folllowing things to fully resolve the issue:
1. The AST has to be generated in the order according to their nesting relationship. We should not generate AST out of order because then the code generator would incorrectly track the state of temporaries and when a cleanup is needed. So the code in buildCoawaitCalls is reorganized so that we will be generating the AST for each coawait member call in order along with their child AST.
2. await_ready() call is wrapped with an ExprWithCleanups so that temporaries in it gets cleaned up as early as possible to avoid living across suspension.
3. await_suspend() call is wrapped with an ExprWithCleanups if it's not a symmetric transfer. In the case of a symmetric transfer, in order to maintain the musttail call contract, the ExprWithCleanups is wraaped before the resume call.
4. In the end, we mark again that it needs a cleanup, so that the entire CoawaitExpr will be wrapped with a ExprWithCleanups which will clean up the Awaiter object associated with the await expression.
Differential Revision: https://reviews.llvm.org/D90990
2020-11-11 05:02:18 +08:00
|
|
|
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++14 -O1 -emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
|
|
|
|
|
|
|
|
|
|
#include "Inputs/coroutine.h"
|
|
|
|
|
|
|
|
|
|
namespace coro = std::experimental::coroutines_v1;
|
|
|
|
|
|
|
|
|
|
struct Task {
|
|
|
|
|
struct promise_type {
|
|
|
|
|
Task get_return_object() noexcept {
|
|
|
|
|
return Task{coro::coroutine_handle<promise_type>::from_promise(*this)};
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
void return_void() noexcept {}
|
|
|
|
|
|
|
|
|
|
struct final_awaiter {
|
|
|
|
|
bool await_ready() noexcept { return false; }
|
|
|
|
|
coro::coroutine_handle<> await_suspend(coro::coroutine_handle<promise_type> h) noexcept {
|
|
|
|
|
h.destroy();
|
|
|
|
|
return {};
|
|
|
|
|
}
|
|
|
|
|
void await_resume() noexcept {}
|
|
|
|
|
};
|
|
|
|
|
|
|
|
|
|
void unhandled_exception() noexcept {}
|
|
|
|
|
|
|
|
|
|
final_awaiter final_suspend() noexcept { return {}; }
|
|
|
|
|
|
|
|
|
|
coro::suspend_always initial_suspend() noexcept { return {}; }
|
|
|
|
|
|
|
|
|
|
template <typename Awaitable>
|
|
|
|
|
auto await_transform(Awaitable &&awaitable) {
|
|
|
|
|
return awaitable.co_viaIfAsync();
|
|
|
|
|
}
|
|
|
|
|
};
|
|
|
|
|
|
|
|
|
|
using handle_t = coro::coroutine_handle<promise_type>;
|
|
|
|
|
|
|
|
|
|
class Awaiter {
|
|
|
|
|
public:
|
|
|
|
|
explicit Awaiter(handle_t coro) noexcept;
|
|
|
|
|
Awaiter(Awaiter &&other) noexcept;
|
|
|
|
|
Awaiter(const Awaiter &) = delete;
|
|
|
|
|
~Awaiter();
|
|
|
|
|
|
|
|
|
|
bool await_ready() noexcept { return false; }
|
|
|
|
|
handle_t await_suspend(coro::coroutine_handle<> continuation) noexcept;
|
|
|
|
|
void await_resume();
|
|
|
|
|
|
|
|
|
|
private:
|
|
|
|
|
handle_t coro_;
|
|
|
|
|
};
|
|
|
|
|
|
|
|
|
|
Task(handle_t coro) noexcept : coro_(coro) {}
|
|
|
|
|
|
|
|
|
|
handle_t coro_;
|
|
|
|
|
|
|
|
|
|
Task(const Task &t) = delete;
|
|
|
|
|
Task(Task &&t) noexcept;
|
|
|
|
|
~Task();
|
|
|
|
|
Task &operator=(Task t) noexcept;
|
|
|
|
|
|
|
|
|
|
Awaiter co_viaIfAsync();
|
|
|
|
|
};
|
|
|
|
|
|
|
|
|
|
static Task foo() {
|
|
|
|
|
co_return;
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
Task bar() {
|
|
|
|
|
auto mode = 2;
|
|
|
|
|
switch (mode) {
|
|
|
|
|
case 1:
|
|
|
|
|
co_await foo();
|
|
|
|
|
break;
|
|
|
|
|
case 2:
|
|
|
|
|
co_await foo();
|
|
|
|
|
break;
|
|
|
|
|
default:
|
|
|
|
|
break;
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
2020-12-31 16:27:11 +08:00
|
|
|
// CHECK-LABEL: define{{.*}} void @_Z3barv
|
[Coroutine][Sema] Cleanup temporaries as early as possible
The original bug was discovered in T75057860. Clang front-end emits an AST that looks like this for an co_await expression:
|- ExprWithCleanups
|- -CoawaitExpr
|- -MaterializeTemporaryExpr ... Awaiter
...
|- -CXXMemberCallExpr ... .await_ready
...
|- -CallExpr ... __builtin_coro_resume
...
|- -CXXMemberCallExpr ... .await_resume
...
ExprWithCleanups is responsible for cleaning up (including calling dtors) for the temporaries generated in the wrapping expression).
In the above structure, the __builtin_coro_resume part (which corresponds to the code for the suspend case in the co_await with symmetric transfer), the pseudocode looks like this:
__builtin_coro_resume(
awaiter.await_suspend(
from_address(
__builtin_coro_frame())).address());
One of the temporaries that's generated as part of this code is the coroutine handle returned from awaiter.await_suspend() call. The call returns a handle which is a prvalue (since it's a returned value on the fly). In order to call the address() method on it, it needs to be converted into an xvalue. Hence a materialized temp is created to hold it. This temp will need to be cleaned up eventually. Now, since all cleanups happen at the end of the entire co_await expression, which is after the <coro.suspend> suspension point, the compiler will think that such a temp needs to live across suspensions, and need to be put on the coroutine frame, even though it's only used temporarily just to call address() method.
Such a phenomena not only unnecessarily increases the frame size, but can lead to ASAN failures, if the coroutine was already destroyed as part of the await_suspend() call. This is because if the coroutine was already destroyed, the frame no longer exists, and one can not store anything into it. But if the temporary object is considered to need to live on the frame, it will be stored into the frame after await_suspend() returns.
A fix attempt was done in https://reviews.llvm.org/D87470. Unfortunately it is incorrect. The reason is that cleanups in Clang works more like linearly than nested. There is one current state indicating whether it needs cleanup, and an ExprWithCleanups resets that state. This means that an ExprWithCleanups must be capable of cleaning up all temporaries created in the wrapping expression, otherwise there will be dangling temporaries cleaned up at the wrong place.
I eventually found a walk-around (https://reviews.llvm.org/D89066) that doesn't break any existing tests while fixing the issue. But it targets the final co_await only. If we ever have a co_await that's not on the final awaiter and the frame gets destroyed after suspend, we are in trouble. Hence we need a proper fix.
This patch is the proper fix. It does the folllowing things to fully resolve the issue:
1. The AST has to be generated in the order according to their nesting relationship. We should not generate AST out of order because then the code generator would incorrectly track the state of temporaries and when a cleanup is needed. So the code in buildCoawaitCalls is reorganized so that we will be generating the AST for each coawait member call in order along with their child AST.
2. await_ready() call is wrapped with an ExprWithCleanups so that temporaries in it gets cleaned up as early as possible to avoid living across suspension.
3. await_suspend() call is wrapped with an ExprWithCleanups if it's not a symmetric transfer. In the case of a symmetric transfer, in order to maintain the musttail call contract, the ExprWithCleanups is wraaped before the resume call.
4. In the end, we mark again that it needs a cleanup, so that the entire CoawaitExpr will be wrapped with a ExprWithCleanups which will clean up the Awaiter object associated with the await expression.
Differential Revision: https://reviews.llvm.org/D90990
2020-11-11 05:02:18 +08:00
|
|
|
// CHECK: %[[MODE:.+]] = load i32, i32* %mode
|
|
|
|
|
// CHECK-NEXT: switch i32 %[[MODE]], label %{{.+}} [
|
|
|
|
|
// CHECK-NEXT: i32 1, label %[[CASE1:.+]]
|
|
|
|
|
// CHECK-NEXT: i32 2, label %[[CASE2:.+]]
|
|
|
|
|
// CHECK-NEXT: ]
|
|
|
|
|
|
|
|
|
|
// CHECK: [[CASE1]]:
|
|
|
|
|
// CHECK: br i1 %{{.+}}, label %[[CASE1_AWAIT_READY:.+]], label %[[CASE1_AWAIT_SUSPEND:.+]]
|
|
|
|
|
// CHECK: [[CASE1_AWAIT_SUSPEND]]:
|
|
|
|
|
// CHECK-NEXT: %{{.+}} = call token @llvm.coro.save(i8* null)
|
|
|
|
|
// CHECK-NEXT: %[[HANDLE11:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP1:.+]] to i8*
|
|
|
|
|
// CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 8, i8* %[[HANDLE11]])
|
|
|
|
|
|
|
|
|
|
// CHECK: %[[HANDLE12:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP1]] to i8*
|
|
|
|
|
// CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 8, i8* %[[HANDLE12]])
|
|
|
|
|
// CHECK-NEXT: call void @llvm.coro.resume
|
|
|
|
|
// CHECK-NEXT: %{{.+}} = call i8 @llvm.coro.suspend
|
|
|
|
|
// CHECK-NEXT: switch i8 %{{.+}}, label %coro.ret [
|
|
|
|
|
// CHECK-NEXT: i8 0, label %[[CASE1_AWAIT_READY]]
|
|
|
|
|
// CHECK-NEXT: i8 1, label %[[CASE1_AWAIT_CLEANUP:.+]]
|
|
|
|
|
// CHECK-NEXT: ]
|
|
|
|
|
// CHECK: [[CASE1_AWAIT_CLEANUP]]:
|
|
|
|
|
// make sure that the awaiter eventually gets cleaned up.
|
|
|
|
|
// CHECK: call void @{{.+Awaiter.+}}
|
|
|
|
|
|
|
|
|
|
// CHECK: [[CASE2]]:
|
|
|
|
|
// CHECK: br i1 %{{.+}}, label %[[CASE2_AWAIT_READY:.+]], label %[[CASE2_AWAIT_SUSPEND:.+]]
|
|
|
|
|
// CHECK: [[CASE2_AWAIT_SUSPEND]]:
|
|
|
|
|
// CHECK-NEXT: %{{.+}} = call token @llvm.coro.save(i8* null)
|
|
|
|
|
// CHECK-NEXT: %[[HANDLE21:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP2:.+]] to i8*
|
|
|
|
|
// CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 8, i8* %[[HANDLE21]])
|
|
|
|
|
|
|
|
|
|
// CHECK: %[[HANDLE22:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP2]] to i8*
|
|
|
|
|
// CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 8, i8* %[[HANDLE22]])
|
|
|
|
|
// CHECK-NEXT: call void @llvm.coro.resume
|
|
|
|
|
// CHECK-NEXT: %{{.+}} = call i8 @llvm.coro.suspend
|
|
|
|
|
// CHECK-NEXT: switch i8 %{{.+}}, label %coro.ret [
|
|
|
|
|
// CHECK-NEXT: i8 0, label %[[CASE2_AWAIT_READY]]
|
|
|
|
|
// CHECK-NEXT: i8 1, label %[[CASE2_AWAIT_CLEANUP:.+]]
|
|
|
|
|
// CHECK-NEXT: ]
|
|
|
|
|
// CHECK: [[CASE2_AWAIT_CLEANUP]]:
|
|
|
|
|
// make sure that the awaiter eventually gets cleaned up.
|
|
|
|
|
// CHECK: call void @{{.+Awaiter.+}}
|