C++20 coroutine capture with reference weird crash with `unique_ptr`

Question

Here is a code that crash when, in main, line (2) version is used (and line (1) is commented). Weird enough, this code compiles fines with a simple replacement implementation (line (1)) that mimic the behavior of line (2). Of course, if it's an undefined behavior, it can't have a good explanation, but I don't understand why it crashes. Basically, it's a generator implementation coroutine in C++, tested with captures by references. It works always, except when used with unique_ptr (raw pointer works). Just, why ?

#include <iostream>
#include <coroutine>
#include <cassert>
#include <optional>
#include <memory>

using std::cout;
using std::endl;

template<typename T>
class generator
{
public:
    struct promise_type
    {
        std::optional<T> t_;

        promise_type() = default;
        ~promise_type() = default;

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

        std::suspend_always yield_value(T t) { t_ = t; return {}; }
        void return_void() {}
    };
private:
    std::coroutine_handle<promise_type> h_;

    generator(std::coroutine_handle<promise_type> h) : h_(h) {}

public:
    generator() = default;

    // ------ Prevent copies
    generator(const generator&) = delete;
    generator& operator=(const generator&) = delete;

    // ------ Allow moves
    generator(generator&& other) noexcept
        : h_(move(other.h_)) // move may be unnecessary, coroutine_handle acts like a lightweight pointer
    {
        other.h_ = {}; // Unlink handle in moved generator
                       // move() does not guarantee to destroy original value
    }

    generator& operator=(generator&& other) noexcept
    {
        h_ = move(other.h_);
        other.h_ = {};
        return *this;
    }

    ~generator()
    {
        if(h_)
        {
            h_.destroy();
            h_ = {};
        }
    }

    bool is_resumable() const
    {
        return h_ && !h_.done();
    }

    bool operator()()
    {
        return resume();
    }

    bool resume()
    {
        assert(is_resumable());

        h_();

        return !h_.done();
    }

    [[nodiscard]] const T& get() const
    {
        return h_.promise().t_.value();
    }

    [[nodiscard]] T& get() // Allow movable
    {
        return h_.promise().t_.value();
    }
};

struct F
{
    /*F(const std::function<generator<int>()>& del)
    {
        handle = del();
    }*/

    template<typename T>
    F(T del)
    {
        handle = del();
    }

    ~F() { cout << "dtor" << endl; }

    generator<int> handle;
};

template<typename T>
struct UniquePtr
{
    UniquePtr(T* t) : t_(t) {}

    UniquePtr(UniquePtr&&) = delete;
    UniquePtr(const UniquePtr&) = delete;
    UniquePtr& operator=(UniquePtr&&) = delete;
    UniquePtr& operator=(const UniquePtr&) = delete;

    ~UniquePtr() { delete t_; }

    T* operator->() const { return t_;}

private:
    T* t_;
};

int main()
{
    int x = 10;
    auto a = [&]() -> generator<int> {
        x = 20;
        co_yield x;
    };

    //UniquePtr<F> ptr(new F(a)); // (1)
    std::unique_ptr<F> ptr(new F(a)); // (2)

    generator<int>& gen = ptr->handle;
    gen();
    cout << gen.get() << "/" << x << endl;

    return 0;
}

EDIT. :

It crashes also a Godbolt (error 139), here is the link : https://godbolt.org/z/cWYY8PKx4. Maybe is it a gcc implementation problem, around std::unique_ptr optimizations? I can't test on other compilers on Godbolt, there is no support for coroutines on clang.

Answer 1

While still unclear why it happens, it appears that std::suspend_always initial_suspend() { return {}; } when used in conjunction with a lambda and combined with optimizations for scoped unique-ptrs also incorrectly performs the parameter capture in the wrong scope.

Slighly modified variant:

struct F
{
    template<typename T>
    F(T del)
    {
        auto y = 0;
        cout << "&y   = " << &y << endl;
        handle = del();
    }

    ~F() { cout << "dtor" << endl; }

    generator<int> handle;
};

