[coroutines] Clang generates incorrect code for throwing promise.unhandled_exception()

[lewissbaker]

The wording for coroutines in [dcl.fct.def.coroutine] for unhandled_exception() says that:

If the evaluation of the expression promise.unhandled_exception() exits via an exception, the coroutine is considered suspended at the final suspend-point and the exception propagates to the caller or resumer.

However, it seems that the current codegen for Clang does not set the suspend point to be at a final suspend point in this case. Instead, if a coroutine exits the call to unhandled_exception() with an exception, which you then catch and then destroy the coroutine, the coroutine behaves as if the coroutine was still suspended at the last suspend-point it suspended at. i.e. it does not seem to be writing a new suspend-point index to the coroutine-state if unhandled_exception() exits with an exception, instead it leaves the last-written suspend-point index value unchanged.

This can lead to unexpected behaviour. e.g. double-destruction of variables in-scope at the last suspend-point.

For example: Compile the following code with Clang 16.0 and the compiler args -std=c++20 -stdlib=libc++ -O2

#include <coroutine>
#include <cstdio>

struct generator {
    struct promise_type {
        generator get_return_object() noexcept { return generator{std::coroutine_handle<promise_type>::from_promise(*this)}; }
        void unhandled_exception() { throw; }
        void return_void() noexcept {}
        std::suspend_always yield_value(int) noexcept { return {}; }
        std::suspend_always final_suspend() noexcept { return {}; }
        std::suspend_always initial_suspend() noexcept { return {}; }
    };

    explicit generator(std::coroutine_handle<promise_type> h) noexcept
    : coro(h)
    {}

    generator(generator&& g) noexcept
    : coro(g.coro)
    {
        g.coro = {};
    }

    ~generator() {
        if (coro)
            coro.destroy();
    }

    std::coroutine_handle<promise_type> coro;
};

struct RAII {
    __attribute__((noinline)) RAII() noexcept { std::puts("ctor"); }
    __attribute__((noinline)) ~RAII() { std::puts("dtor"); }
};

struct X {};

static generator example() {
    RAII raii;
    co_yield 0;
    throw X{};
}

int main() {
    generator g = example();
    g.coro.resume();
    try {
        g.coro.resume();
    } catch (X) {
        std::puts("caught X");
    }
}

Compiler-Explorer: https://godbolt.org/z/xqY4bbPTW

The expected output is:

ctor
dtor
caught X

The observed output under Clang 16 is:

ctor
dtor
caught X
dtor

[llvmbot]

@llvm/issue-subscribers-coroutines

[ChuanqiXu9]

If I call coro.done() after try-catch statement, it will return true as expected, which implies that the coroutine is suspended at the final suspend point actually. https://godbolt.org/z/rbK8Wd8dT. But it looks indeed the state of coroutines are broken.

[ChuanqiXu9]

Update to myself: although I solved the O0 case, the optimized case is more complex than I thought. Because:

The coroutine will be transformed to (roughly):

try {
  coro-body;
} catch(...) {
  unhandled_exception();
}

final_suspend:
   ...

In the reproducer, the optimizer found that the unhandled_exception() here will always throw after inlining. So the final suspend won't be reached. So the optimizer removes the final_suspend part in the early places. Maybe I need to find something tricky to solve this.

[ChuanqiXu9]

Note to my self: given the transformation from http://eel.is/c++draft/dcl.fct.def.coroutine, the destroy function from the final suspend point should destroy the same thing with the initial suspend point. This may help us to handle the case that the final suspend is optimized out. However, it is problematic since the optimizier don't have knowledge about first suspend and final suspend point.

So the best method may still be to add a coro intrinsic to teach the middle end about the semantics of final suspend to not optimize it out.

[lewissbaker]

Note that destroying at the final suspend point may have different behaviour compared to the initial suspend point depending on the awaitable object. The initial suspend point may have one temporary awaiter returned from operator co_await() (or indeed a temporary returned from initial_suspend() itself) and the final-suspend point may have a different type of temporary awaitable/awaiter used in the co_await promise.final_suspend() expression. And this may be different again from the final suspend point that is reached when unhandled_exception() throws, where there will be no such temporary awaitable/awaiter.

The destroy() behaviour from all of these suspend points will have commonalities - they all end up eventually executing the sequence of promise destruction, parameter destruction and frame deallocation.

[ChuanqiXu9]

Thanks for correcting me.

While this is relatively easy to fix: we can fix it by making unhandled_exception() noinline, or we can add intrinsics to make the final_suspend to not be removed, etc... the reason why I put this so long is that the condition to trigger the bug is super critical and I don't like to add costs for a corner case.

The condition to trigger the bug including:

• The function body must be always-throw statically.
• The unhandled_exception() function need to be always-throw statically too.

Then the optimizer found the final_suspend part is never executed then it got removed. Then, I'd like to see this is really a corner case. https://godbolt.org/z/Tsj7Th41s

So I am still trying to find a light-weight solution to users so that the users who don't use such pattern won't get worse code than before.

[ChuanqiXu9]

Inspired by #64945, the best solution may be to introduce something like @llvm.coro.unhandled_exception(%promise, @unhandled_exception), and if:

• the @llvm.coro.unhandled_exception(...) call dominates all the terminators of the function. e.g., in another word, we know the unhandled_exception must be executed in the middle end.
• the @unhandled_exception may throw exceptions.

then we will add special information for the final suspend to make it not to be optimized out. Otherwise, we will convert the call to @llvm.coro.unhandled_exception(...) to a call to @unhandled_exception directly in the CoroEarly phase. Then everything will be fine. This may be the most promising solution we got now.