diff --git a/include/boost/capy/detail/await_suspend_helper.hpp b/include/boost/capy/detail/await_suspend_helper.hpp index 35837254a..eb4672b59 100644 --- a/include/boost/capy/detail/await_suspend_helper.hpp +++ b/include/boost/capy/detail/await_suspend_helper.hpp @@ -1,6 +1,7 @@ // // Copyright (c) 2025 Vinnie Falco (vinnie.falco@gmail.com) // Copyright (c) 2026 Steve Gerbino +// Copyright (c) 2026 Michael Vandeberg // // Distributed under the Boost Software License, Version 1.0. (See accompanying // file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) @@ -13,6 +14,7 @@ #include #include +#include #include @@ -20,7 +22,7 @@ namespace boost { namespace capy { namespace detail { -/** Perform symmetric transfer, working around an MSVC codegen bug. +/** Perform symmetric transfer, working around MSVC codegen bugs. MSVC stores the `std::coroutine_handle<>` returned from `await_suspend` in a hidden `__$ReturnUdt$` variable located @@ -40,19 +42,61 @@ namespace detail { frame before the runtime reads `__$ReturnUdt$` (e.g. `boundary_trampoline` final_suspend). - On MSVC this function calls `h.resume()` on the current stack - and returns `void`, causing unconditional suspension. The - trade-off is O(n) stack growth instead of O(1) tail-calls. - - On other compilers the handle is returned directly for proper - symmetric transfer. + On affected compilers this function calls `h.resume()` on the + current stack and returns `void`, causing unconditional + suspension. The trade-off is O(n) stack growth instead of + O(1) tail-calls. + + This defect affects MSVC 19.34 through 19.44 on every target + and is fixed in MSVC 19.50 (VS 2026 / 18.0). Measured on + 19.44 the caller builds the hidden return slot at + `__coro_frame_ptr$ + 0xC0`, on the coroutine frame; on 19.51 + it is an `rsp`-relative stack temporary, so destroying the + frame no longer invalidates it. + + Do not widen this gate on the basis of Developer Community + ticket 10251975, tagged "Fixed in VS 2022 17.9 Preview 2"; + 19.39 reproduces the fault identically to 19.34. + + On ARM64 the workaround does not retire at 19.50, because + that target has a second, unrelated defect. With a real + symmetric transfer, MSVC 19.51 release loses the handler for + a `try` region that spans the suspend point: after the + coroutine is resumed from another call stack, a `throw` + inside that region is not caught by the `catch` beside it and + escapes to the promise's `unhandled_exception`. A `catch(...)` + misses it too, so the region is not found at all rather than + the handler failing to match. Debug builds are unaffected, as + is x64 at the same toolset. `testCatchAfterDeferredResume` in + test/unit/task.cpp covers this. + + `_M_ARM64EC` is included conservatively. It generates ARM64 + code and has not been tested here; keeping the workaround on + is the safe direction, since it costs stack depth rather than + correctness. + + The gate deliberately excludes Clang by testing + `BOOST_CAPY_MSVC` rather than `_MSC_VER`. Both `clang-cl` and + `clang++` targeting Windows define `_MSC_VER` for ABI + compatibility, but generate a correct tail-call. + + Note that a probe which merely poisons the destroyed frame + cannot validate this gate. Routing the return through this + function moves the frame write to after `destroy()`, which + repairs the poison pattern and hides the defect. The + regression test in + test/unit/detail/await_suspend_helper.cpp unmaps the frame + instead, so any post-destroy access faults. + + On unaffected compilers the handle is returned directly for + proper symmetric transfer. Callers must use `auto` return type on their `await_suspend` so the return type adapts per platform. @param h The coroutine handle to transfer to. */ -#if BOOST_CAPY_WORKAROUND(_MSC_VER, >= 1) +#if BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND inline void symmetric_transfer(std::coroutine_handle<> h) noexcept { // safe_resume is not needed here: the calling coroutine is diff --git a/include/boost/capy/detail/config.hpp b/include/boost/capy/detail/config.hpp index 791a239d0..83b970396 100644 --- a/include/boost/capy/detail/config.hpp +++ b/include/boost/capy/detail/config.hpp @@ -1,5 +1,6 @@ // // Copyright (c) 2023 Vinnie Falco (vinnie.falco@gmail.com) +// Copyright (c) 2026 Michael Vandeberg // // Distributed under the Boost Software License, Version 1.0. (See accompanying // file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) @@ -27,12 +28,26 @@ corresponding _WORKAROUND_GUARD macro is 1, which makes BOOST_CAPY_WORKAROUND evaluate to 0 on that compiler. + BOOST_CAPY_MSVC is _MSC_VER on Microsoft's compiler only. Clang + targeting Windows (clang-cl, and clang++ with an MSVC target) + also defines _MSC_VER for ABI compatibility but does not share + MSVC's codegen bugs, so an MSVC bug workaround tests + BOOST_CAPY_MSVC rather than _MSC_VER. + Usage: - #if BOOST_CAPY_WORKAROUND(_MSC_VER, >= 1) // any MSVC - #if BOOST_CAPY_WORKAROUND(_MSC_VER, <= 1900) // MSVC 14.0 and earlier - #if BOOST_CAPY_WORKAROUND(__GNUC__, < 12) // GCC before 12 + #if BOOST_CAPY_WORKAROUND(BOOST_CAPY_MSVC, >= 1) // any MSVC, not Clang + #if BOOST_CAPY_WORKAROUND(BOOST_CAPY_MSVC, < 1950) // MSVC before 19.50 + #if BOOST_CAPY_WORKAROUND(_MSC_VER, <= 1900) // MSVC 14.0 and earlier + #if BOOST_CAPY_WORKAROUND(__GNUC__, < 12) // GCC before 12 */ +#if defined(_MSC_VER) && !defined(__clang__) +# define BOOST_CAPY_MSVC _MSC_VER +# define BOOST_CAPY_MSVC_WORKAROUND_GUARD 0 +#else +# define BOOST_CAPY_MSVC_WORKAROUND_GUARD 1 +#endif + #ifndef _MSC_VER # define _MSC_VER_WORKAROUND_GUARD 1 #else @@ -55,6 +70,29 @@ ((symbol ## _WORKAROUND_GUARD + 0 == 0) && \ (symbol != 0) && (1 % (( (symbol test) ) + 1))) +/* Selects the void-returning symmetric_transfer on MSVC toolsets + whose real symmetric transfer is miscompiled. Two unrelated + defects, both described on detail::symmetric_transfer + in detail/await_suspend_helper.hpp: + + - MSVC 19.34 through 19.44, every target: the returned handle + is stored on the coroutine frame. + - MSVC 19.50 and later, ARM64 and ARM64EC, release builds: a + try region spanning the suspend point loses its handler. + + Not a configuration option: translation units that disagree on it + get different definitions of symmetric_transfer. To check whether + a newer toolset still needs it, edit this gate locally; the + regression tests pass trivially while the workaround is on. +*/ +#if BOOST_CAPY_WORKAROUND(BOOST_CAPY_MSVC, < 1950) || \ + (defined(BOOST_CAPY_MSVC) && \ + (defined(_M_ARM64) || defined(_M_ARM64EC))) +# define BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND 1 +#else +# define BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND 0 +#endif + // MSVC warning suppression helpers. // On MSVC these expand to __pragma(); elsewhere they are empty. #ifdef _MSC_VER diff --git a/include/boost/capy/ex/frame_allocator.hpp b/include/boost/capy/ex/frame_allocator.hpp index 34affe4c3..2fc0d2415 100644 --- a/include/boost/capy/ex/frame_allocator.hpp +++ b/include/boost/capy/ex/frame_allocator.hpp @@ -59,7 +59,7 @@ current_frame_allocator_ref() noexcept; // MSVC may keep an inlined thread_local's address in a coroutine // frame across a suspension, so a resumption on another thread // would write the first thread's slot. -#if BOOST_CAPY_WORKAROUND(_MSC_VER, >= 1) && !defined(__clang__) +#if BOOST_CAPY_WORKAROUND(BOOST_CAPY_MSVC, >= 1) BOOST_CAPY_NOINLINE #endif inline std::pmr::memory_resource*& diff --git a/test/unit/detail/await_suspend_helper.cpp b/test/unit/detail/await_suspend_helper.cpp index e33236662..01e92796c 100644 --- a/test/unit/detail/await_suspend_helper.cpp +++ b/test/unit/detail/await_suspend_helper.cpp @@ -13,6 +13,14 @@ #include #include +#include +#include + +#ifdef _WIN32 +# include +#else +# include +#endif #include "test_suite.hpp" @@ -20,6 +28,111 @@ namespace boost { namespace capy { namespace detail { +namespace { + +bool probe_continuation_ran = false; + +// Coroutine frames for the destroy-then-transfer probe, allocated so +// that destroying a frame UNMAPS it. Any access to the frame after +// destroy - read or write - then faults. +// +// A poisoning allocator is not sufficient. Routing the return through +// symmetric_transfer moves the compiler's write of the handle into the +// frame slot to after the frame is destroyed, which repairs a poison +// pattern and hides the defect. Unmapping cannot be repaired, so this +// probe sees the frame access that poisoning misses. +struct probe_frame +{ + static void* allocate(std::size_t n) noexcept + { + #ifdef _WIN32 + return ::VirtualAlloc( + nullptr, n, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE); + #else + void* const p = ::mmap(nullptr, n, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + return p == MAP_FAILED ? nullptr : p; + #endif + } + + static void release(void* p, std::size_t n) noexcept + { + #ifdef _WIN32 + (void)n; + ::VirtualFree(p, 0, MEM_RELEASE); + #else + ::munmap(p, n); + #endif + } +}; + +struct probe_task +{ + struct promise_type + { + void* operator new(std::size_t n) + { + void* const p = probe_frame::allocate(n); + if(! p) + throw std::bad_alloc(); + return p; + } + + void operator delete(void* p, std::size_t n) noexcept + { + probe_frame::release(p, n); + } + + probe_task get_return_object() noexcept + { + return { std::coroutine_handle< + promise_type>::from_promise(*this) }; + } + + std::suspend_always initial_suspend() noexcept { return {}; } + std::suspend_always final_suspend() noexcept { return {}; } + void return_void() noexcept {} + void unhandled_exception() noexcept { BOOST_TEST(false); } + }; + + std::coroutine_handle h; +}; + +// Mirrors the final_suspend awaiters in when_all_runner and +// when_any_runner: destroy our own frame, then transfer to the +// continuation. auto return type, because symmetric_transfer +// returns void on the workaround path. +struct destroy_then_transfer +{ + std::coroutine_handle<> next; + + bool await_ready() const noexcept { return false; } + + auto await_suspend(std::coroutine_handle<> self) noexcept + { + // Copy to the stack first: this awaiter lives on the frame + // about to be destroyed. + auto const continuation = next; + self.destroy(); + return symmetric_transfer(continuation); + } + + void await_resume() const noexcept {} +}; + +probe_task probe_continuation() +{ + probe_continuation_ran = true; + co_return; +} + +probe_task probe_victim(std::coroutine_handle<> next) +{ + co_await destroy_then_transfer{ next }; +} + +} // (anon) + class await_suspend_helper_test { // await_suspend returning void: caller suspends unconditionally. @@ -55,9 +168,37 @@ class await_suspend_helper_test }; public: + // capy#378: transferring out of an await_suspend that has already + // destroyed its own frame must not touch that frame afterwards. + // On affected MSVC toolsets symmetric_transfer resumes on the + // current stack to avoid the frame round-trip; elsewhere it + // performs a real tail-call. Either way the continuation must run + // and the process must survive. + // + // When the gate is wrong for the compiler in use this fails as a + // hard access violation rather than a BOOST_TEST failure. Each + // test runs in its own process under CTest, so the crash is + // reported as this test failing. + void + testDestroyThenTransfer() + { + probe_continuation_ran = false; + + auto cont = probe_continuation(); + auto victim = probe_victim(cont.h); + + victim.h.resume(); + + BOOST_TEST(probe_continuation_ran); + + cont.h.destroy(); + } + void run() { + testDestroyThenTransfer(); + auto const h = std::noop_coroutine(); { diff --git a/test/unit/task.cpp b/test/unit/task.cpp index 7c9cdecca..f26537bcc 100644 --- a/test/unit/task.cpp +++ b/test/unit/task.cpp @@ -1642,6 +1642,96 @@ struct task_test BOOST_TEST_EQ(result, 7); } + // Suspends by parking the handle and returning noop_coroutine, so + // the coroutine resumes later from whatever stack calls resume(), + // rather than being transferred into inline. This is the shape of + // an async read that completes on the io_context thread. + struct parking_awaitable + { + std::coroutine_handle<>& slot; + + bool await_ready() const noexcept { return false; } + + std::coroutine_handle<> + await_suspend( + std::coroutine_handle<> h, await_context const*) const noexcept + { + slot = h; + return std::noop_coroutine(); + } + + // Returning a value the body tests before throwing mirrors + // `auto [ec, n] = co_await sock.read_some(buf); if(ec) throw;`. + bool await_resume() const noexcept { return true; } + }; + + // The body whose catch must run. Kept as a coroutine taking + // references rather than a capturing lambda so the shape matches + // the corosio session function this reproduces. + static task + deferred_throw_session( + std::coroutine_handle<>& slot, + bool& caught) + { + try + { + if(co_await parking_awaitable{slot}) + throw_test_exception("deferred resume"); + } + catch(test_exception const&) + { + caught = true; + } + } + + // Regression test for an MSVC 14.51 ARM64 release codegen bug. + // + // When a co_await whose await_suspend returns a coroutine_handle + // suspends, and the coroutine is later resumed from a different + // call stack, a throw in the same try block was not caught by the + // catch beside it. The exception escaped to the promise's + // unhandled_exception and reached run_async's error handler; in + // corosio's 3a_echo_server documentation test it terminated the + // process. + // + // detail::symmetric_transfer hides the bug wherever its workaround + // is enabled, because it resumes on the current stack and returns + // void instead of performing a real symmetric transfer. So this + // test passes on every compiler the workaround covers, and is only + // meaningful where it does not. + // + // The escape is checked through run_async's error handler rather + // than allowed to propagate: an uncaught exception here would + // terminate the whole test binary and hide every other result. + void + testCatchAfterDeferredResume() + { + int dispatch_count = 0; + test_executor ex(dispatch_count); + + std::coroutine_handle<> parked; + bool caught = false; + bool escaped = false; + bool completed = false; + + run_async(ex, + [&]() { completed = true; }, + [&](std::exception_ptr) { escaped = true; })( + deferred_throw_session(parked, caught)); + + // The awaitable parked the handle and nothing has thrown yet. + BOOST_TEST(static_cast(parked)); + BOOST_TEST(!caught); + + // Resume from here, not from inside await_suspend: arriving on + // an unrelated call stack is what the bug needs. + parked.resume(); + + BOOST_TEST(caught); + BOOST_TEST(!escaped); + BOOST_TEST(completed); + } + void run() { @@ -1651,6 +1741,7 @@ struct task_test testMoveAssignOverLentTaskTerminates(); testDestroyUnstartedOrFinishedTaskIsFine(); #endif + testCatchAfterDeferredResume(); testBoolSuspendAwaitable(); testReturnValue(); testException();