Fwd: [C++ coroutines 3/7, v2] Front end parsing and transforms.

[Nathan Sidwell]

[for some reason something dropped the emails on their first reposting, 
it took me a while to realize I'd missed something]

On 1/10/20 7:29 AM, Iain Sandoe wrote:

> I was able to address all the comments that you made without finding any
> show-stoppers.  In addition to your comments, I’ve had one or two privately
> (and likewise a few bug reports) these have been addressed in the revised
> patch.

Thanks!

There are a few nits and some trivial inconsistencies.  The larger 
issues are:

1) we keep doing name lookup for the traits and handle template.  This 
is unnecessary, but not wrong.  It can be improved later.

2) you use 'struct __X' names in a lot of places.  Why the __?

3) there's an uninitialized read concerning ggc_alloc and the hash 
table.  See comments.  Needs fixing.

4) I don't think it's not PCH safe.  See comments.  I think it's an easy 
but tedious fix.  I guess we can't break it yet?

Otherwise ok.

nathan

> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c

> @@ -4583,6 +4584,14 @@ build_new_function_call (tree fn, vec<tree, va_gc> **args,
>        result = build_over_call (cand, flags, complain);
>      }
>  
> +  if (flag_coroutines
> +      && result
> +      && result != error_mark_node
> +      && TREE_CODE (result) == CALL_EXPR

FWIW you don't need the error_mark_node test in this case, because 
that'll fail the TREE_CODE test anyway.  Change at your discretion.

> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -7752,6 +7752,12 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>      case ANNOTATE_EXPR:
>        return RECUR (TREE_OPERAND (t, 0), rval);
>  
> +    /* coroutine await expressions are not.  */

Capital 'Coroutine'.

> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc


> +/* Get the existing coroutine_info for FN_DECL, or insert a new one if the
> +   entry does not yet exist.  */
> +
> +coroutine_info *
> +get_or_insert_coroutine_info (tree fn_decl)
> +{
> +  gcc_checking_assert (coroutine_info_table != NULL);
> +
> +  coroutine_info **slot = coroutine_info_table->find_slot_with_hash
> +    (fn_decl, coroutine_info_hasher::hash (fn_decl), INSERT);
> +
> +  if (*slot == NULL)
> +    {
> +      *slot = new (ggc_alloc<coroutine_info> ()) coroutine_info ();

How is the coroutine_info initialized?  ggc_alloc returns non-zero 
initialized storage?  ggc_cleared_alloc return zeroed storage?

> +/* Trees we only need to set up once.  */
> +
> +static tree void_coro_handle_type;

All these static tree variables are not captured by GC, because there's 
no GTY marker on them.  As it happens, you're ok because the trees are 
all reachable via some other path (identifiers are in the identifier 
hash for instance).

It does mean this is incompatible with PCH though -- if the PCH'd header 
causes coroutines to be inited, things will probably go wrong, or at 
least behave strangely.

I have noticed PCH can easily bitrot, so don't feel bad.


> +static tree
> +find_coro_traits_template_class (tree fndecl, location_t kw)

> +  tree traits_class
> +    = lookup_template_class (coro_traits_identifier, targ,
> +			     /* in_decl */ NULL_TREE,
> +			     /* context */ std_node,
> +			     /* entering scope */ false, tf_warning_or_error);

You're still always doing name lookup in the coro_traits.  As I 
mentioned before, you can cache that lookup, and simply pass the 
TEMPLATE_DECL into lookup_template_class.  lookup_template_class is 
somewhat misnamed -- it's instantiating a template, and might involve 
namelookup.  Is there a problem with doing that?

> +/* [coroutine.handle] */
> +
> +static tree
> +find_coro_handle_type (location_t kw, tree promise_type)
> +{
> +  /* So now build up a type list for the template, one entry, the promise.  */
> +  tree targ = make_tree_vec (1);
> +  TREE_VEC_ELT (targ, 0) = promise_type;
> +  tree handle_type
> +    = lookup_template_class (coro_handle_identifier, targ,
> +			     /* in_decl */ NULL_TREE,
> +			     /* context */ std_node,
> +			     /* entering scope */ false, tf_warning_or_error);

... and the same here.
Doing the lookup once in init_coroutines could give a more informative 
user diagnostic 'hey, user, you need these two templates to proceed'

> +/* Look for the promise_type in the instantiated.  */
in the instantiated $what?  [traits]

Needs a comment ...

> +static bool
> +coro_promise_type_found_p (tree fndecl, location_t loc)
> +{

> +  coroutine_info *coro_info = get_or_insert_coroutine_info (fndecl);
> +  /* Without this, we cannot really proceed.  */
> +  gcc_checking_assert (coro_info);
> +
> +  /* If we don't already have a current promise type, try to look it up.  */
> +  if (coro_info->promise_type == NULL_TREE)

Yeah, I think you're reading uninitialized state here.  See above 
ggc_alloc comment.


> +static tree
> +lookup_promise_method (tree fndecl, tree member_id, location_t loc,
> +		       bool musthave)
> +{
> +  tree promise = get_coroutine_promise_type (fndecl);
> +  tree pm_memb
> +    = lookup_member (promise, member_id,
> +		     /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
> +  if (musthave && (pm_memb == NULL_TREE || pm_memb == error_mark_node))
> +    {
> +      error_at (loc, "no member named %qs in %qT",
> +		IDENTIFIER_POINTER (member_id), promise);

you can use '%qE' and just pass the member_id.  No need to use %qs and 
the POINTER.

> +/* Here we check the constraints that are common to all keywords (since the
> +   presence of a coroutine keyword makes the function into a coroutine).  */
> +
> +static bool
> +coro_common_keyword_context_valid_p (tree fndecl, location_t kw_loc,
> +				     const char *kw_name)

Why is kw_name not an IDENTIFIER? oh, I see below the routines don't get 
the token.  Ok.

> +  /* Since we think the function is a coroutine, that implies we parsed
> +     a keyword that triggered this.  Keywords check promise validity for
> +     their context and thus the promise type should be known at this point.  */
> +  gcc_assert (get_coroutine_handle_type (fndecl) != NULL_TREE
> +	      && get_coroutine_promise_type (fndecl) != NULL_TREE);

I have a mild preference for gcc_checking_assert on expensive asserts 
like this.  Sadly gcc_assert doesn't evaporate to nothing in release 
builds.  For reasons.  Your call.


> +  tree o_type = complete_type_or_else (TREE_TYPE (o), o);
> +  if (TREE_CODE (o_type) != RECORD_TYPE)
^ must be a struct.  Use CLASS_TYPE_P (o_type)
> +    {
> +      error_at (loc, "awaitable type %qT is not a structure or union",
> +		o_type);
^ error says union would be ok.


> +  else if (TREE_CODE (susp_return_type) == RECORD_TYPE)
> +    /* ???: perhaps we should have some way to check that this is actually
> +	    a coroutine handle type.  */

Something like
  CLASSTYPE_PRIMARY_TEMPLATE (susp_return_type) == coroutine_handle_template
would do that.  But fine to leave as is.

> +tree
> +finish_co_await_expr (location_t kw, tree expr)


> +  tree at_meth
> +    = lookup_promise_method (current_function_decl,
> +			     coro_await_transform_identifier, kw,
> +			     /*musthave*/ false);
idiom is /*musthave=*/false (see pt.c etc)

> +  /* The incoming expr is "e" per [expr.yield] para 1, lookup and build a
> +     call for p.yield_value(e).  */
> +  tree y_meth = lookup_promise_method (current_function_decl,
> +				       coro_yield_value_identifier, kw,
> +				       true /*musthave*/);

/*musthave=*/true

> +  /* Suppress -Wreturn-type for co_return, we need to check indirectly
> +     whether the promise type has a suitable return_void/return_value.  */
> +  if (warn_return_type)
> +    TREE_NO_WARNING (current_function_decl) = true;

Does this inhibit other warnings? (why the warn_return_type 
conditional?)  Is it possible to teach Wreturn-type not to, or how to, 
check coroutines?

> +	= lookup_promise_method (current_function_decl,
> +				 coro_return_void_identifier, kw,
> +				 /*musthave=*/ true);

... and that's the third way you spelt that!

> +	= lookup_promise_method (current_function_decl,
> +				 coro_return_value_identifier, kw,
> +				 /*musthave=*/ true);

> +/* ================= Morph and Expand. =================

My knowledge of the expanders is not as good as the FE proper.  I may 
miss things here.

> +struct __proxy_replace
Why __?
> +{
> +  tree from, to;
> +};

Say hi to std::pair<T, U>  (no need to use that though)


> +/* Support for expansion of co_return statements.  */
> +
> +struct __coro_ret_data

likewise, why __?

> +static tree
> +coro_maybe_expand_co_return (tree co_ret_expr, __coro_ret_data *data)

> +/* Support for expansion of co_await statements.  */
> +
> +struct __coro_aw_data

..and here too

> +      else if (tree r
> +	       = cp_walk_tree (&TREE_OPERAND (stripped_stmt, 1),
> +			       co_await_find_in_subtree, &buried_stmt, NULL))
> +	{
> +	  saved_co_await = r;
> +	}

Unnecessary bracing.


> +  r = build_case_label (build_int_cst (integer_type_node, 0), NULL_TREE,
> +			create_anon_label_with_ctx (loc, actor));
> +  add_stmt (r); // case 0:
> +  // Implement the suspend, a scope exit without clean ups.

bad comment format, perhaps we can change to allow //

> +static hash_map<tree, struct suspend_point_info> *suspend_points;

This is not GTY marked.  Is it cleared out after each use?  Does it need 
GTY((discard))?  I'm concerned it being live over a GC.

> +struct __await_xform_data

You've done it again, is there some idiom I'm missing?

> +/* When we built the await expressions, we didn't know the coro frame
> +   layout, therefore no idea where to find the promise or where to put
> +   the awaitables.  Now we know these things, fill them in.  */
> +
> +static tree
> +transform_await_expr (tree await_expr, struct __await_xform_data *xform)

> +typedef struct __param_info


> +{
> +  tree field_id;
> +  vec<tree *> *body_uses;
> +  tree frame_type;
> +} __param_info_t;

No need for typedef struct X {} X_t;

> +static tree
> +transform_local_var_uses (tree *stmt, int *do_subtree, void *d)

> +  /* For each var in this bind expr (that has a frame id, which means it was
> +     accessed), build a frame reference for each and then walk the bind expr
> +     statements, substituting the frame ref for the original var.
> +  */

bad line break on comment.

> +
> +static bool
> +register_await_info (tree await_expr, tree aw_type, tree aw_nam, tree susp_type,
> +		     tree susp_handle_nam)
fn needs a comment

> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 094e32edf5..41c14001fd 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -16783,6 +16783,29 @@ add_return_star_this_fixit (gcc_rich_location *richloc, tree fndecl)
>  				       indent);
>  }
>  
> +static void
> +emit_coro_helper (tree helper)
fn needs a comment.

thanks,

nathan

-- 
Nathan Sidwell