int main()
{
    int x = 10;
    cout << "&x   = " << &x << endl;
    auto a = [&]() -> generator<int> {
        cout << "[&x] = " << &x << endl;
        co_yield x;
    };

    auto ptr = std::make_unique<F>(a);

    generator<int>& gen = ptr->handle;
    gen();

    a()();

    return 0;
}

Output:

&x   = 0x7fff46a718cc
&y   = 0x7fff46a71834
[&x] = 0x7fff46a71840
[&x] = 0x7fff46a718cc
dtor

The 2nd and 3rd output points within the stackframe of the constructor of F(). Which is only expected for &y though, the last one performs correctly.

It also does behave correctly when going with std::suspend_never initial_suspend() { return {}; }, or when casting to an explicit std::function, indicating a bug in the capture mechanic.

Depending on the optimization level, the captured scope is either this of F or the stack frame of the constructor of F().

Answer 2

I think I figured out why it crashes. Without the details, I will try to explain it what happens step by step.

Let's focus on this part:

    int x = 10;
    auto a = [&]() -> generator<int> {
        x = 20;
        co_yield x;
    };

We may be used to these lambdas, but sometimes we forget the basics and what happens really, and we may get fooled. Firstly, replace the lambda syntaxic sugar by the explicit final equivalent.

    int x = 10;
    struct A {
        int &x;
        A(int& x) : x(x) {}
        generator<int> operator()() {
            x = 20;
            co_yield x;
        }
    }
    A a(x);

Then, things become more clearer. Now, focus on this part.

    template<typename T>
    F(T del)
    {
        handle = del();
    }

After template parsing, this will effectively do this code:

    F(A del)
    {
        handle = del();
    }

Invoking del(), we create a new coroutine but as explained later, the coroutine refers to variables of the structure A. When we leave the scope of F(), del is destroyed. Now, when we do it:

    A a(x);
    F f(a);
    f.handle();

This is essentially the same as replacing f.handle(); by:

    del.x = 20;

This snippet of code is short, but need explanation since the variable del does not exist here, it is for semantic illustration. Let's explain it: We assign to the member reference x of del the value 20. For the moment, it is just obvious translation. But what is del ? It refers to the variable created in the constructor of F. In C++, this is implicitly used for looking up variables, which we usually abuse when using lambdas like with x = 20; co_yield x; (otherwise, lambdas would be way less useful) and which here is just del. But since we are outside the constructor of f, del does not exists anymore.

Yes, but since the variable is captured by reference we should be fine here, isn't it ?

Actually no, because yes, x is captured by reference, and this reference exists. In C++, references can translate to pointer, or even nothing at all. In this situation, since we store a reference in a class member, the reference is probably compiled into a raw pointer. And it points to a value that actually exists when we need it. So, what's the problem ? The pointed value exists, but the pointer actually does not exist anymore. This is the opposite of usual runtime errors and a confusing one, because the pointee is not in cause. It wasn't actually explicited but the type of del here is, semantically, a reference to the del argument in the constructor of F, which does not exist anymore. Hence, the error appears obvious.

I am not an expert so I don't if it is right, but with the tests and alternatives I use I am pretty sure this is what happen under the hood. I tried related code with MSVC and it also crashs. It is not a compiler bug. The fact it works sometime and not with some std::unique_ptr falls maybe under the undefined behavior case, it is just high-levelly speaking bad luck. In some situations, a good alternative would be heap allocated lambdas, which unfortunately, and probably for good reasons, does not exist.

Answer 3

I'm not following all your code but the first thing that jumps out at me is that you are creating a lambda (a) that uses a local variable by reference. I believe that may mean x isn't used until it is not in scope in certain control paths. It is much cleaner to pass x as an argument rather than pass it by reference.

I may be way off though if I am misunderstanding some syntax around the lambda.

Answer 4

The parameter del needs to be const reference in this case. I don't expertise the details of unique_ptr implementation but it may have some special things

template<typename T>
F(const T& del)
{
    handle = del();
}