[Bug c++/95615] New: [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[Bug c++/95615] New: [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[lewissbaker.opensource at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

            Bug ID: 95615
           Summary: [coroutines] Coroutine frame and promise is leaked if
                    exception thrown from promise.initial_suspend()
           Product: gcc
           Version: 10.1.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: lewissbaker.opensource at gmail dot com
  Target Milestone: ---

If an exception is thrown from any of the following:
- promise_type constructor
- promise.get_return_object()
- promise.initial_suspend()
- initSuspendAwaitable.await_ready()
- initSuspendAwaitable.await_suspend()

Then I believe the compiler is required to automatically
destroy the promise (if constructor completed successfully),
destroy the return return-object (if get_return_object() completed
successfully) and free the coroutine frame before letting the
exception propagate to the caller.

However, it seems that this cleanup logic is not currently
begin called by GCC in these situations.

For example, see https://godbolt.org/z/kQWjpF which shows the
behaviour when an exception is thrown from promise.initial_suspend()'s
await_suspend() method (other variants are commented out in the code).


The wording described in [dcl.fct.def.coroutine] p5 in conjunction with p11
seems to indicate that the promise object should be destroyed as the
exception propagates out of the coroutine.

[Bug c++/95615] [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-06-09
   Target Milestone|---                         |10.2
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Iain Sandoe <iains at gcc dot gnu.org> ---
thanks for the report.

I think the get-return-object case should now be handled (once the patch to add
the cleanup is approved), since that can be covered by a conventional cleanup
expression.

Will need to look at the best way to handle the other two.

[Bug c++/95615] [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.2                        |10.3

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.2 is released, adjusting target milestone.

[Bug c++/95615] [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

--- Comment #3 from Iain Sandoe <iains at gcc dot gnu.org> ---
Actually, I don't think the example goes far enough.

ISTM, [on exception in the places noted] that non-trivial DTORs for frame
copies of parms should be run after the GRO and promise DTOR but before the
frame is freed.

Do you agree? (if so I have a patch for this now)

So, I think the modified example below should print:

Y ()
Y (const Y&)
operator new()
Y (const Y&)
promise_type()
get_return_object()
task()
~task()
~promise_type()
~Y ()
operator delete
~Y ()
caught X
~Y ()

=======
but *both* [master] GCC and clang print:

Y ()
Y (const Y&)
operator new()
Y (const Y&)
promise_type()
task()
~Y ()
~task()
~Y ()

=======

#if __has_include(<coroutine>)
#include <coroutine>
#else
#include <experimental/coroutine>
namespace std {
    using namespace std::experimental;
}
#endif
#include <cstdio>

struct X {};

int Y_live = 0;

struct Y
{
  Y () { std::puts("Y ()"); Y_live++; } 
  Y (const Y&) { std::puts("Y (const Y&)"); Y_live++; } 
  ~Y () { std::puts("~Y ()"); Y_live--; }
};

struct task {
    struct promise_type {
        void* operator new(size_t sz) {
            std::puts("operator new()");
            return ::operator new(sz);
        }

        void operator delete(void* p, size_t sz) {
            std::puts("operator delete");
            return ::operator delete(p, sz);
        }

        promise_type() {
            std::puts("promise_type()");
            // throw X{};
        }

        ~promise_type() {
            std::puts("~promise_type()");
        }

        struct awaiter {
            bool await_ready() {
                //throw X{};
                return false;
            }
            void await_suspend(std::coroutine_handle<>) {
                //throw X{};
            }
            void await_resume() {
                //throw X{};
            }
        };

        awaiter initial_suspend() {
            // throw X{};
            return {};
        }

        task get_return_object() {
            // throw X{};
            return task{};
        }

        std::suspend_never final_suspend() noexcept { return {}; }
        void return_void() noexcept {}
        void unhandled_exception() noexcept {
            std::puts("unhandled_exception()");
        }
    };

    task() { std::puts("task()"); }
    ~task() { std::puts("~task()"); }
    task(task&&) { std::puts("task(task&&)"); }
};

task f(Y Val) {
    co_return;
}

int main() {
    Y Val;
    try {
        auto a = f(Val);
    } catch (X) {
        std::puts("caught X");
    }
}

====

[Bug c++/95615] [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
Created attachment 50195
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50195&action=edit
Patch for testing

This implements the change including cleanup of parm copies with non-trivial
DTORs as mentioned above.

The test needs to be fixed up to call it multiple times for each throwing case,
and to place it in the 'torture' suite, since it's testing code-gen.

[Bug c++/95615] [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain D Sandoe <iains@gcc.gnu.org>:

https://gcc.gnu.org/g:fe55086547c9360b530e040a6673dae10ac77847

commit r11-7527-gfe55086547c9360b530e040a6673dae10ac77847
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Mon Feb 15 15:09:27 2021 +0000

    coroutines : Handle exceptions throw before the first await_resume()
[PR95615].

    The coroutine body is wrapped in a try-catch block which is responsible for
    handling any exceptions thrown by the original function body.  Originally,
the
    initial suspend expression was outside this, but an amendement to the
standard
    places the await_resume call inside and eveything else outside.

    This means that any exception thrown prior to the initial suspend
expression
    await_resume() will propagate to the ramp function.  However, some portion
of
    the coroutine state will exist at that point (how much depends on where the
    exception is thrown from).  For example, we might have some frame parameter
    copies, or the promise object or the return object any of which might have
a
    non-trivial DTOR.  Also the frame itself needs to be deallocated. This
patch
    fixes the handling of these cases.

    gcc/cp/ChangeLog:

            PR c++/95615
            * coroutines.cc (struct param_info): Track parameter copies that
need
            a DTOR.
            (coro_get_frame_dtor): New helper function factored from
build_actor().
            (build_actor_fn): Use coro_get_frame_dtor().
            (morph_fn_to_coro): Track parameters that need DTORs on exception,
            likewise the frame promise and the return object.  On exception,
run the
            DTORs for these, destroy the frame and then rethrow the exception.

    gcc/testsuite/ChangeLog:

            PR c++/95615
            * g++.dg/coroutines/torture/pr95615-01.C: New test.
            * g++.dg/coroutines/torture/pr95615-02.C: New test.
            * g++.dg/coroutines/torture/pr95615-03.C: New test.
            * g++.dg/coroutines/torture/pr95615-04.C: New test.
            * g++.dg/coroutines/torture/pr95615-05.C: New test.

[Bug c++/95615] [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Iain D Sandoe
<iains@gcc.gnu.org>:

https://gcc.gnu.org/g:25880e8fe253d5b6ec258dbf2af3ef8e62d1752e

commit r10-9512-g25880e8fe253d5b6ec258dbf2af3ef8e62d1752e
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Mon Feb 15 15:09:27 2021 +0000

    coroutines : Handle exceptions throw before the first await_resume()
[PR95615].

    The coroutine body is wrapped in a try-catch block which is responsible for
    handling any exceptions thrown by the original function body.  Originally,
the
    initial suspend expression was outside this, but an amendement to the
standard
    places the await_resume call inside and eveything else outside.

    This means that any exception thrown prior to the initial suspend
expression
    await_resume() will propagate to the ramp function.  However, some portion
of
    the coroutine state will exist at that point (how much depends on where the
    exception is thrown from).  For example, we might have some frame parameter
    copies, or the promise object or the return object any of which might have
a
    non-trivial DTOR.  Also the frame itself needs to be deallocated. This
patch
    fixes the handling of these cases.

    gcc/cp/ChangeLog:

            PR c++/95615
            * coroutines.cc (struct param_info): Track parameter copies that
need
            a DTOR.
            (coro_get_frame_dtor): New helper function factored from
build_actor().
            (build_actor_fn): Use coro_get_frame_dtor().
            (morph_fn_to_coro): Track parameters that need DTORs on exception,
            likewise the frame promise and the return object.  On exception,
run the
            DTORs for these, destroy the frame and then rethrow the exception.

    gcc/testsuite/ChangeLog:

            PR c++/95615
            * g++.dg/coroutines/torture/pr95615-01.C: New test.
            * g++.dg/coroutines/torture/pr95615-02.C: New test.
            * g++.dg/coroutines/torture/pr95615-03.C: New test.
            * g++.dg/coroutines/torture/pr95615-04.C: New test.
            * g++.dg/coroutines/torture/pr95615-05.C: New test.

    (cherry picked from commit fe55086547c9360b530e040a6673dae10ac77847)

[Bug c++/95615] [coroutines] Coroutine frame and promise is leaked if exception thrown from promise.initial_suspend()

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95615

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> ---
so fixed for master and 10.3.