Jason Merrill <ja...@redhat.com> writes:
> On 7/31/24 6:54 AM, Arsen Arsenović wrote:
>> Tested on x86_64-pc-linux-gnu. OK for trunk?
>> TIA, have a lovely day.
>> ---------- >8 ----------
>> By doing so, we can get diagnostics in template decls when we know we
>> can. For instance, in the following:
>> awaitable g();
>> template<typename>
>> task f()
>> {
>> co_await g();
>> co_yield 1;
>> co_return "foo";
>> }
>> ... the coroutine promise type in each statement is always
>> std::coroutine_handle<task>::promise_type, and all of the operands are
>> not type-dependent, so we can always compute the resulting types (and
>> expected types) of these expressions and statements.
>> Also, when we do not know the type of the CO_AWAIT_EXPR or
>> CO_YIELD_EXPR, we now return NULL_TREE as the type rather than
>> unknown_type_node. This is more correct, since the type is not unknown,
>> it just isn't determined yet. This also means we can remove the
>> CO_AWAIT_EXPR and CO_YIELD_EXPR special-cases from
>> type_dependent_expression_p.
>> PR c++/112341 - error: insufficient contextual information to determine type
>> on co_await result in function template
>> gcc/cp/ChangeLog:
>> PR c++/112341
>> * coroutines.cc (struct coroutine_info): Also cache the
>> traits type.
>> (ensure_coro_initialized): New function. Makes sure we have
>> initialized the coroutine state successfully, or informs the
>> caller should it fail to do so. Extracted from
>> coro_promise_type_found_p.
>> (coro_get_traits_class): New function. Gets the (cached)
>> coroutine traits type for a given coroutine. Extracted from
>> coro_promise_type_found_p and refactored to cache the result.
>> (coro_promise_type_found_p): Use the two functions above.
>> (build_template_co_await_expr): New function. Builds a
>> CO_AWAIT_EXPR representing a CO_AWAIT_EXPR in a template
>> declaration.
>> (build_co_await): Use the above if processing_template_decl, and
>> give it a propert type.
>> (defer_expansion_p): New function. Returns true iff its
>> argument is a type-dependent expression OR the current functions
>> traits class is type dependent.
>> (finish_co_await_expr): Defer expansion only in the case
>> defer_expasnion_p returns true.
>> (finish_co_yield_expr): Ditto.
>> (finish_co_return_stmt): Ditto.
>> * pt.cc (type_dependent_expression_p): Do not treat
>> CO_AWAIT/CO_YIELD specially.
>> gcc/testsuite/ChangeLog:
>> PR c++/112341
>> * g++.dg/coroutines/pr112341-2.C: New test.
>> * g++.dg/coroutines/pr112341-3.C: New test.
>> * g++.dg/coroutines/torture/co-yield-03-tmpl-nondependent.C: New
>> test.
>> * g++.dg/coroutines/pr112341.C: New test.
>> ---
>> gcc/cp/coroutines.cc | 142 ++++++++++++++----
>> gcc/cp/pt.cc | 5 -
>> gcc/testsuite/g++.dg/coroutines/pr112341-2.C | 25 +++
>> gcc/testsuite/g++.dg/coroutines/pr112341-3.C | 65 ++++++++
>> gcc/testsuite/g++.dg/coroutines/pr112341.C | 21 +++
>> .../torture/co-yield-03-tmpl-nondependent.C | 140 +++++++++++++++++
>> 6 files changed, 361 insertions(+), 37 deletions(-)
>> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr112341-2.C
>> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr112341-3.C
>> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr112341.C
>> create mode 100644
>> gcc/testsuite/g++.dg/coroutines/torture/co-yield-03-tmpl-nondependent.C
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index 127a1c06b56e..9494cb499454 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -85,6 +85,7 @@ struct GTY((for_user)) coroutine_info
>> tree actor_decl; /* The synthesized actor function. */
>> tree destroy_decl; /* The synthesized destroy function. */
>> tree promise_type; /* The cached promise type for this function. */
>> + tree traits_type; /* The cached traits type for this function. */
>> tree handle_type; /* The cached coroutine handle for this function. */
>> tree self_h_proxy; /* A handle instance that is used as the proxy for
>> the
>> one that will eventually be allocated in the coroutine
>> @@ -429,11 +430,12 @@ find_promise_type (tree traits_class)
>> return promise_type;
>> }
>> +/* Perform initialization of the coroutine processor state, if not done
>> + before. */
>> +
>> static bool
>> -coro_promise_type_found_p (tree fndecl, location_t loc)
>> +ensure_coro_initialized (location_t loc)
>> {
>> - gcc_assert (fndecl != NULL_TREE);
>> -
>> if (!coro_initialized)
>> {
>> /* Trees we only need to create once.
>> @@ -466,6 +468,33 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
>> coro_initialized = true;
>> }
>> + return true;
>> +}
>> +
>> +/* Try to get the coroutine traits class. */
>> +static tree
>> +coro_get_traits_class (tree fndecl, location_t loc)
>> +{
>> + gcc_assert (fndecl != NULL_TREE);
>> +
>> + if (!ensure_coro_initialized (loc))
>> + /* We can't continue. */
>> + return error_mark_node;
>> +
>> + coroutine_info *coro_info = get_or_insert_coroutine_info (fndecl);
>> + auto& traits_type = coro_info->traits_type;
>> + if (!traits_type)
>> + traits_type = instantiate_coro_traits (fndecl, loc);
>> + return traits_type;
>> +}
>> +
>> +static bool
>> +coro_promise_type_found_p (tree fndecl, location_t loc)
>> +{
>> + gcc_assert (fndecl != NULL_TREE);
>> +
>> + if (!ensure_coro_initialized (loc))
>> + return false;
>> /* Save the coroutine data on the side to avoid the overhead on every
>> function decl tree. */
>> @@ -480,7 +509,7 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
>> /* Get the coroutine traits template class instance for the function
>> signature we have - coroutine_traits <R, ...> */
>> - tree templ_class = instantiate_coro_traits (fndecl, loc);
>> + tree templ_class = coro_get_traits_class (fndecl, loc);
>> /* Find the promise type for that. */
>> coro_info->promise_type = find_promise_type (templ_class);
>> @@ -914,6 +943,19 @@ coro_diagnose_throwing_final_aw_expr (tree expr)
>> return coro_diagnose_throwing_fn (fn);
>> }
>> +/* Build a co_await suitable for later expansion. */
>> +
>> +tree
>> +build_template_co_await_expr (location_t kw, tree expr, tree type, tree
>> kind)
>
> For consistency with other build*, let's put type before expr.
Sure, done.
>> +{
>> + tree aw_expr = build5_loc (kw, CO_AWAIT_EXPR, type, expr,
>> + NULL_TREE, NULL_TREE, NULL_TREE,
>> + kind);
>
> Typically in templates we use build_min_nt_loc for the dependent case, and
> build_min_non_dep for the non-dependent case...though I suppose that
> distinction is primarily about handling TREE_SIDE_EFFECTS, which is less
> significant when we're always setting it to true anyway. So I guess this is
> fine.
I don't think we can use the _nt variant here since the type is
variable, no?
>> + TREE_SIDE_EFFECTS (aw_expr) = true;
>> + return aw_expr;
>> +}
>> +
>> +
>> /* This performs [expr.await] bullet 3.3 and validates the interface
>> obtained.
>> It is also used to build the initial and final suspend points.
>> @@ -922,11 +964,24 @@ coro_diagnose_throwing_final_aw_expr (tree expr)
>> A, the original yield/await expr, is found at source location LOC.
>> We will be constructing a CO_AWAIT_EXPR for a suspend point of one of
>> - the four suspend_point_kind kinds. This is indicated by SUSPEND_KIND.
>> */
>> + the four suspend_point_kind kinds. This is indicated by SUSPEND_KIND.
>> +
>> + In the case that we're processing a template declaration, we can't save
>> + actual awaiter expressions as the frame type will differ between
>> + instantiations, but we can generate them to type-check them and compute
>> the
>> + resulting expression type. In those cases, we will return a
>> + template-appropriate CO_AWAIT_EXPR and throw away the rest of the
>> results.
>> + Such an expression requires the original co_await operand unaltered.
>> Pass
>> + it as ORIG_OPERAND. If it is the same as 'a', you can pass
>> error_mark_node
>> + (the default) to use 'a' as the value. */
>
> Why error_mark_node rather than NULL_TREE as the default? I'd prefer to
> reserve error_mark_node for actual errors.
Hm, unsure why I did that, and I see no reason to keep error_mark_node,
so I'll swap it out for NULL_TREE.
>> static tree
>> -build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind)
>> +build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
>> + tree orig_operand = error_mark_node)
>> {
>> + if (orig_operand == error_mark_node)
>> + orig_operand = a;
>> +
>> /* Try and overload of operator co_await, .... */
>> tree o;
>> if (MAYBE_CLASS_TYPE_P (TREE_TYPE (a)))
>> @@ -1131,11 +1186,13 @@ build_co_await (location_t loc, tree a,
>> suspend_point_kind suspend_kind)
>> 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);
>> tree await_expr = build5_loc (loc, CO_AWAIT_EXPR,
>> - TREE_TYPE (TREE_TYPE (awrs_func)),
>> + awrs_type,
>> a, e_proxy, o, awaiter_calls,
>> - build_int_cst (integer_type_node,
>> - (int) suspend_kind));
>> + suspend_kind_cst);
>> TREE_SIDE_EFFECTS (await_expr) = true;
>> if (te)
>> {
>> @@ -1144,7 +1201,26 @@ build_co_await (location_t loc, tree a,
>> suspend_point_kind suspend_kind)
>> await_expr = te;
>> }
>> SET_EXPR_LOCATION (await_expr, loc);
>> - return convert_from_reference (await_expr);
>> +
>> + if (processing_template_decl)
>> + /* FIXME: Can we reuse the results above rather than re-make at template
>> + instantiating? The "obvious" solution of checking if we have a fully
>> + built expression and recurring into the operands and 'e' call
>> + expressions seems to cause PR103868 again. */
>
> In general all that we keep is types and name lookup results, everything else
> gets rebuilt at instantiation time, I wouldn't try to do anything different
> for
> co_await.
OK. I've dropped the FIXME.
>> + return build_template_co_await_expr (loc, orig_operand, awrs_type,
>> + suspend_kind_cst);
>> + return convert_from_reference (await_expr);
>> +}
>> +
>> +/* Returns true iff EXPR or the current functions traits class are
>> dependent.
>> + In those cases, we can't decide what the type of our expressions is, so
>> we
>> + defer expansion entirely. */
>> +
>> +static bool
>> +defer_expansion_p (tree expr, location_t loc)
>
> This name doesn't suggest it's specific to coroutines. Maybe
> coro_dependent_p?
Yes, that's a better name, I've renamed it.
> I'm also skeptical about passing in loc here; we shouldn't need to give a
> diagnostic by this point.
>
> Relatedly, it looks like none of the callers of coro_get_traits_class deal
> with
> error_mark_node return. If you aren't going to handle that, maybe
> coro_get_traits_class should abort instead of returning it. Or call
> ensure_coro_initialized somewhere else.
Hm, I think a better design is indeed to extract the
ensure_coro_initialized call and make coro_initialized a precondition of
coro_dependent_p. This means that coro_get_traits_class returns
NULL_TREE as the only possible error return. I think I'll also extract
getting the traits class a level up so that the predicate is only busy
with checking whether the types are dependent, meaning there's no need
to contrive the promise_type being NULL_TREE into either a true or false
return from coro_dependent_p (currently true, because of
dependent_type_p (NULL_TREE) == true).
Working on those changes now.
Thanks for the comments.
--
Arsen Arsenović
signature.asc
Description: PGP signature
