[Bug c++/95599] New: [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[Bug c++/95599] New: [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[lewissbaker.opensource at gmail dot com]

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

            Bug ID: 95599
           Summary: [coroutines] destructor for temporary operand to
                    co_yield expression called before end of
                    full-expression
           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 I write the statement:
  co_yield T{};

Then I expect the T constructor to be called before the coroutine suspends
and that the T destructor will not be called until after the coroutine
resumes and execution reaches the semicolon (ie. end of full expression).

However, I am observing that the T destructor is being called before the
await_suspend() method on the awaitable returned from promise.yield_value()
is called (ie. before the coroutine suspends).

To reproduce, compile the following code sample using GCC trunk.
Flags: -std=c++2a -fcoroutines

-------
#include <coroutine>

using namespace std;

#include <cstdio>
#include <utility>

struct resource {
    resource() { std::printf("resource()\n"); }
    ~resource() { std::printf("~resource()\n"); }
    resource(resource&&) = delete;
};

template<typename T>
struct generator {
    struct promise_type {
        generator get_return_object() {
            return
generator{coroutine_handle<promise_type>::from_promise(*this)};
        }

        void return_void() {}
        void unhandled_exception() {}
        suspend_always initial_suspend() { return {}; }
        suspend_always final_suspend() { return {}; }

        struct awaitable {
            resource& r;

            awaitable(resource&& r) : r(r) {}

            ~awaitable() {}

            bool await_ready() noexcept { return false; }

            void await_suspend(coroutine_handle<> h) noexcept {
                std::printf("awaitable::await_suspend()\n");
            }

            void await_resume() noexcept {
                std::printf("awaitable::await_resume()\n");
            }
        };

        awaitable yield_value(resource&& r) {
            return awaitable{std::move(r)};
        }
    };

    generator(coroutine_handle<promise_type> coro) : coro(coro)
    {}

    generator(generator&& g) noexcept : coro(std::exchange(g.coro, {}))
    {}

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

    coroutine_handle<promise_type> coro;
};

generator<int> f() {
    co_yield resource{};
}

int main() {
    generator x = f();
    x.coro.resume();
    x.coro.resume();
}
-------

The expected output of this program is:
```
resource()
awaitable::await_suspend()
awaitable::await_resume()
~resource()
```

However, the observed output of this program is:
```
resource()
~resource()
awaitable::await_suspend()
awaitable::await_resume()
```

i.e. the temporary is not being kept alive until the end
of the full-expression containing the co_yield statement.

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[lewissbaker.opensource at gmail dot com]

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

--- Comment #1 from Lewis Baker <lewissbaker.opensource at gmail dot com> ---
Note there is also some different behaviour if we change the example slightly
by removing awaitable::~awaitable() and leaving the awaitable type with a
trivial destructor.

See https://godbolt.org/z/ff5Uvy

In this case, instead of the resource destructor being called before the
coroutine suspends, the resource destructor is never called.

ie. observed output becomes:
```
resource()
awaitable::await_suspend()
awaitable::await_resume()
```

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
           Assignee|unassigned at gcc dot gnu.org      |iains at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2020-06-09
   Target Milestone|---                         |10.2

--- Comment #2 from Iain Sandoe <iains at gcc dot gnu.org> ---
thanks for the report.
I made a thinko with awaitables (they were not being considered in the same way
as other captured-by-reference entities).  I have a local rework in progress to
fix this.

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[lewissbaker.opensource at gmail dot com]

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

--- Comment #3 from Lewis Baker <lewissbaker.opensource at gmail dot com> ---
I don't think that whether or not the awaitables or operands to co_yield are
captured by reference / aliased should play into the sequencing of calls to the
destructor.

The destructors of temporaries should always be sequenced to occur at the
semicolon, which will always happen after the coroutine resumes.

For another clearer example, see https://godbolt.org/z/tKQVnb

e.g. changing the example 

----------
struct resource {
    int x;
    resource(int x) : x(x) { std::printf("resource(%i)\n", x); }
    ~resource() { std::printf("~resource(%i)\n", x); }
    resource(resource&&) = delete;
};

generator<int> f() {
    (resource{1}, co_yield resource{2});
}
----------

Here we are creating temporaries and sequencing expressions using operator,().

The destructor of both resource{1} and resource{2} should both occur at the
semicolon, after the coroutine suspends and resumes, regardless of whether the
object is captured by reference in an awaitable.

ie. the expected output of the modified program should be (this is what clang
does):
```
resource(1)
resource(2)
awaitable::await_suspend()
awaitable::await_resume()
~resource(2)
~resource(1)
```

But the observed output with GCC is:
```
resource(2)
awaitable::await_suspend()
resource(1)
awaitable::await_resume()
~resource(1)
```

Also, I'm not sure why GCC is evaluating the second operand to operator,()
first and why it is constructing resource{1} _after_ the coroutine has
suspended. Nothing should happen in the coroutine body between await_suspend()
and await_resume().

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[iains at gcc dot gnu.org]

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

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Lewis Baker from comment #3)

> Also, I'm not sure why GCC is evaluating the second operand to operator,()
> first and why it is constructing resource{1} _after_ the coroutine has
> suspended. Nothing should happen in the coroutine body between
> await_suspend() and await_resume().

two problems - correctling the lifetime of awaitables I have already fixed
locally (not ready to post yet, but).  The second, handling of operator, needs
a bit more.

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[cvs-commit at gcc dot gnu.org]

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

--- 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:0f66b8486cea8668020e4bd48f261b760cb579be

commit r11-2183-g0f66b8486cea8668020e4bd48f261b760cb579be
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Sat Jul 11 08:49:33 2020 +0100

    coroutines: Correct frame capture of compiler temps [PR95591+4].

    When a full expression contains a co_await (or co_yield), this means
    that it might be suspended; which would destroy temporary variables
    (on a stack for example).  However the language guarantees that such
    temporaries are live until the end of the expression.

    In order to preserve this, we 'promote' temporaries where necessary
    so that they are saved in the coroutine frame (which allows them to
    remain live potentially until the frame is destroyed).  In addition,
    sub-expressions that produce control flow (such as TRUTH_AND/OR_IF
    or COND_EXPR) must be handled specifically to ensure that await
    expressions are properly expanded.

    This patch corrects two mistakes in which we were (a) failing to
    promote some temporaries and (b) we we failing to sequence DTORs for
    the captures properly. This manifests in a number of related (but not
    exact duplicate) PRs.

    The revised code collects the actions into one place and maps all the
    control flow into one form - a benefit of this is that co_returns are
    now expanded earlier (which provides an opportunity to address PR95517
    in some future patch).

    We replace a statement that contains await expression(s) with a bind
    scope that has a variable list describing the temporaries that have
    been 'promoted' and a statement list that contains a series of cleanup
    expressions for each of those.  Where we encounter nested conditional
    expressions, these are wrapped in a try-finally block with a guard var
    for each sub-expression variable that needs a DTOR.  The guards are all
    declared and initialized to false before the first conditional sub-
    expression.  The 'finally' block contains a series of if blocks (one
    per guard variable) enclosing the relevant DTOR.

    Variables listed in a bind scope in this manner are automatically moved
    to a coroutine frame version by existing code (so we re-use that rather
    than having a separate mechanism).

    gcc/cp/ChangeLog:

            PR c++/95591
            PR c++/95599
            PR c++/95823
            PR c++/95824
            PR c++/95895
            * coroutines.cc (struct coro_ret_data): Delete.
            (coro_maybe_expand_co_return): Delete.
            (co_return_expander): Delete.
            (expand_co_returns): Delete.
            (co_await_find_in_subtree): Remove unused name.
            (build_actor_fn): Remove unused parm, remove handling
            for co_return expansion.
            (register_await_info): Demote duplicate info message to a
            warning.
            (coro_make_frame_entry): Move closer to use site.
            (struct susp_frame_data): Add fields for final suspend label
            and a flag to indicate await expressions with initializers.
            (captures_temporary): Delete.
            (register_awaits): Remove unused code, update comments.
            (find_any_await): New.
            (tmp_target_expr_p): New.
            (struct interesting): New.
            (find_interesting_subtree): New.
            (struct var_nest_node): New.
            (flatten_await_stmt): New.
            (handle_nested_conditionals): New.
            (process_conditional): New.
            (replace_statement_captures): Rename to...
            (maybe_promote_temps): ... this.
            (maybe_promote_captured_temps): Delete.
            (analyze_expression_awaits): Check for await expressions with
            initializers.  Simplify handling for truth-and/or-if.
            (expand_one_truth_if): Simplify (map cases that need expansion
            to COND_EXPR).
            (await_statement_walker): Handle CO_RETURN_EXPR. Simplify the
            handling for truth-and/or-if expressions.
            (register_local_var_uses): Ensure that we create names in the
            implementation namespace.
            (morph_fn_to_coro): Add final suspend label to suspend frame
            callback data and remove it from the build_actor_fn call.

    gcc/testsuite/ChangeLog:

            PR c++/95591
            PR c++/95599
            PR c++/95823
            PR c++/95824
            PR c++/95895
            * g++.dg/coroutines/pr95591.C: New test.
            * g++.dg/coroutines/pr95599.C: New test.
            * g++.dg/coroutines/pr95823.C: New test.
            * g++.dg/coroutines/pr95824.C: New test.

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[rguenth at gcc dot gnu.org]

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

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

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

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

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 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:f43a1b1d1718969423337190ddbbbc9037c67783

commit r10-8545-gf43a1b1d1718969423337190ddbbbc9037c67783
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Sun Jul 19 18:39:21 2020 +0100

    coroutines: Correct frame capture of compiler temps [PR95591+4].

    When a full expression contains a co_await (or co_yield), this means
    that it might be suspended; which would destroy temporary variables
    (on a stack for example).  However the language guarantees that such
    temporaries are live until the end of the expression.

    In order to preserve this, we 'promote' temporaries where necessary
    so that they are saved in the coroutine frame (which allows them to
    remain live potentially until the frame is destroyed).  In addition,
    sub-expressions that produce control flow (such as TRUTH_AND/OR_IF
    or COND_EXPR) must be handled specifically to ensure that await
    expressions are properly expanded.

    This patch corrects two mistakes in which we were (a) failing to
    promote some temporaries and (b) we we failing to sequence DTORs for
    the captures properly. This manifests in a number of related (but not
    exact duplicate) PRs.

    The revised code collects the actions into one place and maps all the
    control flow into one form - a benefit of this is that co_returns are
    now expanded earlier (which provides an opportunity to address PR95517
    in some future patch).

    We replace a statement that contains await expression(s) with a bind
    scope that has a variable list describing the temporaries that have
    been 'promoted' and a statement list that contains a series of cleanup
    expressions for each of those.  Where we encounter nested conditional
    expressions, these are wrapped in a try-finally block with a guard var
    for each sub-expression variable that needs a DTOR.  The guards are all
    declared and initialized to false before the first conditional sub-
    expression.  The 'finally' block contains a series of if blocks (one
    per guard variable) enclosing the relevant DTOR.

    Variables listed in a bind scope in this manner are automatically moved
    to a coroutine frame version by existing code (so we re-use that rather
    than having a separate mechanism).

    gcc/cp/ChangeLog:

            PR c++/95591
            PR c++/95599
            PR c++/95823
            PR c++/95824
            PR c++/95895
            * coroutines.cc (struct coro_ret_data): Delete.
            (coro_maybe_expand_co_return): Delete.
            (co_return_expander): Delete.
            (expand_co_returns): Delete.
            (co_await_find_in_subtree): Remove unused name.
            (build_actor_fn): Remove unused parm, remove handling
            for co_return expansion.
            (register_await_info): Demote duplicate info message to a
            warning.
            (coro_make_frame_entry): Move closer to use site.
            (struct susp_frame_data): Add fields for final suspend label
            and a flag to indicate await expressions with initializers.
            (captures_temporary): Delete.
            (register_awaits): Remove unused code, update comments.
            (find_any_await): New.
            (tmp_target_expr_p): New.
            (struct interesting): New.
            (find_interesting_subtree): New.
            (struct var_nest_node): New.
            (flatten_await_stmt): New.
            (handle_nested_conditionals): New.
            (process_conditional): New.
            (replace_statement_captures): Rename to...
            (maybe_promote_temps): ... this.
            (maybe_promote_captured_temps): Delete.
            (analyze_expression_awaits): Check for await expressions with
            initializers.  Simplify handling for truth-and/or-if.
            (expand_one_truth_if): Simplify (map cases that need expansion
            to COND_EXPR).
            (await_statement_walker): Handle CO_RETURN_EXPR. Simplify the
            handling for truth-and/or-if expressions.
            (register_local_var_uses): Ensure that we create names in the
            implementation namespace.
            (morph_fn_to_coro): Add final suspend label to suspend frame
            callback data and remove it from the build_actor_fn call.

    gcc/testsuite/ChangeLog:

            PR c++/95591
            PR c++/95599
            PR c++/95823
            PR c++/95824
            PR c++/95895
            * g++.dg/coroutines/pr95591.C: New test.
            * g++.dg/coroutines/pr95599.C: New test.
            * g++.dg/coroutines/pr95823.C: New test.
            * g++.dg/coroutines/pr95824.C: New test.

    (cherry picked from commit 0f66b8486cea8668020e4bd48f261b760cb579be)

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[iains at gcc dot gnu.org]

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

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

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

--- Comment #8 from Iain Sandoe <iains at gcc dot gnu.org> ---
fixed on master and for the 10.x branch.

[Bug c++/95599] [coroutines] destructor for temporary operand to co_yield expression called before end of full-expression

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |max at duempel dot org

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
*** Bug 95491 has been marked as a duplicate of this bug. ***