c++, coroutines: Fix awaiter var creation [PR116506].

Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
thanks,
Iain

--- >8 ---

Awaiters always need to have a coroutine state frame copy since
they persist across potential supensions.  It simplifies the later
analysis considerably to assign these early which we do when
building co_await expressions.

The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
processing used to cater for cases where the var created from an
xvalue, or is a pointer/reference type.

Corrected thus.

	PR c++/116506

gcc/cp/ChangeLog:

	* coroutines.cc (build_co_await): Ensure that xvalues are
	materialised.  Handle references/pointer values in awaiter
	access expressions.

gcc/testsuite/ChangeLog:

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

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
---
 gcc/cp/coroutines.cc                       | 39 ++++++++++------
 gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
 2 files changed, 77 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C

Hi Jason,

gentle ping for this one

> On 29 Aug 2024, at 16:22, Iain Sandoe <iains.gcc@gmail.com> wrote:
> 
> Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
> thanks,
> Iain
> 
> --- >8 ---
> 
> Awaiters always need to have a coroutine state frame copy since
> they persist across potential supensions.  It simplifies the later
> analysis considerably to assign these early which we do when
> building co_await expressions.
> 
> The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
> processing used to cater for cases where the var created from an
> xvalue, or is a pointer/reference type.
> 
> Corrected thus.
> 
> 	PR c++/116506
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (build_co_await): Ensure that xvalues are
> 	materialised.  Handle references/pointer values in awaiter
> 	access expressions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr116506.C: New test.
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> ---
> gcc/cp/coroutines.cc                       | 39 ++++++++++------
> gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
> 2 files changed, 77 insertions(+), 15 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index d4d74838eb4..d208ce429db 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -1130,13 +1130,12 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>   if (o_type && !VOID_TYPE_P (o_type))
>     o_type = complete_type_or_else (o_type, o);
> 
> -  if (!o_type)
> +  if (!o_type || o_type == error_mark_node)
>     return error_mark_node;
> 
>   if (TREE_CODE (o_type) != RECORD_TYPE)
>     {
> -      error_at (loc, "awaitable type %qT is not a structure",
> -		o_type);
> +      error_at (loc, "awaitable type %qT is not a structure", o_type);
>       return error_mark_node;
>     }
> 
> @@ -1162,20 +1161,32 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>   if (!glvalue_p (o))
>     o = get_target_expr (o, tf_warning_or_error);
> 
> -  tree e_proxy = o;
> -  if (glvalue_p (o))
> +  /* We know that we need a coroutine state frame variable for the awaiter,
> +     since it must persist across suspension; so where one is needed, build
> +     it now.  */
> +  if (INDIRECT_REF_P (o))
> +    o = TREE_OPERAND (o, 0);
> +  tree e_proxy = STRIP_NOPS (o);
> +  o_type = TREE_TYPE (e_proxy);
> +  if (lvalue_p (e_proxy))
>     o = NULL_TREE; /* Use the existing entity.  */
> -  else /* We need to materialise it.  */
> +  else /* We need a non-temp var.  */
>     {
>       e_proxy = get_awaitable_var (suspend_kind, o_type);
>       o = cp_build_init_expr (loc, e_proxy, o);
> -      e_proxy = convert_from_reference (e_proxy);
>     }
> 
> +  /* Build an expression for the object that will be used to call the awaitable
> +     methods.  */
> +  tree e_object = convert_from_reference (e_proxy);
> +  if (POINTER_TYPE_P (TREE_TYPE (e_object)))
> +    e_object = cp_build_indirect_ref (input_location, e_object, RO_UNARY_STAR,
> +				      tf_warning_or_error);
> +
>   /* I suppose we could check that this is contextually convertible to bool.  */
>   tree awrd_func = NULL_TREE;
>   tree awrd_call
> -    = build_new_method_call (e_proxy, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
> +    = build_new_method_call (e_object, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
> 			     &awrd_func, tf_warning_or_error);
> 
>   if (!awrd_func || !awrd_call || awrd_call == error_mark_node)
> @@ -1189,7 +1200,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>   tree h_proxy = get_coroutine_self_handle_proxy (current_function_decl);
>   vec<tree, va_gc> *args = make_tree_vector_single (h_proxy);
>   tree awsp_call
> -    = build_new_method_call (e_proxy, awsp_meth, &args, NULL_TREE,
> +    = build_new_method_call (e_object, awsp_meth, &args, NULL_TREE,
> 			     LOOKUP_NORMAL, &awsp_func, tf_warning_or_error);
> 
>   release_tree_vector (args);
> @@ -1221,7 +1232,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>   /* Finally, the type of e.await_resume() is the co_await's type.  */
>   tree awrs_func = NULL_TREE;
>   tree awrs_call
> -    = build_new_method_call (e_proxy, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
> +    = build_new_method_call (e_object, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
> 			     &awrs_func, tf_warning_or_error);
> 
>   if (!awrs_func || !awrs_call || awrs_call == error_mark_node)
> @@ -1235,7 +1246,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
> 	return error_mark_node;
>       if (coro_diagnose_throwing_fn (awrs_func))
> 	return error_mark_node;
> -      if (tree dummy = cxx_maybe_build_cleanup (e_proxy, tf_none))
> +      if (tree dummy = cxx_maybe_build_cleanup (e_object, tf_none))
> 	{
> 	  if (CONVERT_EXPR_P (dummy))
> 	    dummy = TREE_OPERAND (dummy, 0);
> @@ -1246,7 +1257,8 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>     }
> 
>   /* We now have three call expressions, in terms of the promise, handle and
> -     'e' proxies.  Save them in the await expression for later expansion.  */
> +     'e' proxy expression.  Save them in the await expression for later
> +     expansion.  */
> 
>   tree awaiter_calls = make_tree_vec (3);
>   TREE_VEC_ELT (awaiter_calls, 0) = awrd_call; /* await_ready().  */
> @@ -1259,9 +1271,6 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>     }
>   TREE_VEC_ELT (awaiter_calls, 2) = awrs_call; /* await_resume().  */
> 
> -  if (REFERENCE_REF_P (e_proxy))
> -    e_proxy = TREE_OPERAND (e_proxy, 0);
> -
>   tree awrs_type = TREE_TYPE (TREE_TYPE (awrs_func));
>   tree suspend_kind_cst = build_int_cst (integer_type_node,
> 					 (int) suspend_kind);
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr116506.C b/gcc/testsuite/g++.dg/coroutines/pr116506.C
> new file mode 100644
> index 00000000000..57a6e360566
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr116506.C
> @@ -0,0 +1,53 @@
> +// { dg-do run }
> +// { dg-additional-options "-fno-exceptions" }
> +                                                       
> +#include <coroutine>
> +
> +bool g_too_early = true;
> +std::coroutine_handle<> g_handle;
> +
> +struct Awaiter {
> +    Awaiter() {}
> +    ~Awaiter() {
> +        if (g_too_early) {
> +            __builtin_abort ();
> +        }
> +    }
> +
> +    bool await_ready() { return false; }
> +    void await_suspend(std::coroutine_handle<> handle) {
> +        g_handle = handle;
> +    }
> +
> +    void await_resume() {}
> +};
> +
> +struct SuspendNever {
> +    bool await_ready() noexcept { return true; }
> +    void await_suspend(std::coroutine_handle<>) noexcept {}
> +    void await_resume() noexcept {}
> +};
> +
> +struct Coroutine {
> +    struct promise_type {
> +        Coroutine get_return_object() { return {}; }
> +        SuspendNever initial_suspend() { return {}; }
> +        SuspendNever final_suspend() noexcept { return {}; }
> +        void return_void() {}
> +        void unhandled_exception() {}
> +
> +        Awaiter&& await_transform(Awaiter&& u) {
> +            return static_cast<Awaiter&&>(u);
> +        }
> +    };
> +};
> +
> +Coroutine foo() {
> +    co_await Awaiter{};
> +}
> +
> +int main() {
> +    foo();
> +    g_too_early = false;
> +    g_handle.destroy();
> +}
> -- 
> 2.39.2 (Apple Git-143)
>

On 8/29/24 5:22 PM, Iain Sandoe wrote:
> Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
> thanks,
> Iain
> 
> --- >8 ---
> 
> Awaiters always need to have a coroutine state frame copy since
> they persist across potential supensions.  It simplifies the later
> analysis considerably to assign these early which we do when
> building co_await expressions.
> 
> The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
> processing used to cater for cases where the var created from an
> xvalue, or is a pointer/reference type.

This seems closely related to CWG2472, which was resolved as NAD; it 
sounds to me like an xvalue should be cast to an lvalue referring to the 
same object, not that it should be used to initialize a new variable of 
class type.  See below...

> Corrected thus.
> 
> 	PR c++/116506
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (build_co_await): Ensure that xvalues are
> 	materialised.  Handle references/pointer values in awaiter
> 	access expressions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr116506.C: New test.
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> ---
>   gcc/cp/coroutines.cc                       | 39 ++++++++++------
>   gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
>   2 files changed, 77 insertions(+), 15 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index d4d74838eb4..d208ce429db 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -1130,13 +1130,12 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>     if (o_type && !VOID_TYPE_P (o_type))
>       o_type = complete_type_or_else (o_type, o);
>   
> -  if (!o_type)
> +  if (!o_type || o_type == error_mark_node)
>       return error_mark_node;
>   
>     if (TREE_CODE (o_type) != RECORD_TYPE)
>       {
> -      error_at (loc, "awaitable type %qT is not a structure",
> -		o_type);
> +      error_at (loc, "awaitable type %qT is not a structure", o_type);
>         return error_mark_node;
>       }
>   
> @@ -1162,20 +1161,32 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>     if (!glvalue_p (o))
>       o = get_target_expr (o, tf_warning_or_error);
>   
> -  tree e_proxy = o;
> -  if (glvalue_p (o))
> +  /* We know that we need a coroutine state frame variable for the awaiter,
> +     since it must persist across suspension; so where one is needed, build
> +     it now.  */
> +  if (INDIRECT_REF_P (o))
> +    o = TREE_OPERAND (o, 0);
> +  tree e_proxy = STRIP_NOPS (o);
> +  o_type = TREE_TYPE (e_proxy);

So if you have an lvalue 'o' derived from a reference, this will change 
'o' to be the reference, or if it's derived from a pointer, to the 
pointer.  This seems like weird special-casing to me.  If you need a 
frame variable, how about always creating a reference variable for 
glvalue o, regardless of how that glvalue was derived?

Why would you need a variable for an xvalue, but not for an lvalue?

> +  if (lvalue_p (e_proxy))
>       o = NULL_TREE; /* Use the existing entity.  */
> -  else /* We need to materialise it.  */
> +  else /* We need a non-temp var.  */
>       {
>         e_proxy = get_awaitable_var (suspend_kind, o_type);
>         o = cp_build_init_expr (loc, e_proxy, o);
> -      e_proxy = convert_from_reference (e_proxy);
>       }
>   
> +  /* Build an expression for the object that will be used to call the awaitable
> +     methods.  */
> +  tree e_object = convert_from_reference (e_proxy);
> +  if (POINTER_TYPE_P (TREE_TYPE (e_object)))
> +    e_object = cp_build_indirect_ref (input_location, e_object, RO_UNARY_STAR,
> +				      tf_warning_or_error);
> +
>     /* I suppose we could check that this is contextually convertible to bool.  */
>     tree awrd_func = NULL_TREE;
>     tree awrd_call
> -    = build_new_method_call (e_proxy, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
> +    = build_new_method_call (e_object, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
>   			     &awrd_func, tf_warning_or_error);
>   
>     if (!awrd_func || !awrd_call || awrd_call == error_mark_node)
> @@ -1189,7 +1200,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>     tree h_proxy = get_coroutine_self_handle_proxy (current_function_decl);
>     vec<tree, va_gc> *args = make_tree_vector_single (h_proxy);
>     tree awsp_call
> -    = build_new_method_call (e_proxy, awsp_meth, &args, NULL_TREE,
> +    = build_new_method_call (e_object, awsp_meth, &args, NULL_TREE,
>   			     LOOKUP_NORMAL, &awsp_func, tf_warning_or_error);
>   
>     release_tree_vector (args);
> @@ -1221,7 +1232,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>     /* Finally, the type of e.await_resume() is the co_await's type.  */
>     tree awrs_func = NULL_TREE;
>     tree awrs_call
> -    = build_new_method_call (e_proxy, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
> +    = build_new_method_call (e_object, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
>   			     &awrs_func, tf_warning_or_error);
>   
>     if (!awrs_func || !awrs_call || awrs_call == error_mark_node)
> @@ -1235,7 +1246,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>   	return error_mark_node;
>         if (coro_diagnose_throwing_fn (awrs_func))
>   	return error_mark_node;
> -      if (tree dummy = cxx_maybe_build_cleanup (e_proxy, tf_none))
> +      if (tree dummy = cxx_maybe_build_cleanup (e_object, tf_none))
>   	{
>   	  if (CONVERT_EXPR_P (dummy))
>   	    dummy = TREE_OPERAND (dummy, 0);
> @@ -1246,7 +1257,8 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>       }
>   
>     /* We now have three call expressions, in terms of the promise, handle and
> -     'e' proxies.  Save them in the await expression for later expansion.  */
> +     'e' proxy expression.  Save them in the await expression for later
> +     expansion.  */
>   
>     tree awaiter_calls = make_tree_vec (3);
>     TREE_VEC_ELT (awaiter_calls, 0) = awrd_call; /* await_ready().  */
> @@ -1259,9 +1271,6 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>       }
>     TREE_VEC_ELT (awaiter_calls, 2) = awrs_call; /* await_resume().  */
>   
> -  if (REFERENCE_REF_P (e_proxy))
> -    e_proxy = TREE_OPERAND (e_proxy, 0);
> -
>     tree awrs_type = TREE_TYPE (TREE_TYPE (awrs_func));
>     tree suspend_kind_cst = build_int_cst (integer_type_node,
>   					 (int) suspend_kind);
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr116506.C b/gcc/testsuite/g++.dg/coroutines/pr116506.C
> new file mode 100644
> index 00000000000..57a6e360566
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr116506.C
> @@ -0,0 +1,53 @@
> +// { dg-do run }
> +// { dg-additional-options "-fno-exceptions" }
> +
> +#include <coroutine>
> +
> +bool g_too_early = true;
> +std::coroutine_handle<> g_handle;
> +
> +struct Awaiter {
> +    Awaiter() {}
> +    ~Awaiter() {
> +        if (g_too_early) {
> +            __builtin_abort ();
> +        }
> +    }
> +
> +    bool await_ready() { return false; }
> +    void await_suspend(std::coroutine_handle<> handle) {
> +        g_handle = handle;
> +    }
> +
> +    void await_resume() {}
> +};
> +
> +struct SuspendNever {
> +    bool await_ready() noexcept { return true; }
> +    void await_suspend(std::coroutine_handle<>) noexcept {}
> +    void await_resume() noexcept {}
> +};
> +
> +struct Coroutine {
> +    struct promise_type {
> +        Coroutine get_return_object() { return {}; }
> +        SuspendNever initial_suspend() { return {}; }
> +        SuspendNever final_suspend() noexcept { return {}; }
> +        void return_void() {}
> +        void unhandled_exception() {}
> +
> +        Awaiter&& await_transform(Awaiter&& u) {
> +            return static_cast<Awaiter&&>(u);
> +        }
> +    };
> +};
> +
> +Coroutine foo() {
> +    co_await Awaiter{};
> +}
> +
> +int main() {
> +    foo();
> +    g_too_early = false;
> +    g_handle.destroy();
> +}

> On 17 Sep 2024, at 18:24, Jason Merrill <jason@redhat.com> wrote:
> 
> On 8/29/24 5:22 PM, Iain Sandoe wrote:
>> Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
>> thanks,
>> Iain
>> --- >8 ---
>> Awaiters always need to have a coroutine state frame copy since
>> they persist across potential supensions.  It simplifies the later
>> analysis considerably to assign these early which we do when
>> building co_await expressions.
>> The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
>> processing used to cater for cases where the var created from an
>> xvalue, or is a pointer/reference type.
> 
> This seems closely related to CWG2472, which was resolved as NAD; it sounds to me like an xvalue should be cast to an lvalue referring to the same object, not that it should be used to initialize a new variable of class type.  See below...

Ah … I am not sure how to do that cast (mechanically) - but that would also do what is required.

>> Corrected thus.
>> 	PR c++/116506
>> gcc/cp/ChangeLog:
>> 	* coroutines.cc (build_co_await): Ensure that xvalues are
>> 	materialised.  Handle references/pointer values in awaiter
>> 	access expressions.
>> gcc/testsuite/ChangeLog:
>> 	* g++.dg/coroutines/pr116506.C: New test.
>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>> ---
>>  gcc/cp/coroutines.cc                       | 39 ++++++++++------
>>  gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
>>  2 files changed, 77 insertions(+), 15 deletions(-)
>>  create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index d4d74838eb4..d208ce429db 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -1130,13 +1130,12 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>    if (o_type && !VOID_TYPE_P (o_type))
>>      o_type = complete_type_or_else (o_type, o);
>>  -  if (!o_type)
>> +  if (!o_type || o_type == error_mark_node)
>>      return error_mark_node;
>>      if (TREE_CODE (o_type) != RECORD_TYPE)
>>      {
>> -      error_at (loc, "awaitable type %qT is not a structure",
>> -		o_type);
>> +      error_at (loc, "awaitable type %qT is not a structure", o_type);
>>        return error_mark_node;
>>      }
>>  @@ -1162,20 +1161,32 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>    if (!glvalue_p (o))
>>      o = get_target_expr (o, tf_warning_or_error);
>>  -  tree e_proxy = o;
>> -  if (glvalue_p (o))
>> +  /* We know that we need a coroutine state frame variable for the awaiter,
>> +     since it must persist across suspension; so where one is needed, build
>> +     it now.  */
>> +  if (INDIRECT_REF_P (o))
>> +    o = TREE_OPERAND (o, 0);
>> +  tree e_proxy = STRIP_NOPS (o);
>> +  o_type = TREE_TYPE (e_proxy);
> 
> So if you have an lvalue 'o' derived from a reference, this will change 'o' to be the reference, or if it's derived from a pointer, to the pointer.  This seems like weird special-casing to me.  If you need a frame variable, how about always creating a reference variable for glvalue o, regardless of how that glvalue was derived?
> 
> Why would you need a variable for an xvalue, but not for an lvalue?

The intent is to ensure that awaiter instances are lvalues so that they are correctly preserved across suspensions - the reason for calling them out specifically (c.f. any other temporary) is that doing so makes the analysis code simpler.

thanks
Iain

> 
>> +  if (lvalue_p (e_proxy))
>>      o = NULL_TREE; /* Use the existing entity.  */
>> -  else /* We need to materialise it.  */
>> +  else /* We need a non-temp var.  */
>>      {
>>        e_proxy = get_awaitable_var (suspend_kind, o_type);
>>        o = cp_build_init_expr (loc, e_proxy, o);
>> -      e_proxy = convert_from_reference (e_proxy);
>>      }
>>  +  /* Build an expression for the object that will be used to call the awaitable
>> +     methods.  */
>> +  tree e_object = convert_from_reference (e_proxy);
>> +  if (POINTER_TYPE_P (TREE_TYPE (e_object)))
>> +    e_object = cp_build_indirect_ref (input_location, e_object, RO_UNARY_STAR,
>> +				      tf_warning_or_error);
>> +
>>    /* I suppose we could check that this is contextually convertible to bool.  */
>>    tree awrd_func = NULL_TREE;
>>    tree awrd_call
>> -    = build_new_method_call (e_proxy, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
>> +    = build_new_method_call (e_object, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
>>  			     &awrd_func, tf_warning_or_error);
>>      if (!awrd_func || !awrd_call || awrd_call == error_mark_node)
>> @@ -1189,7 +1200,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>    tree h_proxy = get_coroutine_self_handle_proxy (current_function_decl);
>>    vec<tree, va_gc> *args = make_tree_vector_single (h_proxy);
>>    tree awsp_call
>> -    = build_new_method_call (e_proxy, awsp_meth, &args, NULL_TREE,
>> +    = build_new_method_call (e_object, awsp_meth, &args, NULL_TREE,
>>  			     LOOKUP_NORMAL, &awsp_func, tf_warning_or_error);
>>      release_tree_vector (args);
>> @@ -1221,7 +1232,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>    /* Finally, the type of e.await_resume() is the co_await's type.  */
>>    tree awrs_func = NULL_TREE;
>>    tree awrs_call
>> -    = build_new_method_call (e_proxy, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
>> +    = build_new_method_call (e_object, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
>>  			     &awrs_func, tf_warning_or_error);
>>      if (!awrs_func || !awrs_call || awrs_call == error_mark_node)
>> @@ -1235,7 +1246,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>  	return error_mark_node;
>>        if (coro_diagnose_throwing_fn (awrs_func))
>>  	return error_mark_node;
>> -      if (tree dummy = cxx_maybe_build_cleanup (e_proxy, tf_none))
>> +      if (tree dummy = cxx_maybe_build_cleanup (e_object, tf_none))
>>  	{
>>  	  if (CONVERT_EXPR_P (dummy))
>>  	    dummy = TREE_OPERAND (dummy, 0);
>> @@ -1246,7 +1257,8 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>      }
>>      /* We now have three call expressions, in terms of the promise, handle and
>> -     'e' proxies.  Save them in the await expression for later expansion.  */
>> +     'e' proxy expression.  Save them in the await expression for later
>> +     expansion.  */
>>      tree awaiter_calls = make_tree_vec (3);
>>    TREE_VEC_ELT (awaiter_calls, 0) = awrd_call; /* await_ready().  */
>> @@ -1259,9 +1271,6 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>      }
>>    TREE_VEC_ELT (awaiter_calls, 2) = awrs_call; /* await_resume().  */
>>  -  if (REFERENCE_REF_P (e_proxy))
>> -    e_proxy = TREE_OPERAND (e_proxy, 0);
>> -
>>    tree awrs_type = TREE_TYPE (TREE_TYPE (awrs_func));
>>    tree suspend_kind_cst = build_int_cst (integer_type_node,
>>  					 (int) suspend_kind);
>> diff --git a/gcc/testsuite/g++.dg/coroutines/pr116506.C b/gcc/testsuite/g++.dg/coroutines/pr116506.C
>> new file mode 100644
>> index 00000000000..57a6e360566
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/coroutines/pr116506.C
>> @@ -0,0 +1,53 @@
>> +// { dg-do run }
>> +// { dg-additional-options "-fno-exceptions" }
>> +
>> +#include <coroutine>
>> +
>> +bool g_too_early = true;
>> +std::coroutine_handle<> g_handle;
>> +
>> +struct Awaiter {
>> +    Awaiter() {}
>> +    ~Awaiter() {
>> +        if (g_too_early) {
>> +            __builtin_abort ();
>> +        }
>> +    }
>> +
>> +    bool await_ready() { return false; }
>> +    void await_suspend(std::coroutine_handle<> handle) {
>> +        g_handle = handle;
>> +    }
>> +
>> +    void await_resume() {}
>> +};
>> +
>> +struct SuspendNever {
>> +    bool await_ready() noexcept { return true; }
>> +    void await_suspend(std::coroutine_handle<>) noexcept {}
>> +    void await_resume() noexcept {}
>> +};
>> +
>> +struct Coroutine {
>> +    struct promise_type {
>> +        Coroutine get_return_object() { return {}; }
>> +        SuspendNever initial_suspend() { return {}; }
>> +        SuspendNever final_suspend() noexcept { return {}; }
>> +        void return_void() {}
>> +        void unhandled_exception() {}
>> +
>> +        Awaiter&& await_transform(Awaiter&& u) {
>> +            return static_cast<Awaiter&&>(u);
>> +        }
>> +    };
>> +};
>> +
>> +Coroutine foo() {
>> +    co_await Awaiter{};
>> +}
>> +
>> +int main() {
>> +    foo();
>> +    g_too_early = false;
>> +    g_handle.destroy();
>> +}

On 9/17/24 8:26 PM, Iain Sandoe wrote:
> 
> 
>> On 17 Sep 2024, at 18:24, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 8/29/24 5:22 PM, Iain Sandoe wrote:
>>> Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
>>> thanks,
>>> Iain
>>> --- >8 ---
>>> Awaiters always need to have a coroutine state frame copy since
>>> they persist across potential supensions.  It simplifies the later
>>> analysis considerably to assign these early which we do when
>>> building co_await expressions.
>>> The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
>>> processing used to cater for cases where the var created from an
>>> xvalue, or is a pointer/reference type.
>>
>> This seems closely related to CWG2472, which was resolved as NAD; it sounds to me like an xvalue should be cast to an lvalue referring to the same object, not that it should be used to initialize a new variable of class type.  See below...
> 
> Ah … I am not sure how to do that cast (mechanically) - but that would also do what is required.

int&& f();
reinterpret_cast<int&>(f()) // lvalue
-or-
int&& r = f(); // r is an lvalue

>>> Corrected thus.
>>> 	PR c++/116506
>>> gcc/cp/ChangeLog:
>>> 	* coroutines.cc (build_co_await): Ensure that xvalues are
>>> 	materialised.  Handle references/pointer values in awaiter
>>> 	access expressions.
>>> gcc/testsuite/ChangeLog:
>>> 	* g++.dg/coroutines/pr116506.C: New test.
>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>> ---
>>>   gcc/cp/coroutines.cc                       | 39 ++++++++++------
>>>   gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
>>>   2 files changed, 77 insertions(+), 15 deletions(-)
>>>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>> index d4d74838eb4..d208ce429db 100644
>>> --- a/gcc/cp/coroutines.cc
>>> +++ b/gcc/cp/coroutines.cc
>>> @@ -1130,13 +1130,12 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>>     if (o_type && !VOID_TYPE_P (o_type))
>>>       o_type = complete_type_or_else (o_type, o);
>>>   -  if (!o_type)
>>> +  if (!o_type || o_type == error_mark_node)
>>>       return error_mark_node;
>>>       if (TREE_CODE (o_type) != RECORD_TYPE)
>>>       {
>>> -      error_at (loc, "awaitable type %qT is not a structure",
>>> -		o_type);
>>> +      error_at (loc, "awaitable type %qT is not a structure", o_type);
>>>         return error_mark_node;
>>>       }
>>>   @@ -1162,20 +1161,32 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>>     if (!glvalue_p (o))
>>>       o = get_target_expr (o, tf_warning_or_error);
>>>   -  tree e_proxy = o;
>>> -  if (glvalue_p (o))
>>> +  /* We know that we need a coroutine state frame variable for the awaiter,
>>> +     since it must persist across suspension; so where one is needed, build
>>> +     it now.  */
>>> +  if (INDIRECT_REF_P (o))
>>> +    o = TREE_OPERAND (o, 0);
>>> +  tree e_proxy = STRIP_NOPS (o);
>>> +  o_type = TREE_TYPE (e_proxy);
>>
>> So if you have an lvalue 'o' derived from a reference, this will change 'o' to be the reference, or if it's derived from a pointer, to the pointer.  This seems like weird special-casing to me.  If you need a frame variable, how about always creating a reference variable for glvalue o, regardless of how that glvalue was derived?
>>
>> Why would you need a variable for an xvalue, but not for an lvalue?
> 
> The intent is to ensure that awaiter instances are lvalues so that they are correctly preserved across suspensions - the reason for calling them out specifically (c.f. any other temporary) is that doing so makes the analysis code simpler.

My point is that being an lvalue is distinct from being a variable, 
except for prvalues that need to be materialized; an xvalue you can cast 
to an value, e.g.

int&& f();
reinterpret_cast<int&>(f()) // lvalue

It's true that initializing a variable is a way to turn an xvalue into 
an lvalue without reinterpret_cast:

int&& r = f(); // r is an lvalue

but it's not clear to me that it's worth paying the space in the 
coroutine frame if we don't also need it for lvalue o.

Why don't you need a variable to preserve o across suspensions if it's a 
call returning lvalue reference?  I suspect that the simple case is not 
lvalue_p, but !TREE_SIDE_EFFECTS.

Jason

> On 17 Sep 2024, at 20:06, Jason Merrill <jason@redhat.com> wrote:
> 
> On 9/17/24 8:26 PM, Iain Sandoe wrote:
>>> On 17 Sep 2024, at 18:24, Jason Merrill <jason@redhat.com> wrote:
>>> 
>>> On 8/29/24 5:22 PM, Iain Sandoe wrote:
>>>> Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
>>>> thanks,
>>>> Iain
>>>> --- >8 ---
>>>> Awaiters always need to have a coroutine state frame copy since
>>>> they persist across potential supensions.  It simplifies the later
>>>> analysis considerably to assign these early which we do when
>>>> building co_await expressions.
>>>> The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
>>>> processing used to cater for cases where the var created from an
>>>> xvalue, or is a pointer/reference type.
>>> 
>>> This seems closely related to CWG2472, which was resolved as NAD; it sounds to me like an xvalue should be cast to an lvalue referring to the same object, not that it should be used to initialize a new variable of class type.  See below...
>> Ah … I am not sure how to do that cast (mechanically) - but that would also do what is required.
> 
> int&& f();
> reinterpret_cast<int&>(f()) // lvalue
> -or-
> int&& r = f(); // r is an lvalue

(I was meaning more the APIs to implement the same action inside the compiler).
 
> 
>>>> Corrected thus.
>>>> 	PR c++/116506
>>>> gcc/cp/ChangeLog:
>>>> 	* coroutines.cc (build_co_await): Ensure that xvalues are
>>>> 	materialised.  Handle references/pointer values in awaiter
>>>> 	access expressions.
>>>> gcc/testsuite/ChangeLog:
>>>> 	* g++.dg/coroutines/pr116506.C: New test.
>>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>>> ---
>>>>  gcc/cp/coroutines.cc                       | 39 ++++++++++------
>>>>  gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
>>>>  2 files changed, 77 insertions(+), 15 deletions(-)
>>>>  create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>>> index d4d74838eb4..d208ce429db 100644
>>>> --- a/gcc/cp/coroutines.cc
>>>> +++ b/gcc/cp/coroutines.cc
>>>> @@ -1130,13 +1130,12 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>>>    if (o_type && !VOID_TYPE_P (o_type))
>>>>      o_type = complete_type_or_else (o_type, o);
>>>>  -  if (!o_type)
>>>> +  if (!o_type || o_type == error_mark_node)
>>>>      return error_mark_node;
>>>>      if (TREE_CODE (o_type) != RECORD_TYPE)
>>>>      {
>>>> -      error_at (loc, "awaitable type %qT is not a structure",
>>>> -		o_type);
>>>> +      error_at (loc, "awaitable type %qT is not a structure", o_type);
>>>>        return error_mark_node;
>>>>      }
>>>>  @@ -1162,20 +1161,32 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>>>    if (!glvalue_p (o))
>>>>      o = get_target_expr (o, tf_warning_or_error);
>>>>  -  tree e_proxy = o;
>>>> -  if (glvalue_p (o))
>>>> +  /* We know that we need a coroutine state frame variable for the awaiter,
>>>> +     since it must persist across suspension; so where one is needed, build
>>>> +     it now.  */
>>>> +  if (INDIRECT_REF_P (o))
>>>> +    o = TREE_OPERAND (o, 0);
>>>> +  tree e_proxy = STRIP_NOPS (o);
>>>> +  o_type = TREE_TYPE (e_proxy);
>>> 
>>> So if you have an lvalue 'o' derived from a reference, this will change 'o' to be the reference, or if it's derived from a pointer, to the pointer.  This seems like weird special-casing to me.  If you need a frame variable, how about always creating a reference variable for glvalue o, regardless of how that glvalue was derived?
>>> 
>>> Why would you need a variable for an xvalue, but not for an lvalue?
>> The intent is to ensure that awaiter instances are lvalues so that they are correctly preserved across suspensions - the reason for calling them out specifically (c.f. any other temporary) is that doing so makes the analysis code simpler.
> 
> My point is that being an lvalue is distinct from being a variable, except for prvalues that need to be materialized; an xvalue you can cast to an value, e.g.
> 
> int&& f();
> reinterpret_cast<int&>(f()) // lvalue
> 
> It's true that initializing a variable is a way to turn an xvalue into an lvalue without reinterpret_cast:
> 
> int&& r = f(); // r is an lvalue
> 
> but it's not clear to me that it's worth paying the space in the coroutine frame if we don't also need it for lvalue o.
> 
> Why don't you need a variable to preserve o across suspensions if it's a call returning lvalue reference?

We always need a space for the awaiter, unless it is already a variable/parameter (or part of one).

>  I suspect that the simple case is not lvalue_p, but !TREE_SIDE_EFFECTS.

That is likely where I’m going wrong - we must not generate a variable for any case that already has one (or a parm), but we must for any case that is a temporary.

So, I should adjust the logic to use !TREE_SIDE_EFFECTS.

does the intention make sense now?
thanks
Iain

> 
> Jason

On 9/17/24 9:26 PM, Iain Sandoe wrote:
> 
> 
>> On 17 Sep 2024, at 20:06, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 9/17/24 8:26 PM, Iain Sandoe wrote:
>>>> On 17 Sep 2024, at 18:24, Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> On 8/29/24 5:22 PM, Iain Sandoe wrote:
>>>>> Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
>>>>> thanks,
>>>>> Iain
>>>>> --- >8 ---
>>>>> Awaiters always need to have a coroutine state frame copy since
>>>>> they persist across potential supensions.  It simplifies the later
>>>>> analysis considerably to assign these early which we do when
>>>>> building co_await expressions.
>>>>> The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
>>>>> processing used to cater for cases where the var created from an
>>>>> xvalue, or is a pointer/reference type.
>>>>
>>>> This seems closely related to CWG2472, which was resolved as NAD; it sounds to me like an xvalue should be cast to an lvalue referring to the same object, not that it should be used to initialize a new variable of class type.  See below...
>>> Ah … I am not sure how to do that cast (mechanically) - but that would also do what is required.
>>
>> int&& f();
>> reinterpret_cast<int&>(f()) // lvalue
>> -or-
>> int&& r = f(); // r is an lvalue
> 
> (I was meaning more the APIs to implement the same action inside the compiler).
>   
>>
>>>>> Corrected thus.
>>>>> 	PR c++/116506
>>>>> gcc/cp/ChangeLog:
>>>>> 	* coroutines.cc (build_co_await): Ensure that xvalues are
>>>>> 	materialised.  Handle references/pointer values in awaiter
>>>>> 	access expressions.
>>>>> gcc/testsuite/ChangeLog:
>>>>> 	* g++.dg/coroutines/pr116506.C: New test.
>>>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>>>> ---
>>>>>   gcc/cp/coroutines.cc                       | 39 ++++++++++------
>>>>>   gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
>>>>>   2 files changed, 77 insertions(+), 15 deletions(-)
>>>>>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
>>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>>>> index d4d74838eb4..d208ce429db 100644
>>>>> --- a/gcc/cp/coroutines.cc
>>>>> +++ b/gcc/cp/coroutines.cc
>>>>> @@ -1130,13 +1130,12 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>>>>     if (o_type && !VOID_TYPE_P (o_type))
>>>>>       o_type = complete_type_or_else (o_type, o);
>>>>>   -  if (!o_type)
>>>>> +  if (!o_type || o_type == error_mark_node)
>>>>>       return error_mark_node;
>>>>>       if (TREE_CODE (o_type) != RECORD_TYPE)
>>>>>       {
>>>>> -      error_at (loc, "awaitable type %qT is not a structure",
>>>>> -		o_type);
>>>>> +      error_at (loc, "awaitable type %qT is not a structure", o_type);
>>>>>         return error_mark_node;
>>>>>       }
>>>>>   @@ -1162,20 +1161,32 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>>>>>     if (!glvalue_p (o))
>>>>>       o = get_target_expr (o, tf_warning_or_error);
>>>>>   -  tree e_proxy = o;
>>>>> -  if (glvalue_p (o))
>>>>> +  /* We know that we need a coroutine state frame variable for the awaiter,
>>>>> +     since it must persist across suspension; so where one is needed, build
>>>>> +     it now.  */
>>>>> +  if (INDIRECT_REF_P (o))
>>>>> +    o = TREE_OPERAND (o, 0);
>>>>> +  tree e_proxy = STRIP_NOPS (o);
>>>>> +  o_type = TREE_TYPE (e_proxy);
>>>>
>>>> So if you have an lvalue 'o' derived from a reference, this will change 'o' to be the reference, or if it's derived from a pointer, to the pointer.  This seems like weird special-casing to me.  If you need a frame variable, how about always creating a reference variable for glvalue o, regardless of how that glvalue was derived?
>>>>
>>>> Why would you need a variable for an xvalue, but not for an lvalue?
>>> The intent is to ensure that awaiter instances are lvalues so that they are correctly preserved across suspensions - the reason for calling them out specifically (c.f. any other temporary) is that doing so makes the analysis code simpler.
>>
>> My point is that being an lvalue is distinct from being a variable, except for prvalues that need to be materialized; an xvalue you can cast to an value, e.g.
>>
>> int&& f();
>> reinterpret_cast<int&>(f()) // lvalue
>>
>> It's true that initializing a variable is a way to turn an xvalue into an lvalue without reinterpret_cast:
>>
>> int&& r = f(); // r is an lvalue
>>
>> but it's not clear to me that it's worth paying the space in the coroutine frame if we don't also need it for lvalue o.
>>
>> Why don't you need a variable to preserve o across suspensions if it's a call returning lvalue reference?
> 
> We always need a space for the awaiter, unless it is already a variable/parameter (or part of one).
> 
>>   I suspect that the simple case is not lvalue_p, but !TREE_SIDE_EFFECTS.
> 
> That is likely where I’m going wrong - we must not generate a variable for any case that already has one (or a parm), but we must for any case that is a temporary.
> 
> So, I should adjust the logic to use !TREE_SIDE_EFFECTS.

Or perhaps DECL_P.  The difference would be for compound lvalues like *p 
or a[n]; if the value of p or a or n could change across suspension, the 
same side-effect-free lvalue expression could refer to a different object.

> does the intention make sense now?

Yes.

Jason