Coroutine promise types don't support destroying operator delete

[Quuxplusone]

Bugzilla Link	PR48344
Status	NEW
Importance	P normal
Reported by	Pavel A. Lebedev (cygnus@michiru.ru)
Reported on	2020-11-30 17:23:07 -0800
Last modified on	2020-12-02 18:02:37 -0800
Version	trunk
Hardware	All All
CC	blitzrakete@gmail.com, erik.pilkington@gmail.com, gornishanov@gmail.com, llvm-bugs@lists.llvm.org, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

$ cat pdd.cpp

#if __has_include(<coroutine>)
#include <coroutine>
namespace stdcoro = std;
#else
#include <experimental/coroutine>
namespace stdcoro = std::experimental;
#endif

#include <cstdlib>
#include <new>

struct coroutine
{
    struct promise_type
    {
        coroutine get_return_object() noexcept
        {
            return {*this};
        }

        stdcoro::suspend_always initial_suspend() const noexcept
        {
            return {};
        }

        stdcoro::suspend_always final_suspend() const noexcept
        {
            return {};
        }

        void return_void() const noexcept {}

        void unhandled_exception() noexcept {}

        void* operator new(std::size_t n)
        {
            return std::malloc(n);
        }

        void operator delete(
            promise_type* p,std::destroying_delete_t,std::size_t)
        {
            std::free(p);
        }
    };

    stdcoro::coroutine_handle<promise_type> h_;

    coroutine(promise_type& p)
        : h_{decltype(h_)::from_promise(p)}
    {}

    ~coroutine()
    {
        h_.destroy();
    }
};

coroutine f()
{
    co_return;
}

$ clang++ -std=c++20 -fsyntax-only pdd.cpp

pdd.cpp:59:11: error: too few arguments to function call, expected 3, have 2
coroutine f()
          ^
pdd.cpp:40:14: note: 'operator delete' declared here
        void operator delete(
             ^
1 error generated.

Looking at the end of CoroutineStmtBuilder::makeNewAndDeleteExpr(), it seems
like support for destroying operator delete on promise types is not implemented.

Neither gcc 10.2 nor MSVC 16.8.2 accept this code with similar error messages,
but it looks like it's impossible to implement a coroutine with statefull
Allocator support for promise that also has access to its allocator in its
implementation without support for this combination. I also can't find anything
that prohibits this in the C++ standard.

[Quuxplusone]

[dcl.fct.def.coroutine]p12:

"If deallocation function lookup finds both a usual deallocation function with only a pointer parameter and a usual deallocation function with both a pointer parameter and a size parameter, then the selected deallocation function shall be the one with two parameters. Otherwise, the selected deallocation function shall be the function with one parameter."

This does not support use of a destroying operator delete.

It presumably should, though; this seems like a bug in the language rules.

[Quuxplusone]

Hmm, actually, I take that back. We can't reasonably support destroying operator delete here, because implementations allocate extra storage before the start of the coroutine promise object. That's unfortunate.

[Quuxplusone]

Somehow I missed that paragraph. Unfortunate, indeed.

The best I can figure out for coroutines with allocators therefore is this:

- If destroying delete is not available, we'll have to use a normal new/delete
pair to customize memory allocation at all.
- Normal operator delete runs after the destructor of promise has run, so the
allocator has to be stored elsewhere. We can allocate extra space in new and
store allocator before or after the promise object. This can be skipped when
the allocator type is empty and default-constructible. That's a well-known
general solution.
- The problem here comes when the promise itself needs access to the allocator
(for get_allocator() on the coroutine type, at least), as new/delete might have
been elided, so in that case it needs to store a copy of allocator in itself.
And since its layout is fixed and it has no way of knowing whether it was
dynamically allocated, it has to store a copy always. So when the allocator is
statefull, we end up storing two copies of it, which works, but feels rather
suboptimal.

[Quuxplusone]

Yes, I don't think the allocator can stash information anywhere that the promise can get at it, because the relationship between the start of the allocation and the target of the coroutine_handle pointer / the start of the promise object is unknown, and in general there can be -- and is -- implementation-specific data both before and after the promise within the allocation.

Now, we could permit something like:

struct promise_type { void operator delete(promise_type p, std::destroying_delete_t, [std::size_t size,] [std::align_val_t align,] void alloc_base); };

... to which we would pass both the start of the promise and the start of the overall allocation; that would only require a fairly modest language change. The destruction callback for the coroutine already needs to know how to map between the promise and the allocation base, so this doesn't require much of anything new from the implementation. With this, you could store the allocator within the promise itself.

Gor, what do you think? Is something like this worth pursuing in the committee? Is there another way to solve this problem?

I suppose another approach would be to pass the function argument copies to the operator delete function, in addition to passing them to the operator new; presumably the allocator is typically accessible via the function argument list?