fix: defer await_ready into await_suspend on affected MSVC builds

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<class Awaitable>
+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<R, bool>)
+    {
+        return a.await_suspend(h, env);
+    }
+    else if constexpr (std::is_void_v<R>)
+    {
+        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<typename Awaitable>
 std::coroutine_handle<> call_await_suspend(

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

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<class Promise>
             auto await_suspend(
                 std::coroutine_handle<Promise> 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
             }
         };
 

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<class Promise>
             auto await_suspend(std::coroutine_handle<Promise> 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<R, std::coroutine_handle<>>)
                     return detail::symmetric_transfer(a_.await_suspend(h, p_->environment()));
                 else
                     return a_.await_suspend(h, p_->environment());
+#endif
             }
         };
 

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<int>& 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<int>& 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<int> 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<bool>(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<int> 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<bool>(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();
     }
 };