[Bug c++/115908] New: [coroutines] Wrong behavior of using get_return_object() coroutines creation

[Bug c++/115908] New: [coroutines] Wrong behavior of using get_return_object() coroutines creation

[ddvamp007 at gmail dot com]

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

            Bug ID: 115908
           Summary: [coroutines] Wrong behavior of using
                    get_return_object() coroutines creation
           Product: gcc
           Version: 15.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ddvamp007 at gmail dot com
  Target Milestone: ---

The standard says: "The expression promise.get_return_object() is used to
initialize the returned reference or value result object of a call to a
coroutine. The call to get_return_object is sequenced before the call to
initial_suspend and is invoked at most once." [dcl.fct.def.coroutine]
However, if you look at the result of the following program
https://godbolt.org/z/nY8Yo6vGs, it can be seen that the standard is not being
followed.
Firstly, it is not the expression itself that is used for initialization, but
its result
Secondly, if this result is a reference, it is still stored as a value

As a consequence, the behavior is not what the user expects. If we return an
non-copyable and non-movable object type referece, we will get a compilation
error. If the result contains references to the promise object, it may be
destroyed before it is used. etc.

P.S. The same thing is observed in clang

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 58653
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58653&action=edit
testcase

Next time please attach or place inline the testcase rather than just link to
godbolt.

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[ddvamp007 at gmail dot com]

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

--- Comment #2 from Artyom Kolpakov <ddvamp007 at gmail dot com> ---
When I wrote about returning the reference, i meant the return type of
get_return_object(), and not the coroutine itself

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[arsen at gcc dot gnu.org]

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

Arsen Arsenović <arsen at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |arsen at gcc dot gnu.org
   Last reconfirmed|                            |2024-07-30
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #3 from Arsen Arsenović <arsen at gcc dot gnu.org> ---
the sequencing rule is a bit annoying here, we determine the result of the
initial coroutine call only after the first suspend (which might not be the
initial suspend), but I think we can do this correctly anyway.  currently, we
use an intermediary variable called _Coro_gro in the case you showed, hence the
difference in behavior.  confirmed

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |iains at gcc dot gnu.org

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
Created attachment 58948
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58948&action=edit
patch under test

this patch is on top of other changes - but the principle would be unaltered if
made stand-alone.

With this the test case produces:
Promise()
get_return_object()
Handle(Promise &)
return_void()
~Promise()

Which, I think, is what is expected.
(I added a promise ctor print).

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from GCC 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:68ee624bc52ba1154040a904db56dd2f9c3af1f6

commit r15-3153-g68ee624bc52ba1154040a904db56dd2f9c3af1f6
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Sun Aug 18 14:54:38 2024 +0100

    c++, coroutines: Fix ordering of return object conversions [PR115908].

    [dcl.fct.def.coroutine]/7 says:
    The expression promise.get_return_object() is used to initialize the
returned
    reference or prvalue result object of a call to a coroutine. The call to
    get_return_object is sequenced before the call to initial_suspend and is
    invoked at most once.

    The issue is about when any conversions are carried out if the type of
    the g_r_o call is not the same as the ramp return.  Currently, we have been
    doing this by materialising the g_r_o return value and passing that to
    finish_return_expr() which handles the necessary conversions and checks.

    As the PR shows, this does not work as expected.

    In the revised version we carry out the work of the conversions when
    intialising the return slot (with the same facilities that are used by
    finish_return_expr()).  We do this before the call that initiates the
    coroutine body, satisfying the requirements for one call before initial
    suspend.

    The return expression becomes a trivial 'return <retval>'.

    This simplifies the ramp logic considerably, since we no longer need to
    keep track of the temporarily-materialised g_r_o value.

            PR c++/115908

    gcc/cp/ChangeLog:

            * coroutines.cc
            (cp_coroutine_transform::build_ramp_function): Rework the return
            value initialisation to initialise the return slot always from
            get_return_object,  even if that implies carrying out conversions
            to do so.

    gcc/testsuite/ChangeLog:

            * g++.dg/coroutines/pr115908.C: New test.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #6 from Iain Sandoe <iains at gcc dot gnu.org> ---
fixed on trunk, waiting for possible back-port

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[ddvamp007 at gmail dot com]

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

--- Comment #7 from Artyom Kolpakov <ddvamp007 at gmail dot com> ---
(In reply to Iain Sandoe from comment #6)
> fixed on trunk, waiting for possible back-port

I'm not sure if I should write this here, but now a warning has appeared in the
original example: unused parameter 'frame_ptr'

[Bug c++/115908] [coroutines] Wrong behavior of using get_return_object() when creating coroutines

[iains at gcc dot gnu.org]

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

--- Comment #8 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Artyom Kolpakov from comment #7)
> (In reply to Iain Sandoe from comment #6)
> > fixed on trunk, waiting for possible back-port
> 
> I'm not sure if I should write this here, but now a warning has appeared in
> the original example: unused parameter 'frame_ptr'

that should be fixed after  r15-3211-g8d6d6c864442.