diff --git a/include/boost/capy/detail/await_suspend_helper.hpp b/include/boost/capy/detail/await_suspend_helper.hpp index eb4672b59..97c7ea159 100644 --- a/include/boost/capy/detail/await_suspend_helper.hpp +++ b/include/boost/capy/detail/await_suspend_helper.hpp @@ -60,15 +60,24 @@ namespace detail { 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` + symmetric transfer, MSVC ARM64 release loses the handler for + a `try` region that spans the suspend point: 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. + the handler failing to match. Any `await_suspend` that returns + a handle triggers it, whether the coroutine resumes at once + or later from another call stack, and whether the throw comes + from `await_resume` or from the body after it. A standalone + program without capy reproduces it on 19.44 and 19.51; the + first clause of the gate already covers 19.44. Debug builds + are unaffected, as is x64 at the same toolset. + `testCatchAfterDeferredResume` in test/unit/task.cpp covers + this. + + The void-returning transfer alone does not cover every case on + 19.51. See `await_ready_then_suspend` below for the second + safeguard that `task` and `quitter` apply where this one is on. `_M_ARM64EC` is included conservatively. It generates ARM64 code and has not been tested here; keeping the workaround on @@ -112,6 +121,72 @@ symmetric_transfer(std::coroutine_handle<> h) noexcept } #endif +#if BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND +/** Ask whether an awaitable is ready, and suspend on it if not. + + Used by the `transform_awaiter` of `task` and `quitter` where the + symmetric-transfer workaround is on. There the awaiter's own + `await_ready` returns `false`, and its `await_suspend` calls this + instead, so the wrapped awaitable's `await_ready` runs inside + `await_suspend` rather than at the start of the `co_await`. + + This avoids a second MSVC ARM64 release defect. When the wrapped + `await_ready` is evaluated at the start of the `co_await`, and + performs an atomic read-modify-write (as + `async_waker::wait_awaiter` does to consume a latched wakeup), a + `try` region spanning the `co_await` can lose its handler if a + call precedes the `try`. This happens even with the workaround + above in place. Observed on 19.51; not on 19.44. A plain or + atomic load in `await_ready` was not affected. The regression + tests are `testCatchAfterRmwAwaitReady` and + `testCatchAroundWakerWait` in test/unit/task.cpp, and + `testCatchAfterRmwAwaitReady` and + `testCatchStopAfterRmwAwaitReady` in test/unit/quitter.cpp. + + The awaiting coroutine continues without suspending when the + wrapped awaitable is ready, or when its `await_suspend` returns + `false`. When the wrapped `await_suspend` returns a handle, that + handle is resumed through `symmetric_transfer` and the awaiting + coroutine stays suspended. Returning `false` instead of resuming + inline keeps immediate completions from growing the stack. + + @param a The wrapped awaitable. + + @param h The coroutine performing the `co_await`. + + @param env The execution environment to pass to the wrapped + awaitable's `await_suspend`. + + @return `false` to continue without suspending, otherwise `true`. +*/ +template +bool +await_ready_then_suspend( + Awaitable& a, + std::coroutine_handle<> h, + await_context const* env) noexcept +{ + // Called exactly once: await_ready may consume state. + if(a.await_ready()) + return false; + using R = decltype(a.await_suspend(h, env)); + if constexpr (std::is_same_v) + { + return a.await_suspend(h, env); + } + else if constexpr (std::is_void_v) + { + a.await_suspend(h, env); + return true; + } + else + { + symmetric_transfer(a.await_suspend(h, env)); + return true; + } +} +#endif + // Helper to normalize await_suspend return types to std::coroutine_handle<> template std::coroutine_handle<> call_await_suspend( diff --git a/include/boost/capy/detail/config.hpp b/include/boost/capy/detail/config.hpp index 83b970396..95461f03c 100644 --- a/include/boost/capy/detail/config.hpp +++ b/include/boost/capy/detail/config.hpp @@ -77,8 +77,14 @@ - 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. + - MSVC ARM64 and ARM64EC, release builds: a try region spanning + the suspend point loses its handler. Reproduced on 19.44 and + 19.51; the first clause already covers 19.44. + + Where it is on, task's and quitter's transform_awaiter also defer + the wrapped await_ready into await_suspend, through + detail::await_ready_then_suspend. That works around a related + ARM64 release defect the void-returning transfer does not cover. Not a configuration option: translation units that disagree on it get different definitions of symmetric_transfer. To check whether diff --git a/include/boost/capy/quitter.hpp b/include/boost/capy/quitter.hpp index 824a481ea..251a1aeea 100644 --- a/include/boost/capy/quitter.hpp +++ b/include/boost/capy/quitter.hpp @@ -351,12 +351,22 @@ struct [[nodiscard]] BOOST_CAPY_CORO_AWAIT_ELIDABLE arrives before an already-complete operation is observed by @ref await_resume, which runs in either case. + Where `BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND` is on, + this always returns `false`, and @ref await_suspend asks + the wrapped awaitable instead. See + `detail::await_ready_then_suspend`. + @return The wrapped awaitable's own `await_ready` result: - `true` if no suspension is needed. + `true` if no suspension is needed. Always `false` where + the workaround is on. */ bool await_ready() noexcept { +#if BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND + return false; +#else return a_.await_ready(); +#endif } /** Restore the frame allocator, check for stop, then resume the @@ -407,16 +417,24 @@ struct [[nodiscard]] BOOST_CAPY_CORO_AWAIT_ELIDABLE @return Whatever the wrapped awaitable's `await_suspend` returns. When that is a `std::coroutine_handle<>`, the handle is routed through `detail::symmetric_transfer`. - On MSVC that helper resumes the handle on the current - stack, and this function returns `void`, so the awaiting - coroutine suspends unconditionally. On every other - compiler the handle is returned unchanged for symmetric - transfer. + Where `BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND` is on, + this function instead returns `bool` from + `detail::await_ready_then_suspend`. It first asks the + wrapped awaitable's `await_ready`. When that reports + ready, it returns `false` to continue without + suspending. Otherwise it resumes any returned handle on + the current stack and returns `true`. Where the + workaround is off, a returned handle is passed back + unchanged for symmetric transfer. */ template auto await_suspend( std::coroutine_handle h) noexcept { +#if BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND + return detail::await_ready_then_suspend( + a_, h, p_->environment()); +#else using R = decltype( a_.await_suspend(h, p_->environment())); if constexpr (std::is_same_v< @@ -426,6 +444,7 @@ struct [[nodiscard]] BOOST_CAPY_CORO_AWAIT_ELIDABLE else return a_.await_suspend( h, p_->environment()); +#endif } }; diff --git a/include/boost/capy/task.hpp b/include/boost/capy/task.hpp index 67f9a8f21..57d0fedef 100644 --- a/include/boost/capy/task.hpp +++ b/include/boost/capy/task.hpp @@ -272,12 +272,22 @@ struct [[nodiscard]] BOOST_CAPY_CORO_AWAIT_ELIDABLE /** Report whether the wrapped awaitable is already complete. + Where `BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND` is on, + this always returns `false`, and @ref await_suspend asks + the wrapped awaitable instead. See + `detail::await_ready_then_suspend`. + @return The wrapped awaitable's own `await_ready` result: - `true` if no suspension is needed. + `true` if no suspension is needed. Always `false` where + the workaround is on. */ bool await_ready() noexcept { +#if BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND + return false; +#else return a_.await_ready(); +#endif } /** Restore the frame allocator, then resume the wrapped @@ -313,20 +323,28 @@ struct [[nodiscard]] BOOST_CAPY_CORO_AWAIT_ELIDABLE @return Whatever the wrapped awaitable's `await_suspend` returns. When that is a `std::coroutine_handle<>`, the handle is routed through `detail::symmetric_transfer`. - On MSVC that helper resumes the handle on the current - stack, and this function returns `void`, so the awaiting - coroutine suspends unconditionally. On every other - compiler the handle is returned unchanged for symmetric - transfer. + Where `BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND` is on, + this function instead returns `bool` from + `detail::await_ready_then_suspend`. It first asks the + wrapped awaitable's `await_ready`. When that reports + ready, it returns `false` to continue without + suspending. Otherwise it resumes any returned handle on + the current stack and returns `true`. Where the + workaround is off, a returned handle is passed back + unchanged for symmetric transfer. */ template auto await_suspend(std::coroutine_handle h) noexcept { +#if BOOST_CAPY_SYMMETRIC_TRANSFER_WORKAROUND + return detail::await_ready_then_suspend(a_, h, p_->environment()); +#else using R = decltype(a_.await_suspend(h, p_->environment())); if constexpr (std::is_same_v>) return detail::symmetric_transfer(a_.await_suspend(h, p_->environment())); else return a_.await_suspend(h, p_->environment()); +#endif } }; diff --git a/test/unit/quitter.cpp b/test/unit/quitter.cpp index 8c83c76b2..904b9f12e 100644 --- a/test/unit/quitter.cpp +++ b/test/unit/quitter.cpp @@ -866,6 +866,138 @@ struct quitter_test BOOST_TEST(!reached_end); } + //---------------------------------------------------------- + // 15. Catch around a co_await whose await_ready does an atomic + // read-modify-write (MSVC 14.51 ARM64 release regression) + // + // quitter's transform_awaiter gets the same safeguard as + // task's: where the symmetric-transfer workaround is on, the + // wrapped await_ready is asked from inside await_suspend. See + // testCatchAfterRmwAwaitReady in test/unit/task.cpp. Both + // tests below pass on every other build with or without it. + //---------------------------------------------------------- + + // An opaque call before the try; the defect needs one there. + static BOOST_CAPY_NOINLINE void + call_before_try(bool& marker) + { + marker = true; + } + + // Parks; await_ready performs a compare-exchange that never + // succeeds here, as async_waker's does. await_resume throws when + // throw_on_resume is set. + struct rmw_ready_awaitable + { + std::coroutine_handle<>& slot; + std::atomic& state; + bool throw_on_resume; + + bool await_ready() noexcept + { + int expected = 1; + return state.compare_exchange_strong( + expected, 0, + std::memory_order_acq_rel, + std::memory_order_acquire); + } + + std::coroutine_handle<> + await_suspend( + std::coroutine_handle<> h, await_context const*) noexcept + { + slot = h; + return std::noop_coroutine(); + } + + void await_resume() const + { + if(throw_on_resume) + throw test_exception("rmw ready"); + } + }; + + static quitter<> + rmw_ready_quitter( + std::coroutine_handle<>& slot, + std::atomic& state, + bool throw_on_resume, + bool& marker, + bool& caught) + { + call_before_try(marker); + try + { + co_await rmw_ready_awaitable{slot, state, throw_on_resume}; + } + catch(...) + { + caught = true; + } + } + + // The awaitable's own await_resume throws. + void + testCatchAfterRmwAwaitReady() + { + int dispatch_count = 0; + test_executor ex(dispatch_count); + + std::coroutine_handle<> parked; + std::atomic state{0}; + bool marker = false; + bool caught = false; + bool escaped = false; + bool completed = false; + + run_async(ex, + [&]() { completed = true; }, + [&](std::exception_ptr) { escaped = true; })( + rmw_ready_quitter(parked, state, true, marker, caught)); + + BOOST_TEST(static_cast(parked)); + BOOST_TEST(marker); + if(parked) + parked.resume(); + + BOOST_TEST(caught); + BOOST_TEST(!escaped); + BOOST_TEST(completed); + } + + // A stop request while parked: quitter's await_resume throws its + // stop sentinel, which the catch(...) in the body takes, as in + // the documentation's catch(...) pitfall. + void + testCatchStopAfterRmwAwaitReady() + { + int dispatch_count = 0; + test_executor ex(dispatch_count); + + std::stop_source source; + std::coroutine_handle<> parked; + std::atomic state{0}; + bool marker = false; + bool caught = false; + bool escaped = false; + bool completed = false; + + run_async(ex, source.get_token(), + [&]() { completed = true; }, + [&](std::exception_ptr) { escaped = true; })( + rmw_ready_quitter(parked, state, false, marker, caught)); + + BOOST_TEST(static_cast(parked)); + BOOST_TEST(marker); + source.request_stop(); + if(parked) + parked.resume(); + + BOOST_TEST(caught); + BOOST_TEST(!escaped); + BOOST_TEST(completed); + } + //---------------------------------------------------------- // 13. Echo server with shutdown // @@ -989,6 +1121,8 @@ struct quitter_test testWhenAllWithStop(); testWhenAnyWithStop(); testWakerCancellation(); + testCatchAfterRmwAwaitReady(); + testCatchStopAfterRmwAwaitReady(); testEchoWithShutdown(); } }; diff --git a/test/unit/task.cpp b/test/unit/task.cpp index f26537bcc..003ff9c8c 100644 --- a/test/unit/task.cpp +++ b/test/unit/task.cpp @@ -1,5 +1,6 @@ // // Copyright (c) 2025 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) @@ -11,6 +12,7 @@ #include #include +#include #include #include #include @@ -1631,6 +1633,50 @@ struct task_test int await_resume() noexcept { return 7; } }; + // Not ready, and await_suspend returns void: the coroutine stays + // suspended until the test resumes it. Where the symmetric-transfer + // workaround is on, this is the void branch of + // detail::await_ready_then_suspend. + struct void_suspend_awaitable + { + std::coroutine_handle<>& slot; + + bool await_ready() const noexcept { return false; } + + void await_suspend( + std::coroutine_handle<> h, await_context const*) noexcept + { + slot = h; + } + + int await_resume() const noexcept { return 7; } + }; + + static task + void_suspend_session(std::coroutine_handle<>& slot) + { + co_return co_await void_suspend_awaitable{slot}; + } + + void + testVoidSuspendAwaitable() + { + int dispatch_count = 0; + test_executor ex(dispatch_count); + + std::coroutine_handle<> parked; + int result = 0; + + run_async(ex, [&](int v) { result = v; })( + void_suspend_session(parked)); + + BOOST_TEST(static_cast(parked)); + BOOST_TEST_EQ(result, 0); + if(parked) + parked.resume(); + BOOST_TEST_EQ(result, 7); + } + void testBoolSuspendAwaitable() { @@ -1732,6 +1778,155 @@ struct task_test BOOST_TEST(completed); } + // An opaque call placed before the try in the two tests below. + // The MSVC ARM64 defect they cover needs a call there. + static BOOST_CAPY_NOINLINE void + call_before_try(bool& marker) + { + marker = true; + } + + // Parks like parking_awaitable, but its await_ready performs an + // atomic read-modify-write that never succeeds here, as + // async_waker's does to consume a latched wakeup. await_resume + // throws. + struct rmw_ready_awaitable + { + std::coroutine_handle<>& slot; + std::atomic& state; + + bool await_ready() noexcept + { + int expected = 1; + return state.compare_exchange_strong( + expected, 0, + std::memory_order_acq_rel, + std::memory_order_acquire); + } + + std::coroutine_handle<> + await_suspend( + std::coroutine_handle<> h, await_context const*) noexcept + { + slot = h; + return std::noop_coroutine(); + } + + void await_resume() const + { + throw_test_exception("rmw ready"); + } + }; + + static task + rmw_ready_session( + std::coroutine_handle<>& slot, + std::atomic& state, + bool& marker, + bool& caught) + { + call_before_try(marker); + try + { + co_await rmw_ready_awaitable{slot, state}; + } + catch(test_exception const&) + { + caught = true; + } + } + + // Regression test for a second MSVC 14.51 ARM64 release defect, + // which the void-returning symmetric_transfer does not cover. + // + // With a call before the try, and an awaiter whose await_ready + // performs an atomic read-modify-write, the throw from + // await_resume was not caught by the catch beside it. It + // escaped to run_async's error handler. A plain or atomic load + // in await_ready did not trigger it. transform_awaiter avoids it + // by asking the wrapped await_ready from inside await_suspend + // (detail::await_ready_then_suspend). + // + // As with testCatchAfterDeferredResume, the escape is checked + // through the error handler so it cannot terminate the binary. + void + testCatchAfterRmwAwaitReady() + { + int dispatch_count = 0; + test_executor ex(dispatch_count); + + std::coroutine_handle<> parked; + std::atomic state{0}; + bool marker = false; + bool caught = false; + bool escaped = false; + bool completed = false; + + run_async(ex, + [&]() { completed = true; }, + [&](std::exception_ptr) { escaped = true; })( + rmw_ready_session(parked, state, marker, caught)); + + BOOST_TEST(static_cast(parked)); + BOOST_TEST(marker); + if(parked) + parked.resume(); + + BOOST_TEST(caught); + BOOST_TEST(!escaped); + BOOST_TEST(completed); + } + + static task + waker_session( + async_waker& w, + bool& marker, + bool& caught) + { + call_before_try(marker); + try + { + auto [ec] = co_await w.wait(); + (void)ec; + throw_test_exception("woken"); + } + catch(test_exception const&) + { + caught = true; + } + } + + // The same defect through async_waker, the shape that first + // exposed it: a call before the try, then a wait. On MSVC 14.51 + // ARM64 release the throw after a successful wait escaped the + // catch. test_executor posts inline, so wake() resumes the + // session on this stack. + void + testCatchAroundWakerWait() + { + int dispatch_count = 0; + test_executor ex(dispatch_count); + + async_waker w; + bool marker = false; + bool caught = false; + bool escaped = false; + bool completed = false; + + run_async(ex, + [&]() { completed = true; }, + [&](std::exception_ptr) { escaped = true; })( + waker_session(w, marker, caught)); + + BOOST_TEST(marker); + BOOST_TEST(!caught); + w.wake(); + + BOOST_TEST(caught); + BOOST_TEST(!escaped); + BOOST_TEST(completed); + } + void run() { @@ -1742,7 +1937,10 @@ struct task_test testDestroyUnstartedOrFinishedTaskIsFine(); #endif testCatchAfterDeferredResume(); + testCatchAfterRmwAwaitReady(); + testCatchAroundWakerWait(); testBoolSuspendAwaitable(); + testVoidSuspendAwaitable(); testReturnValue(); testException(); testTaskAwaitsTask();