llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-coroutines Author: Yexuan Xiao (YexuanXiao) <details> <summary>Changes</summary> This patch attempts to implement the solution I proposed for [CWG2935 (Github)](https://github.com/cplusplus/CWG/issues/575), aligning Clang's behavior with GCC and MSVC instead of leaving it undefined. When `initial_suspend` (as well as `ready` and `suspend`) throws an exception, Clang fails to destroy the task even though the task has already been initialized (see https://godbolt.org/z/E4Y4bEn54). This patch updates CGCoroutine.cpp to clean up the coroutine return value after an exception is thrown when it is constructed in place, addressing CWG2935. It does not affect existing valid code, but HALO will not take effect under this circumstance. The test coroutine-cwg2935.cpp should be able to effectively test a series of behaviors during coroutine startup, but I don't know where it should be placed. GCC and MSVC can pass this test successfully, see https://godbolt.org/z/4MjbEExjr. This test is more comprehensive than the one in the previous link. I would like to hear more opinions on the solution and seek help to fix Clang. Since the additional cleanup code currently prevents HALO, and the execution results of the program in this case may be somewhat unexpected, it is planned to add a warning "-Winitial-suspend-throw", which will be implemented by Part2. --- Patch is 23.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/179189.diff 3 Files Affected: - (modified) clang/lib/CodeGen/CGCoroutine.cpp (+47-5) - (added) clang/test/CodeGenCoroutines/cwg2935.cpp (+65) - (added) coroutine-cwg2935.cpp (+612) ``````````diff diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index 7282c42420657..cf5b9546995d6 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -134,24 +134,27 @@ static SmallString<32> buildSuspendPrefixStr(CGCoroData &Coro, AwaitKind Kind) { // Check if function can throw based on prototype noexcept, also works for // destructors which are implicitly noexcept but can be marked noexcept(false). -static bool FunctionCanThrow(const FunctionDecl *D) { +static bool FunctionCanThrow(const FunctionDecl *D, bool InitialSuspend) { const auto *Proto = D->getType()->getAs<FunctionProtoType>(); if (!Proto) { // Function proto is not found, we conservatively assume throwing. return true; } + if (InitialSuspend && D->getIdentifier() && + (D->getIdentifier()->getName() == "await_resume")) + return false; return !isNoexceptExceptionSpec(Proto->getExceptionSpecType()) || Proto->canThrow() != CT_Cannot; } -static bool StmtCanThrow(const Stmt *S) { +static bool StmtCanThrow(const Stmt *S, bool InitialSuspend = false) { if (const auto *CE = dyn_cast<CallExpr>(S)) { const auto *Callee = CE->getDirectCallee(); if (!Callee) // We don't have direct callee. Conservatively assume throwing. return true; - if (FunctionCanThrow(Callee)) + if (FunctionCanThrow(Callee, InitialSuspend)) return true; // Fall through to visit the children. @@ -163,14 +166,14 @@ static bool StmtCanThrow(const Stmt *S) { // We need to mark entire statement as throwing if the destructor of the // temporary throws. const auto *Dtor = TE->getTemporary()->getDestructor(); - if (FunctionCanThrow(Dtor)) + if (FunctionCanThrow(Dtor, false)) return true; // Fall through to visit the children. } for (const auto *child : S->children()) - if (StmtCanThrow(child)) + if (StmtCanThrow(child, InitialSuspend)) return true; return false; @@ -657,10 +660,19 @@ struct GetReturnObjectManager { bool DirectEmit = false; Address GroActiveFlag; + // Active flag used for DirectEmit return value cleanup. When the coroutine + // return value is directly emitted into the return slot, we need to run its + // destructor if an exception is thrown before initial-await-resume. + // DirectGroActiveFlag is unused when InitialSuspendCanThrow is false. + bool InitialSuspendCanThrow = false; + Address DirectGroActiveFlag; + CodeGenFunction::AutoVarEmission GroEmission; GetReturnObjectManager(CodeGenFunction &CGF, const CoroutineBodyStmt &S) : CGF(CGF), Builder(CGF.Builder), S(S), GroActiveFlag(Address::invalid()), + InitialSuspendCanThrow(StmtCanThrow(S.getInitSuspendStmt(), true)), + DirectGroActiveFlag(Address::invalid()), GroEmission(CodeGenFunction::AutoVarEmission::invalid()) { // The call to get_return_object is sequenced before the call to // initial_suspend and is invoked at most once, but there are caveats @@ -768,6 +780,26 @@ struct GetReturnObjectManager { CGF.EmitAnyExprToMem(S.getReturnValue(), CGF.ReturnValue, S.getReturnValue()->getType().getQualifiers(), /*IsInit*/ true); + // If the return value has a non-trivial destructor, register a + // conditional cleanup that will destroy it if an exception is thrown + // before initial-await-resume. The cleanup is activated now and will + // be deactivated once initial_suspend completes normally. + if (InitialSuspendCanThrow) { + if (QualType::DestructionKind DtorKind = + S.getReturnValue()->getType().isDestructedType(); + DtorKind) { + // Create an active flag (initialize to true) for conditional + // cleanup. We are not necessarily in a conditional branch here so + // use a simple temp alloca instead of createCleanupActiveFlag(). + auto ActiveFlag = CGF.CreateTempAlloca( + Builder.getInt1Ty(), CharUnits::One(), "direct.gro.active"); + Builder.CreateStore(Builder.getTrue(), ActiveFlag); + CGF.pushDestroyAndDeferDeactivation(DtorKind, CGF.ReturnValue, + S.getReturnValue()->getType()); + CGF.initFullExprCleanupWithFlag(ActiveFlag); + DirectGroActiveFlag = ActiveFlag; + } + } } return; } @@ -782,6 +814,7 @@ struct GetReturnObjectManager { CGF.EmitAutoVarInit(GroEmission); Builder.CreateStore(Builder.getTrue(), GroActiveFlag); } + // The GRO returns either when it is first suspended or when it completes // without ever being suspended. The EmitGroConv function evaluates these // conditions and perform the conversion if needed. @@ -987,6 +1020,15 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) { CurCoro.Data->CurrentAwaitKind = AwaitKind::Init; CurCoro.Data->ExceptionHandler = S.getExceptionHandler(); EmitStmt(S.getInitSuspendStmt()); + + // If we emitted the return value directly into the return slot, the + // destructor cleanup we registered above should only be active while + // initial_suspend is in progress. After initial_suspend completes + // normally, deactivate the cleanup so the return value is not + // destroyed here. + if (GroManager.DirectEmit && GroManager.DirectGroActiveFlag.isValid()) + Builder.CreateStore(Builder.getFalse(), GroManager.DirectGroActiveFlag); + CurCoro.Data->FinalJD = getJumpDestInCurrentScope(FinalBB); CurCoro.Data->CurrentAwaitKind = AwaitKind::Normal; diff --git a/clang/test/CodeGenCoroutines/cwg2935.cpp b/clang/test/CodeGenCoroutines/cwg2935.cpp new file mode 100644 index 0000000000000..89057cd0f9f10 --- /dev/null +++ b/clang/test/CodeGenCoroutines/cwg2935.cpp @@ -0,0 +1,65 @@ +// Verifies lifetime of __gro local variable +// Verify that coroutine promise and allocated memory are freed up on exception. +// RUN: %clang_cc1 -std=c++20 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -disable-llvm-passes | FileCheck %s + +#include "Inputs/coroutine.h" + +using namespace std; + +struct task { + struct promise_type { + task get_return_object(); + suspend_always initial_suspend(); + suspend_always final_suspend() noexcept; + void return_void(); + void unhandled_exception(); + }; + ~task(); +}; + +task f1() { + co_return; +} +// CHECK: define dso_local void @_Z2f1v +// CHECK: entry: +// CHECK-NEXT: %result.ptr = alloca ptr, align 8 +// CHECK-NEXT: %__promise = alloca %"struct.task::promise_type", align 1 +// CHECK-NEXT: %direct.gro.active = alloca i1, align 1 +// CHECK: coro.init: ; preds = %coro.alloc, %entry +// CHECK-NEXT: %3 = phi ptr [ null, %entry ], [ %call, %coro.alloc ] +// CHECK-NEXT: %4 = call ptr @llvm.coro.begin(token %0, ptr %3) +// CHECK-NEXT: call void @llvm.lifetime.start.p{{.*}}(ptr %__promise) #2 +// CHECK-NEXT: call void @_ZN4task12promise_type17get_return_objectEv(ptr dead_on_unwind writable sret(%struct.task) align 1 %agg.result, ptr noundef nonnull align 1 dereferenceable(1) %__promise) +// CHECK-NEXT: store i1 true, ptr %direct.gro.active, align 1 +// CHECK: cleanup.cont: ; preds = %cleanup +// CHECK-NEXT: store i1 false, ptr %direct.gro.active, align 1 +// CHECK-NEXT: call void @_ZN4task12promise_type11return_voidEv(ptr noundef nonnull align 1 dereferenceable(1) %__promise) +// CHECK-NEXT: br label %coro.final +// CHECK: cleanup.cont{{.*}}: +// CHECK-NEXT: store i1 false, ptr %direct.gro.active, align 1 +// CHECK-NEXT: br label %cleanup{{.*}} +// CHECK: cleanup.action: ; preds = %cleanup{{.*}} +// CHECK-NEXT: call void @_ZN4taskD1Ev(ptr noundef nonnull align 1 dereferenceable(1) %agg.result) #2 +// CHECK-NEXT: br label %cleanup.done + +struct intial_awaiter_resume_throw : std::suspend_always { + void await_resume(); +}; + +struct task_resume_throw { + struct promise_type { + task_resume_throw get_return_object(); + suspend_always initial_suspend() noexcept; + suspend_always final_suspend() noexcept; + void return_void(); + void unhandled_exception(); + }; + ~task_resume_throw(); +}; + +task_resume_throw f2() { + co_return; +} + +// CHECK: define dso_local void @_Z2f2v +// CHECK-NOT: %direct.gro.active = alloca i1, align 1 \ No newline at end of file diff --git a/coroutine-cwg2935.cpp b/coroutine-cwg2935.cpp new file mode 100644 index 0000000000000..6f18c7dae1a28 --- /dev/null +++ b/coroutine-cwg2935.cpp @@ -0,0 +1,612 @@ +#include <array> +#include <cassert> +#include <coroutine> +#include <cstdlib> + +enum check_points { + para_copy_ctor, + para_dtor, + promise_ctor, + promise_dtor, + get_return_obj, + task_ctor, + task_dtor, + init_suspend, + init_a_ready, + init_a_suspend, + init_a_resume, + awaiter_ctor, + awaiter_dtor, + return_v, + unhandled, + fin_suspend +}; + +using per_test_counts_type = std::array<int, fin_suspend + 1>; + +per_test_counts_type per_test_counts{}; + +void record(check_points cp) { + // Each checkpoint's executions must be precisely recorded to prevent double + // free + ++per_test_counts[cp]; +} + +void clear() { per_test_counts = per_test_counts_type{}; } + +std::array<bool, fin_suspend + 1> checked_cond{}; + +// Each test will throw an exception at a designated location. After the +// coroutine is invoked, the execution status of all checkpoints will be +// checked, and then switch to the next test. Before throwing an exception, +// record the execution status first. +void throw_c(check_points cp) { + record(cp); + // Once that point has been tested, it will not be tested again. + if (checked_cond[cp] == false) { + checked_cond[cp] = true; + throw 0; + } +} + +std::size_t allocate_count = 0; + +void *operator new(std::size_t size) { + ++allocate_count; + // When the coroutine is invoked, memory allocation is the first operation + // performed + if (void *ptr = std::malloc(size)) { + return ptr; + } + std::abort(); +} + +void operator delete(void *ptr) noexcept { + // Deallocation is performed last + --allocate_count; + std::free(ptr); +} + +struct copy_observer { +private: + copy_observer() = default; + +public: + copy_observer(copy_observer const &) { throw_c(para_copy_ctor); } + ~copy_observer() { record(para_dtor); } + + static copy_observer get() { return {}; } +}; + +const auto global_observer = copy_observer::get(); + +namespace direct_emit { + +struct task { + task() { throw_c(task_ctor); } + ~task() { record(task_dtor); } + // In this test, the task should be constructed directly, without copy elision + task(task const &) = delete; + struct promise_type { + promise_type() { throw_c(promise_ctor); } + ~promise_type() { record(promise_dtor); } + promise_type(const promise_type &) = delete; + task get_return_object() { + throw_c(get_return_obj); + return {}; + } + auto initial_suspend() { + throw_c(init_suspend); + struct initial_awaiter { + initial_awaiter() { throw_c(awaiter_ctor); } + ~initial_awaiter() { record(awaiter_dtor); } + initial_awaiter(const initial_awaiter &) = delete; + bool await_ready() { + throw_c(init_a_ready); + return false; + } + bool await_suspend(std::coroutine_handle<void>) { + throw_c(init_a_suspend); + return false; + } + void await_resume() { + // From this point onward, all exceptions are handled by + // `unhandled_exception` Since the defect of exceptions escaping from + // `unhandled_exception` remains unresolved (CWG2934), this test only + // covers the coroutine startup phase. Once CWG2934 is resolved, + // further tests can be added based on this one. + record(init_a_resume); + } + }; + + return initial_awaiter{}; + } + void return_void() { record(return_v); } + void unhandled_exception() { record(unhandled); } + // Note that no exceptions may leak after final_suspend is called, otherwise + // the behavior is undefined + std::suspend_never final_suspend() noexcept { + record(fin_suspend); + return {}; + } + }; +}; + +task coro(copy_observer) { co_return; } + +void catch_coro() try { coro(global_observer); } catch (...) { +} + +// Currently, the conditions at the eight potential exception-throwing points +// need to be checked. More checkpoints can be added after CWG2934 is resolved. +void test() { + per_test_counts_type e{}; + allocate_count = 0; + catch_coro(); + e = { + 1, // para_copy_ctor + 0, // para_dtor + 0, // promise_ctor + 0, // promise_dtor + 0, // get_return_obj + 0, // task_ctor + 0, // task_dtor + 0, // init_suspend + 0, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 0, // awaiter_ctor + 0, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 0, // promise_dtor + 0, // get_return_obj + 0, // task_ctor + 0, // task_dtor + 0, // init_suspend + 0, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 0, // awaiter_ctor + 0, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 1, // promise_dtor + 1, // get_return_obj + 0, // task_ctor + 0, // task_dtor + 0, // init_suspend + 0, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 0, // awaiter_ctor + 0, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 1, // promise_dtor + 1, // get_return_obj + 1, // task_ctor + 0, // task_dtor + 0, // init_suspend + 0, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 0, // awaiter_ctor + 0, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 1, // promise_dtor + 1, // get_return_obj + 1, // task_ctor + 1, // task_dtor + 1, // init_suspend + 0, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 0, // awaiter_ctor + 0, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == + per_test_counts); // Clang currently fails starting from this line. If + // the code you modified causes tests above this line + // to fail, it indicates that you have broken the + // correct code and should start over from scratch. + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 1, // promise_dtor + 1, // get_return_obj + 1, // task_ctor + 1, // task_dtor + 1, // init_suspend + 0, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 1, // awaiter_ctor + 0, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 1, // promise_dtor + 1, // get_return_obj + 1, // task_ctor + 1, // task_dtor + 1, // init_suspend + 1, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 1, // awaiter_ctor + 1, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 1, // promise_dtor + 1, // get_return_obj + 1, // task_ctor + 1, // task_dtor + 1, // init_suspend + 1, // init_a_ready + 1, // init_a_suspend + 0, // init_a_resume + 1, // awaiter_ctor + 1, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + // Test for execution without exceptions + { + coro(global_observer); + } + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 1, // promise_dtor + 1, // get_return_obj + 1, // task_ctor + 1, // task_dtor + 1, // init_suspend + 1, // init_a_ready + 1, // init_a_suspend + 1, // init_a_resume + 1, // awaiter_ctor + 1, // awaiter_dtor + 1, // return_v, + 0, // unhandled, + 1, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + assert(allocate_count == 0); +} + +} // namespace direct_emit + +namespace no_direct_emit { + +struct gro_tag_t {}; + +struct task { + task(gro_tag_t) { throw_c(task_ctor); } + ~task() { record(task_dtor); } + // In this test, the task should be constructed directly, without copy elision + task(task const &) = delete; + struct promise_type { + promise_type() { throw_c(promise_ctor); } + ~promise_type() { record(promise_dtor); } + promise_type(const promise_type &) = delete; + gro_tag_t get_return_object() { + throw_c(get_return_obj); + return {}; + } + auto initial_suspend() { + throw_c(init_suspend); + struct initial_awaiter { + initial_awaiter() { throw_c(awaiter_ctor); } + ~initial_awaiter() { record(awaiter_dtor); } + initial_awaiter(const initial_awaiter &) = delete; + bool await_ready() { + throw_c(init_a_ready); + return false; + } + bool await_suspend(std::coroutine_handle<void>) { + throw_c(init_a_suspend); + return false; + } + void await_resume() { + // From this point onward, all exceptions are handled by + // `unhandled_exception` Since the defect of exceptions escaping from + // `unhandled_exception` remains unresolved (CWG2934), this test only + // covers the coroutine startup phase. Once CWG2934 is resolved, + // further tests can be added based on this one. + record(init_a_resume); + } + }; + + return initial_awaiter{}; + } + void return_void() { record(return_v); } + void unhandled_exception() { record(unhandled); } + // Note that no exceptions may leak after final_suspend is called, otherwise + // the behavior is undefined + std::suspend_never final_suspend() noexcept { + record(fin_suspend); + return {}; + } + }; +}; + +task coro(copy_observer) { co_return; } + +void catch_coro() try { coro(global_observer); } catch (...) { +} + +// Currently, the conditions at the eight potential exception-throwing points +// need to be checked. More checkpoints can be added after CWG2934 is resolved. +void test() { + per_test_counts_type e{}; + allocate_count = 0; + catch_coro(); + e = { + 1, // para_copy_ctor + 0, // para_dtor + 0, // promise_ctor + 0, // promise_dtor + 0, // get_return_obj + 0, // task_ctor + 0, // task_dtor + 0, // init_suspend + 0, // init_a_ready + 0, // init_a_suspend + 0, // init_a_resume + 0, // awaiter_ctor + 0, // awaiter_dtor + 0, // return_v, + 0, // unhandled, + 0, // fin_suspend + }; + assert(e == per_test_counts); + clear(); + catch_coro(); + e = { + 2, // para_copy_ctor + 2, // para_dtor + 1, // promise_ctor + 0, // promise_dtor + 0, // get_return_obj + 0, // task_ctor + 0, // task... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/179189 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
