https://github.com/YexuanXiao created https://github.com/llvm/llvm-project/pull/177628
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). Since I know nothing about LLVM IR, this patch is currently incomplete and breaks HALO, causing the following tests to fail: coro-elide-thinlto.cpp, coro-elide.cpp, coro-halo.cpp, pr56919.cpp, pr59723.cpp. All other tests pass. 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. >From 2c70e5670cee67f1525a90cc906cc13ae5051f25 Mon Sep 17 00:00:00 2001 From: Yexuan Xiao <[email protected]> Date: Sat, 24 Jan 2026 01:38:35 +0800 Subject: [PATCH] Try fix CWG2935, but it breaks HALO --- clang/lib/CodeGen/CGCoroutine.cpp | 33 ++ .../coro-suspend-cleanups.cpp | 6 +- coroutine-cwg2935.cpp | 387 ++++++++++++++++++ 3 files changed, 423 insertions(+), 3 deletions(-) create mode 100644 coroutine-cwg2935.cpp diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index c80bb0e004367..d3b064e83a8f9 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -656,10 +656,15 @@ 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. + Address DirectGroActiveFlag; CodeGenFunction::AutoVarEmission GroEmission; GetReturnObjectManager(CodeGenFunction &CGF, const CoroutineBodyStmt &S) : CGF(CGF), Builder(CGF.Builder), S(S), GroActiveFlag(Address::invalid()), + 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 @@ -767,6 +772,25 @@ 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 (QualType::DestructionKind DtorKind = + S.getReturnValue()->getType().isDestructedType()) { + // 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; } @@ -986,6 +1010,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/coro-suspend-cleanups.cpp b/clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp index d71c2c558996a..5aace3f500e07 100644 --- a/clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp +++ b/clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp @@ -47,12 +47,12 @@ coroutine ArrayInitCoro() { // CHECK-NEXT: store ptr %arr.reload.addr, ptr %arrayinit.endOfInit.reload.addr, align 8 // CHECK-NEXT: call void @_ZN6PrintyC1EPKc(ptr noundef nonnull align 8 dereferenceable(8) %arr.reload.addr, ptr noundef @.str) // CHECK-NEXT: %arrayinit.element = getelementptr inbounds %struct.Printy, ptr %arr.reload.addr, i64 1 - // CHECK-NEXT: %arrayinit.element.spill.addr = getelementptr inbounds %_Z13ArrayInitCorov.Frame, ptr %0, i32 0, i32 10 + // CHECK-NEXT: %arrayinit.element.spill.addr = getelementptr inbounds %_Z13ArrayInitCorov.Frame, ptr %0, i32 0, i32 12 // CHECK-NEXT: store ptr %arrayinit.element, ptr %arrayinit.element.spill.addr, align 8 // CHECK-NEXT: store ptr %arrayinit.element, ptr %arrayinit.endOfInit.reload.addr, align 8 co_await Awaiter{} // CHECK-NEXT: @_ZNSt14suspend_always11await_readyEv - // CHECK-NEXT: br i1 %{{.+}}, label %await.ready, label %CoroSave30 + // CHECK-NEXT: br i1 %{{.+}}, label %await.ready, label %CoroSave34 }; // CHECK: await.cleanup: ; preds = %AfterCoroSuspend{{.*}} // CHECK-NEXT: br label %cleanup{{.*}}.from.await.cleanup @@ -61,7 +61,7 @@ coroutine ArrayInitCoro() { // CHECK: br label %cleanup{{.*}} // CHECK: await.ready: - // CHECK-NEXT: %arrayinit.element.reload.addr = getelementptr inbounds %_Z13ArrayInitCorov.Frame, ptr %0, i32 0, i32 10 + // CHECK-NEXT: %arrayinit.element.reload.addr = getelementptr inbounds %_Z13ArrayInitCorov.Frame, ptr %0, i32 0, i32 12 // CHECK-NEXT: %arrayinit.element.reload = load ptr, ptr %arrayinit.element.reload.addr, align 8 // CHECK-NEXT: call void @_ZN7Awaiter12await_resumeEv // CHECK-NEXT: store i1 false, ptr %cleanup.isactive.reload.addr, align 1 diff --git a/coroutine-cwg2935.cpp b/coroutine-cwg2935.cpp new file mode 100644 index 0000000000000..e0c3b8eb60e81 --- /dev/null +++ b/coroutine-cwg2935.cpp @@ -0,0 +1,387 @@ +#include <coroutine> +#include <array> +#include <cassert> +#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 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 {}; + } + }; +}; + +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(); + +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. +int main() +{ + 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); +} \ No newline at end of file _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
